-
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
[Metrics V2] Query processor handle no join strategy #42666
Conversation
Fixes #42462 Make QP preprocessor handle queries referencing metrics in the style of legacy metrics. Enforce various compatibility checks.
|
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, but I have a few questions. Also, I would prefer to have test cases explicitly testing and named for the multi-stage metric execution. I would like to see test showing what happens when expression names in a metric clash with the expression names in the host query.
((requiring-resolve 'metabase.query-processor.preprocess/preprocess) | ||
(lib/query query (:dataset-query card-metadata))))] | ||
{:query metric-query | ||
:aggregation (first (lib/aggregations metric-query)) |
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.
Should this fail if there is not exactly one aggregation?
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 think more than one aggregation should be impossible to build so I think it is safe to assume here.
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 meant, if it should throw. Maybe the > 1 case shouldn't, but then it should log, I guess.
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.
This is good, any remaining issues can be addressed in a follow up PR.
Fixes #42667
Make QP preprocessor handle queries referencing metrics in the style of legacy metrics. Enforce various compatibility checks.