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

[QP] Support :temporal-unit parameters on MBQL queries #42116

Merged
merged 1 commit into from May 2, 2024

Conversation

bshepherdson
Copy link
Contributor

This is a new parameter :type (and :widget-type) to choose a time
granularity (a :temporal-unit, eg. month, day, day-of-week)
which should replace the unit on the target field for the parameter.

The target field should be a breakout with a temporal type (date,
datetime) that's compatible with the input unit. The value of the
parameter should be a string or keyword naming one of the units:
"month", :month.

@bshepherdson bshepherdson requested a review from camsaul as a code owner May 1, 2024 20:02
Copy link
Contributor Author

bshepherdson commented May 1, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bshepherdson and the rest of your teammates on Graphite Graphite

@bshepherdson bshepherdson added the no-backport Do not backport this PR to any branch label May 1, 2024
@metabase-bot metabase-bot bot added the .Team/QueryProcessor :hammer_and_wrench: label May 1, 2024
@bshepherdson bshepherdson requested a review from snoe May 1, 2024 20:03
Copy link

replay-io bot commented May 1, 2024

Status Complete ↗︎
Commit ba9f1ca
Results
⚠️ 5 Flaky
2446 Passed

This is a new parameter `:type` (and `:widget-type`) to choose a time
granularity (a `:temporal-unit`, eg. `month`, `day`, `day-of-week`)
which should replace the unit on the target field for the parameter.

The target field should be a breakout with a temporal type (date,
datetime) that's compatible with the input unit. The value of the
parameter should be a string or keyword naming one of the units:
`"month"`, `:month`.
@bshepherdson bshepherdson force-pushed the qp-parameterize-time-granularity branch from c4416d5 to ba9f1ca Compare May 2, 2024 13:55
@bshepherdson bshepherdson requested review from metamben and removed request for snoe May 2, 2024 20:30
Copy link
Contributor

@metamben metamben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

(let [new-unit (keyword value)]
(lib.util.match/replace-in
query [:query :breakout]
[:field (_ :guard #(= target-field-id %)) (opts :guard #(= temporal-unit (:temporal-unit %)))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it essential to check for the expected temporal-unit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, not really. Soon we plan to allow multiple breakouts for the same column with different units, and this would become a latent bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be a better world where parameters were attached to specific occurrences of a column (via uuid)?

@bshepherdson bshepherdson merged commit 4fa5778 into master May 2, 2024
109 checks passed
@bshepherdson bshepherdson deleted the qp-parameterize-time-granularity branch May 2, 2024 21:03
Copy link

github-actions bot commented May 2, 2024

@bshepherdson Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryProcessor :hammer_and_wrench:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants