-
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] Port Legacy metric tests and fixes #43094
Conversation
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.
Nice!
(let [metadata-type (case macro-type | ||
:metric :metadata/legacy-metric | ||
:segment :metadata/segment)] |
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 will look quite surprising to the uninitiated.
{:keys [before after]} {:before {:source-table (meta/id :venues) | ||
:aggregation [[:metric (:id source-metric)]]} | ||
:after {:source-table (meta/id :venues) | ||
:aggregation [[:aggregation-options [:sum [:field (meta/id :venues :price) {}]] | ||
{:display-name "My Cool Aggregation"}]] | ||
:filter [:= [:field (meta/id :venues :name) {}] [:value "abc" {}]]}} |
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.
Why not just define before
and after
separately?
|
@snoe Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Fixes #43093
Errors fixed: