-
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
[QP] Support :temporal-unit
parameters on MBQL queries
#42116
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @bshepherdson and the rest of your teammates on Graphite |
|
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`.
c4416d5
to
ba9f1ca
Compare
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.
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 %)))] |
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.
Is it essential to check for the expected temporal-unit
?
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.
Currently, not really. Soon we plan to allow multiple breakouts for the same column with different units, and this would become a latent bug.
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 it be a better world where parameters were attached to specific occurrences of a column (via uuid)?
@bshepherdson Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
This is a new parameter
:type
(and:widget-type
) to choose a timegranularity (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
.