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

fix(schema-compiler): Handle measures with dimension-only member expressions #9335

Merged
merged 7 commits into from
Mar 18, 2025

Conversation

mcheshkov
Copy link
Member

Check List

  • Tests have been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made (if issue reference is not provided)

fullKeyQueryAggregate is using measure.cube() a lot, like for grouping measures to key cube
This can be wrong for measure targeting view: we want to attach measure to subquery
for its definition cube, and there should be no subquery for view itself
It's expected that fullKeyQueryAggregateMeasures would resolve and prepare all measures from its original form in query to actual leaf measures in cubes, then build subqueries with those, and then joinFullKeyQueryAggregate would use renderedReference to point to these leaf measures

Hard case is a measure that:

  • targeting view
  • is a member expression
  • references only dimensions from that view
    Measure like that are "leaf" - there's nowhere to push it, member expression has to be directly in leaf subquery
    If measure references dimension from a single cube it can be multiplied (think SUM(${view.deep_dimension}))
    So, three points:
  • it must be accounted for in multipliedMeasures
  • it must be attached to a proper cube subquery
  • it must use renderedReference correctly

collectRootMeasureToHieararchy will not drop such measure completely. Now it will check is there's 0 measures collected, try to collect all referenced members to gather used cubes and detect multiplication, and add new measure in hierarchy.

Because new returned measure is patched, it will be attached to correct cube subquery in fullKeyQueryAggregate

Then outerMeasuresJoinFullKeyQueryAggregate needs to generate proper alias for it, so outer unpatched measure would pick up alias from inner patched one

Finally, checkShouldBuildJoinForMeasureSelect has to be adjusted: it collected cube names and join hints, and for dimensions-only measures on views it would collect too many, and decide that measure is multiplied, when it should not. So now it first collects non-view members, and then collect cubes and join hints from those.

@mcheshkov mcheshkov force-pushed the fix-dimensions-only-measures branch 2 times, most recently from 8c331af to 8f73be2 Compare March 12, 2025 16:17
@mcheshkov mcheshkov marked this pull request as ready for review March 12, 2025 16:18
@mcheshkov mcheshkov requested a review from a team as a code owner March 12, 2025 16:18
@ovr ovr requested a review from waralexrom March 12, 2025 16:41
@@ -1709,7 +1716,64 @@ export class BaseQuery {
const cubeName = m.expressionCubeName ? `\`${m.expressionCubeName}\` ` : '';
throw new UserError(`The query contains \`COUNT(*)\` expression but cube/view ${cubeName}is missing \`count\` measure`);
}
return [typeof m.measure === 'string' ? m.measure : `${m.measure.cubeName}.${m.measure.name}`, collectedMeasures];
if (collectedMeasures.length === 0 && m.isMemberExpression) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should put this part related to the member expression over dimension into a separate function and just call it here? There is a lot of code inside the lambda function - it's quite hard to read.

Comment on lines 2058 to 2064
const nonViewMembers = memberNamesForMeasure
.filter(mem => {
const cubeName = this.cubeEvaluator.parsePathAnyType(mem)[0];
const cubeDef = this.cubeEvaluator.getCubeDefinition(cubeName);
return !cubeDef.isView;
})
.map(m => this.memberInstanceByPath(m));
Copy link
Member

@waralexrom waralexrom Mar 13, 2025

Choose a reason for hiding this comment

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

Maybe it would be better to use ownedByCube here?
Something like

const nonViewMembers = memberNamesForMeasure
         .map(m => this.memberInstanceByPath(m))
        .filter(mem => {
          
          return mem.definition().ownedByCube;
        });     

@mcheshkov mcheshkov force-pushed the fix-dimensions-only-measures branch 3 times, most recently from 20a456f to 695b941 Compare March 17, 2025 20:00
…sage in checkShouldBuildJoinForMeasureSelect
…essions in fullKeyQueryAggregateMeasures

`fullKeyQueryAggregate` is using measure.cube() a lot, like for grouping measures to key cube
This can be wrong for measure targeting view: we want to attach measure to subquery
for its definition cube, and there should be no subquery for view itself
It's expected that `fullKeyQueryAggregateMeasures` would resolve and prepare all measures from its original form in query to actual leaf measures in cubes, then build subqueries with those, and then `joinFullKeyQueryAggregate` would use `renderedReference` to point to these leaf measures

Hard case is a measure that:
* targeting view
* is a member expression
* references only dimensions from that view
Measure like that are "leaf" - there's nowhere to push it, member expression has to be directly in leaf subquery
If measure references dimension from a single cube it can be multiplied (think `SUM(${view.deep_dimension})`)
So, three points:
* it must be accounted for in `multipliedMeasures`
* it must be attached to a proper cube subquery
* it must use renderedReference correctly

`collectRootMeasureToHieararchy` will not drop such measure completely. Now it will check is there's 0 measures collected, try to collect all referenced members to gather used cubes and detect multiplication, and add new measure in hierarchy.

Because new returned measure is patched, it will be attached to correct cube subquery in `fullKeyQueryAggregate`

Then `outerMeasuresJoinFullKeyQueryAggregate` needs to generate proper alias for it, so outer unpatched measure would pick up alias from inner patched one
@mcheshkov mcheshkov force-pushed the fix-dimensions-only-measures branch from 695b941 to 7893f39 Compare March 18, 2025 15:41
@mcheshkov mcheshkov merged commit 2f7a128 into master Mar 18, 2025
62 checks passed
@mcheshkov mcheshkov deleted the fix-dimensions-only-measures branch March 18, 2025 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants