-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: PatchMeasure member expression #9218
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
Conversation
011a583 to
8792910
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9218 +/- ##
===========================================
+ Coverage 47.86% 76.56% +28.69%
===========================================
Files 171 400 +229
Lines 21312 105029 +83717
Branches 3687 3704 +17
===========================================
+ Hits 10201 80414 +70213
- Misses 10676 24179 +13503
- Partials 435 436 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
214a45a to
6275676
Compare
| if (s.path()) { | ||
| return [s.cube().name].concat(this.evaluateSymbolSql(s.path()[0], s.path()[1], s.definition())); | ||
| } else if (s.patchedMeasure?.patchedFrom) { | ||
| return [s.patchedMeasure.patchedFrom[0]].concat(this.evaluateSymbolSql(s.patchedMeasure.patchedFrom[0], s.patchedMeasure.patchedFrom[1], s.definition())); |
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.
Let's at least add the word Path to the name of the patchedFrom field (ideally, of course, make it a structure with cubeName and Name). Otherwise, every time we come across such a code fragment, we will have to go to the patchedMeasure definition to understand what patchedFrom and its 0th and 1st elements represent
6275676 to
88c5012
Compare
88c5012 to
85910bb
Compare
94afdf6 to
553c15f
Compare
…de::generate_sql_for_expr
…de::generate_column_expr
553c15f to
d522158
Compare
Add new member expression: `PatchMeasure`. It allows to change aggregation type and add measure filters for existing measure. This is to allow queries like `MIN(avgMeasure)` and `SUM(CASE dim = 'foo' WHEN TRUE THEN sumMeasure ELSE NULL END)` in SQL API. Under the hood * SQL API now allows mismatched aggregation functions and aggregation on top of `CASE` in SQL pushdown rules, and rewrites that to special expression `__patch_measure(measure_column, 'new_agg_type', boolean_expression)` * This expression is syntactic-only, it should never execute, instead it would be handled by wrapper SQL generation * Generated member expression is passed to JS-side * `api-gateway` evaluates it from JSON from JS functions * `BaseQuery` and `BaseMeasure` now can handle new form: generate patched measure definition and use original measure during traversing and collecting Supporting changes * Changed rewrites for usual measures in SQL pushdown from just `measure_column` to `MEASURE(measure_column)`, to make rewrites uniform * Turn member expressions to tagged union in JSON representation
Check List
Description of Changes Made (if issue reference is not provided)
Add new member expression:
PatchMeasure.It allows to change aggregation type and add measure filters for existing measure. This is to allow queries like
MIN(avgMeasure)andSUM(CASE dim = 'foo' WHEN TRUE THEN sumMeasure ELSE NULL END)in SQL API.Under the hood
CASEin SQL pushdown rules, and rewrites that to special expression__patch_measure(measure_column, 'new_agg_type', boolean_expression)api-gatewayevaluates it from JSON from JS functionsBaseQueryandBaseMeasurenow can handle new form: generate patched measure definition and use original measure during traversing and collectingSupporting changes
measure_columntoMEASURE(measure_column), to make rewrites uniform