[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 |
bshepherdson
added
the
backport
bshepherdson
requested review from
metamben,
ranquild and
snoe
and removed request for
camsaul
|
| - `:data` - just the source data for the stage | ||
| - `:joins` | ||
| - `:expressions` | ||
| - `:filters` | ||
| - `:aggregation` | ||
| - `:breakout` | ||
| - `:order-by` | ||
| - `:limit` |
There was a problem hiding this comment.
These look like keywords, but at this point we get strings.
There was a problem hiding this comment.
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.
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.
: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
:aggregationwe remove:limitand:order-by - If you preview
:filterswe remove:limit,:order-by,:aggregationand: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.
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.
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.
| :expressions [vector?]}]})))) | |
| :expressions [vector?]}]})))) | |
bshepherdson
force-pushed
the
mblib-preview-query
branch
from
50f6e50
to
68099ab
Compare
bshepherdson
force-pushed
the
mblib-preview-query
branch
from
68099ab
to
c841b23
Compare
bshepherdson
force-pushed
the
mblib-preview-query
branch
from
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.