Skip to content
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

[MLv2] [BE] Add drill-thru/pivot aka Break out by... #33559

Closed
Tracked by #31004
camsaul opened this issue Aug 28, 2023 · 7 comments · Fixed by #36281 or #36509
Closed
Tracked by #31004

[MLv2] [BE] Add drill-thru/pivot aka Break out by... #33559

camsaul opened this issue Aug 28, 2023 · 7 comments · Fixed by #36281 or #36509
Assignees
Labels
.Backend .metabase-lib Label for tracking all issues related to the shared CLJC metabase-lib Querying/Drill Thrus Refining existing queries with Drill Thrus .Team/QueryProcessor :hammer_and_wrench:

Comments

@camsaul
Copy link
Member

camsaul commented Aug 28, 2023

See @bshepherdson 's brain dump thread for more info https://metaboat.slack.com/archives/C04CYTEL9N2/p1691679718619139

Specifically

pivoting returns a big map of possible columns to pivot on, grouped by the kind of column (category, time, location). the FE needs this to produce the 2-click nested view for that particular drill (try it!)

I think we need to fill out the drill-thru-method for pivots, I think one of these files has the FE code we need to port but don't quote me on that.

export function getPivotColumnSplit(question) {

https://github.com/metabase/metabase/blob/0b0edf1b916aa6a4000767ec42a41c2721d22391/frontend/src/metabase-lib/queries/drills/pivot-drill.ts

  1. Port JS code to Cljc
  2. Fill out the skeleton drill-thru-method
  3. Make sure it comes back when expected in available-drill-thrus
  4. Write a billion tests
@camsaul camsaul mentioned this issue Aug 28, 2023
1 task
@camsaul camsaul added .Backend .metabase-lib Label for tracking all issues related to the shared CLJC metabase-lib .Team/QueryProcessor :hammer_and_wrench: Querying/Drill Thrus Refining existing queries with Drill Thrus labels Aug 28, 2023
@camsaul
Copy link
Member Author

camsaul commented Aug 28, 2023

drill-thru-method lives in this PR #31735 currently but we should try to get that PR merged into master so this can be done against master.

@camsaul
Copy link
Member Author

camsaul commented Sep 28, 2023

It seems like pivot is returned but there's no drill-thru-method to apply it yet

@camsaul camsaul self-assigned this Oct 23, 2023
@camsaul camsaul removed their assignment Nov 21, 2023
@bshepherdson bshepherdson self-assigned this Nov 24, 2023
@bshepherdson
Copy link
Contributor

There are already:

  • ML.pivot_types(pivotDrill) returning a list of :category, :location, :time.
    • (That should be converting the keywords to strings; we can fix that. In the meantime on the branch, you can call ML.pivot_type(pivotDrill).map(window.$CLJS.cljs.core.name) to get the strings.)
  • ML.pivot_columns_for_type(pivotDrill, typeFromAbove) gives the list of columns.

On the proposed alternative API:

  • Why the query and stage index?
    • I don't think there's any cases where the query needs to be transformed "in advance" (like column-filter and quick-filter). As I understand it the FE only renders 1-3 lists of plain columns, and when the user selects a pivot column, the column would get passed to drillThru() and a new query returned. Is there some more complex UI I'm missing that might need a modified query?
  • Why return a single list of columns rather than 1-3 separate ones? MLv2 generates them separately since they're powered by different rules.

Overall the proposed API seems less handy than the existing one (except for it returning keywords, which is trivial to fix), but maybe I've got a bad picture of the UI's requirements.

@bshepherdson
Copy link
Contributor

I discussed the existing API with @ranquild and we've agreed that it works well.

There are two remaining tasks here:

  • lib.js/pivot-types is returning a JS array of CLJS keywords to the FE; those should be strings.
  • There's no drill-thru-method implementation for pivot drills.

@ranquild
Copy link
Contributor

Repro test suite

describe.skip("drill-thru/pivot (metabase#33559)", () => {

@ranquild
Copy link
Contributor

ranquild commented Dec 4, 2023

Reopening the issue since the repro test suite still fails. The root cause seems to be that CLJS keywords aren't converted to plain strings.

@ranquild ranquild reopened this Dec 4, 2023
@ranquild
Copy link
Contributor

ranquild commented Dec 4, 2023

cc @bshepherdson

bshepherdson added a commit that referenced this issue Dec 6, 2023
These were broken because `lib.js/pivot-types` was returning CLJS
keywords rather than strings.

Also makes applying drill-thrus less noisy (`log/info` -> `log/debug`).

Unskips the FE integration tests for pivot drills. Fixes #33559.
bshepherdson added a commit that referenced this issue Dec 8, 2023
These were broken because `lib.js/pivot-types` was returning CLJS
keywords rather than strings.

Also makes applying drill-thrus less noisy (`log/info` -> `log/debug`).

Unskips the FE integration tests for pivot drills. Fixes #33559.
qnkhuat pushed a commit that referenced this issue Dec 12, 2023
These were broken because `lib.js/pivot-types` was returning CLJS
keywords rather than strings.

Also makes applying drill-thrus less noisy (`log/info` -> `log/debug`).

Unskips the FE integration tests for pivot drills. Fixes #33559.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Backend .metabase-lib Label for tracking all issues related to the shared CLJC metabase-lib Querying/Drill Thrus Refining existing queries with Drill Thrus .Team/QueryProcessor :hammer_and_wrench:
Projects
None yet
3 participants