-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(explore): adhoc column expressions [ID-3] #17379
Conversation
…#16412) * Support Jinja template functions in async queries * Pylint * Add tests for async tasks * Remove redundant has_request_context check
* feat: Add Aurora Data API engine spec * Fix lint
…#16548) * refactor sql_json view endpoint: encapsulate ctas parameters * fix failed tests * fix failed tests and ci issues
* fix:fix get permission function * feat: add cross filter chart in charts gallery under FF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass, just one minor nit
...rset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx
Outdated
Show resolved
Hide resolved
/testenv up FEATURE_UX_BETA=true FEATURE_ENABLE_EXPLORE_DRAG_AND_DROP=true FEATURE_ENABLE_DND_WITH_CLICK_UX=true |
@kgabryje Ephemeral environment spinning up at http://35.86.253.253:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM! Going to do manual testing
...rset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx
Outdated
Show resolved
Hide resolved
Do we need to re-spin up the test env? Saw the test env is not spin up at the latest commit |
/testenv up FEATURE_UX_BETA=true FEATURE_ENABLE_EXPLORE_DRAG_AND_DROP=true FEATURE_ENABLE_DND_WITH_CLICK_UX=true |
Good point! The latest changes shouldn't affect the functionality in any way, but better to be safe than sorry |
@kgabryje Ephemeral environment spinning up at http://52.26.96.154:8080. Credentials are |
Had a quick look at the ephemeral env. Thanks a lot for implementing it. I am eagerly awaiting it to be merged and released :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
be16261
to
d426d2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Awesome work
Ephemeral environment shutdown and build artifacts deleted. |
🏷️ 2021.46 |
* add support for adhoc columns to api and sqla model * fix some types * fix duplicates in column names * fix more lint * fix schema and dedup * clean up some logic * first pass at fixing viz.py * Add frontend support for adhoc columns * Add title edit * Fix showing custom title * Use column name as default value in sql editor * fix: Adds a loading message when needed in the Select component (#16531) * fix(tests): make parquet select deterministic with order by (#16570) * bump emotion to help with cache clobbering (#16559) * fix: Support Jinja template functions in global async queries (#16412) * Support Jinja template functions in async queries * Pylint * Add tests for async tasks * Remove redundant has_request_context check * fix: impersonate user label/tooltip (#16573) * docs: update for small typos (#16568) * feat: Add Aurora Data API engine spec (#16535) * feat: Add Aurora Data API engine spec * Fix lint * refactor: sql_json view endpoint: encapsulate ctas parameters (#16548) * refactor sql_json view endpoint: encapsulate ctas parameters * fix failed tests * fix failed tests and ci issues * refactor sql_json view endpoint: separate concern into ad hod method (#16595) * feat: Experimental cross-filter plugins (#16594) * fix:fix get permission function * feat: add cross filter chart in charts gallery under FF * chore(deps): bump superset-ui to 0.18.2 (#16601) * update type guard references * fix imports * update series_columns schema * Add changes that got lost in rebase * Use current columns name or expression as sql editor init value * add integration test and do minor fixes * Bump superset-ui * fix linting issue * bump superset-ui to 0.18.22 * resolve merge conflict * lint * fix select filter infinite loop * bump superset-ui to 0.18.23 * Fix auto setting column popover title * Enable adhoc columns only if UX_BETA enabled * put back removed test * Move popover height and width to constants * Refactor big ternary expression Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> Co-authored-by: Rob DiCiuccio <rob.diciuccio@gmail.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: joeADSP <75027008+joeADSP@users.noreply.github.com> Co-authored-by: ofekisr <35701650+ofekisr@users.noreply.github.com> Co-authored-by: simcha90 <56388545+simcha90@users.noreply.github.com>
for column in columns or []: | ||
label = get_column_name(column) | ||
if label not in labels: | ||
deduped_columns.append(column) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we also need to append label in labels
?
but this didn't cause any issue, because there are checks in /explore_json
api somewhere which throws { "error": "Group By' and 'Columns' can't overlap" }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mayurnewase do you have repro steps for this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just going through this feature and found pivot table allows same group by and column name, but looking at this function it's not expected.
So I thought this might be incorrect as its not really putting anything in labels
list.
So may need to add labels.append(label)
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUMMARY
Adds support for adhoc column expressions, similar to how adhoc metrics work. See video for example where
gender
that features values "boy" and "girl" is replaced with a case-statement that maps these values to "male" and "female":adhoc_column.mp4
The resulting query:
Part of the work is done in apache-superset/superset-ui#1342 (featured in the included bump of
superset-ui
).TESTING INSTRUCTIONS
ENABLE_EXPLORE_DRAG_AND_DROP
,UX_BETA
,ENABLE_DND_WITH_CLICK_UX
feature flagsADDITIONAL INFORMATION