-
Notifications
You must be signed in to change notification settings - Fork 4.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
[MBQL lib] Add preview-query
to truncate a query for Notebook preview
#42836
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @bshepherdson and the rest of your teammates on Graphite |
|
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 have only minor suggestions and questions.
- `:data` - just the source data for the stage | ||
- `:joins` | ||
- `:expressions` | ||
- `:filters` | ||
- `:aggregation` | ||
- `:breakout` | ||
- `:order-by` | ||
- `:limit` |
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.
These look like keywords, but at this point we get strings.
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.
We do this everywhere else in the API as well. The intuition is that these are not "data" strings, they're "enum" strings. That's reinforced by the types like type ClauseType = "data" | ...;
.
src/metabase/lib/query.cljc
Outdated
@@ -253,3 +253,43 @@ | |||
[a-query :- ::lib.schema/query | |||
metric-id :- [:or ::lib.schema.id/legacy-metric :string]] | |||
(occurs-in-stage-clause? a-query :aggregation #(occurs-in-expression? % :metric metric-id))) | |||
|
|||
(def ^:private clause-types-order | |||
;; Note that :breakout is never actually sent, but when we get :aggregation we want to drop :breakout too. |
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.
Would using :summary
be cleaner then? The precedence between :aggregation
and :breakout
is somewhat arbitrary (and it's right to left, and reverse lexicographic)).
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
would be more transparent, I agree. However, this is effectively a list of keys on a stage, in a specific order. The order of :aggregation
and :breakout
is not arbitrary.
When previewing some part :x
we want to remove a prefix of the keys in this list, up to but excluding the specified clause :x
.
- So if you preview
:aggregation
we remove:limit
and:order-by
- If you preview
:filters
we remove:limit
,:order-by
,:aggregation
and:breakout
.
This is the only order of :aggregation
and :breakout
that gives those properties.
I've added an expanded comment to explain that.
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.
Isn't the order arbitrary in the sense that I could choose to use :breakout
instead of :aggregation
to signify that the summary has to be removed and swap them in the vector? (I'm assuming we can change the callers.)
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.
Sure, that's true. The important property is that whichever of :aggregation
and :breakout
is sent by the FE when previewing the Summary step is first in this list.
(test-preview 0 :expressions nil | ||
{:stages [{:source-table (meta/id :orders) | ||
:joins [{} {}] | ||
:expressions [vector?]}]})))) |
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.
:expressions [vector?]}]})))) | |
:expressions [vector?]}]})))) | |
50f6e50
to
68099ab
Compare
68099ab
to
c841b23
Compare
c841b23
to
0a3573b
Compare
@bshepherdson Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Fixes #42831.