-
Notifications
You must be signed in to change notification settings - Fork 14k
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: query datasets from SQL Lab #15241
feat: query datasets from SQL Lab #15241
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15241 +/- ##
==========================================
+ Coverage 66.46% 66.51% +0.04%
==========================================
Files 1721 1724 +3
Lines 64466 64647 +181
Branches 6794 6794
==========================================
+ Hits 42846 42997 +151
- Misses 19892 19922 +30
Partials 1728 1728
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Any progress in this? Dreaming this functionality a bit further, I could imagine having a dataset browser included in the schema browser, which presents all datasets of the selected database, so they can be selected from like the existing tables/views of the DB. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
This issue should not be closed by the stale bot, in my eyes. The suggested functionality is just too promising. |
I agree, @rumbin, let me do some more work on this. |
d567e51
to
3cbdda6
Compare
3cbdda6
to
570e7c0
Compare
Want, want want....take my money! |
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.
Really cool feature, great way to leverage the SQLA model! Left a couple non-blocking comments, LGTM.
superset/jinja_context.py
Outdated
"columns": columns, | ||
"groupby": groupby, |
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.
Since groupby
is deprecated in QueryObject
and just gets mapped into columns
, maybe we could just do this:
"columns": columns, | |
"groupby": groupby, | |
"columns": groupby or columns, |
I believe it has the same effect
superset/jinja_context.py
Outdated
def dataset_macro( | ||
dataset_id: int, | ||
include_metrics: bool = False, | ||
groupby: Optional[List[str]] = None, |
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.
actually I was thinking, should we replace groupby
with columns
here? It feels it kinda makes more sense, as you should be able to pick a subset of columns from the dataset even when you don't have metrics with a GROUP BY
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.
Will do, great idea!
98f2854
to
ed43b79
Compare
* feat: Jinja2 macro for querying datasets * Add docs * Address comments
SUMMARY
Introduce the
dataset
macro, allowing users to query physical and virtual datasets in SQL Lab. This is useful if you've defined computed columns and metrics on your datasets, and want to reuse the definition in adhoc SQL Lab queries.Because currently dataset names are not unique the macro uses the dataset ID:
If users want to select the metric definitions as well, in addition to the columns, they can pass an additional keyword argument:
Since metrics are aggregations, the resulting SQL expression will be grouped by all non-metric columns. Users can also specify a subset of columns to group by instead:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Added unit tests.
ADDITIONAL INFORMATION