fix(schema-compiler): Use join paths from pre-aggregation declaration instead of building join tree from scratch

mcheshkov · mcheshkov · commit 85d23eb02630 · 2025-04-07T13:26:26.000+02:00
Without this pre-aggregatoin declaration would build a list of members, and build join tree from scratch. New join tree could be different, and pre-aggregation would contain incorrect data.

Add join hints to member adapters, to build join tree with proper members. This is more of a hack than proper solution: join tree would still disallow to have same cube twice with different join paths.
Just adding extra join hints on query level does not work because of FKQA and multiplication check in subqueries: we have to know join path for every member, so it should be either explicit or unambigous _for each member_.

Join tree from query is still not included in pre-aggregation matching, so user would have to manually ensure that views and pre-aggregations both match each other and does not produce false matches
diff --git a/packages/cubejs-schema-compiler/src/adapter/BaseDimension.ts b/packages/cubejs-schema-compiler/src/adapter/BaseDimension.ts
@@ -10,6 +10,8 @@ export class BaseDimension {
 
   public readonly isMemberExpression: boolean = false;
 
+  public readonly joinHint: Array<string> = [];
+
   public constructor(
     protected readonly query: BaseQuery,
     public readonly dimension: any
@@ -20,6 +22,18 @@ export class BaseDimension {
       // In case of SQL push down expressionName doesn't contain cube name. It's just a column name.
       this.expressionName = dimension.expressionName || `${dimension.cubeName}.${dimension.name}`;
       this.isMemberExpression = !!dimension.definition;
+    } else {
+      // TODO move this `as` to static types
+      const dimensionPath = dimension as string | null;
+      if (dimensionPath !== null) {
+        const parts = dimensionPath.split('.');
+        if (parts.length > 2) {
+          // Measure path contains join path
+          const hint = parts.slice(0, -1);
+          this.dimension = parts.slice(-2).join('.');
+          this.joinHint = hint;
+        }
+      }
     }
   }
 
diff --git a/packages/cubejs-schema-compiler/src/adapter/BaseMeasure.ts b/packages/cubejs-schema-compiler/src/adapter/BaseMeasure.ts
@@ -10,6 +10,8 @@ export class BaseMeasure {
 
   public readonly isMemberExpression: boolean = false;
 
+  public readonly joinHint: Array<string> = [];
+
   public constructor(
     protected readonly query: BaseQuery,
     public readonly measure: any
@@ -20,6 +22,16 @@ export class BaseMeasure {
       // In case of SQL push down expressionName doesn't contain cube name. It's just a column name.
       this.expressionName = measure.expressionName || `${measure.cubeName}.${measure.name}`;
       this.isMemberExpression = !!measure.definition;
+    } else {
+      // TODO move this `as` to static types
+      const measurePath = measure as string;
+      const parts = measurePath.split('.');
+      if (parts.length > 2) {
+        // Measure path contains join path
+        const hint = parts.slice(0, -1);
+        this.measure = parts.slice(-2).join('.');
+        this.joinHint = hint;
+      }
     }
   }
 
diff --git a/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js b/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js
@@ -307,16 +307,16 @@ export class BaseQuery {
   }
 
   prebuildJoin() {
-    if (this.useNativeSqlPlanner) {
-      // Tesseract doesn't require join to be prebuilt and there's a case where single join can't be built for multi-fact query
-      // But we need this join for a fallback when using pre-aggregations. So we’ll try to obtain the join but ignore any errors (which may occur if the query is a multi-fact one).
-      try {
-        this.join = this.joinGraph.buildJoin(this.allJoinHints);
-      } catch (e) {
-        // Ignore
-      }
-    } else {
+    try {
+      // TODO allJoinHints should contain join hints form pre-agg
       this.join = this.joinGraph.buildJoin(this.allJoinHints);
+    } catch (e) {
+      if (this.useNativeSqlPlanner) {
+        // Tesseract doesn't require join to be prebuilt and there's a case where single join can't be built for multi-fact query
+        // But we need this join for a fallback when using pre-aggregations. So we’ll try to obtain the join but ignore any errors (which may occur if the query is a multi-fact one).
+      } else {
+        throw e;
+      }
     }
   }
 
@@ -363,6 +363,10 @@ export class BaseQuery {
     return this.collectedCubeNames;
   }
 
+  /**
+   *
+   * @returns {Array<Array<string>>}
+   */
   get allJoinHints() {
     if (!this.collectedJoinHints) {
       this.collectedJoinHints = this.collectJoinHints();
@@ -1201,7 +1205,16 @@ export class BaseQuery {
 
   collectAllMultiStageMembers(allMemberChildren) {
     const allMembers = R.uniq(R.flatten(Object.keys(allMemberChildren).map(k => [k].concat(allMemberChildren[k]))));
-    return R.fromPairs(allMembers.map(m => ([m, this.memberInstanceByPath(m).isMultiStage()])));
+    return R.fromPairs(allMembers.map(m => {
+      // When `m` is coming from `collectAllMemberChildren`, it can contain `granularities.customGranularityName` in path
+      // And it would mess up with join hints detection
+      const trimmedPath = this
+        .cubeEvaluator
+        .parsePathAnyType(m)
+        .slice(0, 2)
+        .join('.');
+      return [m, this.memberInstanceByPath(trimmedPath).isMultiStage()];
+    }));
   }
 
   memberInstanceByPath(m) {
@@ -1990,7 +2003,7 @@ export class BaseQuery {
     );
 
     if (shouldBuildJoinForMeasureSelect) {
-      const joinHints = this.collectFrom(measures, this.collectJoinHintsFor.bind(this), 'collectJoinHintsFor');
+      const joinHints = this.collectJoinHintsFromMembers(measures);
       const measuresJoin = this.joinGraph.buildJoin(joinHints);
       if (measuresJoin.multiplicationFactor[keyCubeName]) {
         throw new UserError(
@@ -2044,6 +2057,11 @@ export class BaseQuery {
       (!this.safeEvaluateSymbolContext().ungrouped && this.aggregateSubQueryGroupByClause() || '');
   }
 
+  /**
+   * @param {Array<BaseMeasure>} measures
+   * @param {string} keyCubeName
+   * @returns {boolean}
+   */
   checkShouldBuildJoinForMeasureSelect(measures, keyCubeName) {
     // When member expression references view, it would have to collect join hints from view
     // Consider join A->B, as many-to-one, so B is multiplied and A is not, and member expression like SUM(AB_view.dimB)
@@ -2065,7 +2083,11 @@ export class BaseQuery {
         .filter(member => member.definition().ownedByCube);
 
       const cubes = this.collectFrom(nonViewMembers, this.collectCubeNamesFor.bind(this), 'collectCubeNamesFor');
-      const joinHints = this.collectFrom(nonViewMembers, this.collectJoinHintsFor.bind(this), 'collectJoinHintsFor');
+      // Not using `collectJoinHintsFromMembers([measure])` because it would collect too many join hints from view
+      const joinHints = [
+        measure.joinHint,
+        ...this.collectJoinHintsFromMembers(nonViewMembers),
+      ];
       if (R.any(cubeName => keyCubeName !== cubeName, cubes)) {
         const measuresJoin = this.joinGraph.buildJoin(joinHints);
         if (measuresJoin.multiplicationFactor[keyCubeName]) {
@@ -2179,12 +2201,29 @@ export class BaseQuery {
     );
   }
 
+  /**
+   *
+   * @param {boolean} [excludeTimeDimensions=false]
+   * @returns {Array<Array<string>>}
+   */
   collectJoinHints(excludeTimeDimensions = false) {
-    return this.collectFromMembers(
-      excludeTimeDimensions,
-      this.collectJoinHintsFor.bind(this),
-      'collectJoinHintsFor'
-    );
+    const membersToCollectFrom = this.allMembersConcat(excludeTimeDimensions)
+      .concat(this.join ? this.join.joins.map(j => ({
+        getMembers: () => [{
+          path: () => null,
+          cube: () => this.cubeEvaluator.cubeFromPath(j.originalFrom),
+          definition: () => j.join,
+        }]
+      })) : []);
+
+    return this.collectJoinHintsFromMembers(membersToCollectFrom);
+  }
+
+  collectJoinHintsFromMembers(members) {
+    return [
+      ...members.map(m => m.joinHint).filter(h => h?.length > 0),
+      ...this.collectFrom(members, this.collectJoinHintsFor.bind(this), 'collectJoinHintsFromMembers'),
+    ];
   }
 
   collectFromMembers(excludeTimeDimensions, fn, methodName) {
@@ -2199,6 +2238,11 @@ export class BaseQuery {
     return this.collectFrom(membersToCollectFrom, fn, methodName);
   }
 
+  /**
+   *
+   * @param {boolean} excludeTimeDimensions
+   * @returns {Array<BaseMeasure | BaseDimension | BaseSegment>}
+   */
   allMembersConcat(excludeTimeDimensions) {
     return this.measures
       .concat(this.dimensions)
diff --git a/packages/cubejs-schema-compiler/src/adapter/BaseSegment.ts b/packages/cubejs-schema-compiler/src/adapter/BaseSegment.ts
@@ -9,6 +9,8 @@ export class BaseSegment {
 
   public readonly isMemberExpression: boolean = false;
 
+  public readonly joinHint: Array<string> = [];
+
   public constructor(
     protected readonly query: BaseQuery,
     public readonly segment: string | any
@@ -19,6 +21,16 @@ export class BaseSegment {
       // In case of SQL push down expressionName doesn't contain cube name. It's just a column name.
       this.expressionName = segment.expressionName || `${segment.cubeName}.${segment.name}`;
       this.isMemberExpression = !!segment.definition;
+    } else {
+      // TODO move this `as` to static types
+      const segmentPath = segment as string;
+      const parts = segmentPath.split('.');
+      if (parts.length > 2) {
+        // Measure path contains join path
+        const hint = parts.slice(0, -1);
+        this.segment = parts.slice(-2).join('.');
+        this.joinHint = hint;
+      }
     }
   }
 
diff --git a/packages/cubejs-schema-compiler/src/adapter/PreAggregations.js b/packages/cubejs-schema-compiler/src/adapter/PreAggregations.js
@@ -169,13 +169,22 @@ export class PreAggregations {
         const timeDimensionsReference =
           foundPreAggregation.preAggregation.rollupLambdaTimeDimensionsReference ||
           foundPreAggregation.references.timeDimensions;
+        const timeDimensionReference = timeDimensionsReference[0];
 
-        if (td.dimension === timeDimensionsReference[0].dimension) {
+        // timeDimensionsReference[*].dimension can contain full join path, so we should trim it
+        // TODO check full join path match here
+        const timeDimensionReferenceDimension = timeDimensionReference
+          .dimension
+          .split(',')
+          .slice(-2)
+          .join('.');
+
+        if (td.dimension === timeDimensionReferenceDimension) {
           return true;
         }
 
         // Handling for views
-        return td.dimension === allBackAliasMembers[timeDimensionsReference[0].dimension];
+        return td.dimension === allBackAliasMembers[timeDimensionReferenceDimension];
       });
 
     const filters = preAggregation.partitionGranularity && this.query.filters.filter(td => {
@@ -485,6 +494,10 @@ export class PreAggregations {
    * @returns {function(preagg: Object): boolean}
    */
   static canUsePreAggregationForTransformedQueryFn(transformedQuery, refs) {
+    // TODO this needs to check not only members list, but their join paths as well:
+    //  query can have same members as pre-agg, but different calculated join path
+    //  `refs` will come from preagg references, and would contain full join paths
+
     /**
      * Returns an array of 2-elements arrays with the dimension and granularity
      * sorted by the concatenated dimension + granularity key.
@@ -1061,22 +1074,38 @@ export class PreAggregations {
   }
 
   rollupPreAggregationQuery(cube, aggregation) {
+    // `this.evaluateAllReferences` will retain not only members, but their join path as well, and pass join hints
+    // to subquery. Otherwise, members in subquery would regenerate new join tree from clean state,
+    // and it can be different from expected by join path in pre-aggregation declaration
     const references = this.evaluateAllReferences(cube, aggregation);
     const cubeQuery = this.query.newSubQueryForCube(cube, {});
-    return this.query.newSubQueryForCube(
-      cube,
-      {
-        rowLimit: null,
-        offset: null,
-        measures: references.measures,
-        dimensions: references.dimensions,
-        timeDimensions: this.mergePartitionTimeDimensions(references, aggregation.partitionTimeDimensions),
-        preAggregationQuery: true,
-        useOriginalSqlPreAggregationsInPreAggregation: aggregation.useOriginalSqlPreAggregations,
-        ungrouped: cubeQuery.preAggregationAllowUngroupingWithPrimaryKey(cube, aggregation) &&
-          !!references.dimensions.find(d => this.query.cubeEvaluator.dimensionByPath(d).primaryKey)
-      }
-    );
+    return this.query.newSubQueryForCube(cube, {
+      rowLimit: null,
+      offset: null,
+      measures: references.measures,
+      dimensions: references.dimensions,
+      timeDimensions: this.mergePartitionTimeDimensions(
+        references,
+        aggregation.partitionTimeDimensions
+      ),
+      preAggregationQuery: true,
+      useOriginalSqlPreAggregationsInPreAggregation:
+        aggregation.useOriginalSqlPreAggregations,
+      ungrouped:
+        cubeQuery.preAggregationAllowUngroupingWithPrimaryKey(
+          cube,
+          aggregation
+        ) &&
+        !!references.dimensions.find((d) => {
+          // `d` can contain full join path, so we should trim it
+          // TODO check full join path match here
+          const trimmedDimension = d
+            .split(',')
+            .slice(-2)
+            .join('.');
+          return this.query.cubeEvaluator.dimensionByPath(trimmedDimension).primaryKey;
+        }),
+    });
   }
 
   autoRollupPreAggregationQuery(cube, aggregation) {
@@ -1101,6 +1130,7 @@ export class PreAggregations {
     }
     return aggregation.timeDimensions.map(d => {
       const toMerge = partitionTimeDimensions.find(
+        // Both qd and d comes from PreaggregationReferences
         qd => qd.dimension === d.dimension
       );
       return toMerge ? { ...d, dateRange: toMerge.dateRange, boundaryDateRange: toMerge.boundaryDateRange } : d;
@@ -1119,6 +1149,13 @@ export class PreAggregations {
       .toLowerCase();
   }
 
+  /**
+   *
+   * @param {string} cube
+   * @param aggregation
+   * @param {string} [preAggregationName]
+   * @returns {PreAggregationReferences}
+   */
   evaluateAllReferences(cube, aggregation, preAggregationName) {
     const evaluateReferences = () => {
       const references = this.query.cubeEvaluator.evaluatePreAggregationReferences(cube, aggregation);
diff --git a/packages/cubejs-schema-compiler/src/compiler/CubeEvaluator.ts b/packages/cubejs-schema-compiler/src/compiler/CubeEvaluator.ts
@@ -81,6 +81,7 @@ type PreAggregationTimeDimensionReference = {
   granularity: string,
 };
 
+/// Strings in `dimensions`, `measures` and `timeDimensions[*].dimension` can contain full join path, not just `cube.member`
 type PreAggregationReferences = {
   allowNonStrictDateRangeMatch?: boolean,
   dimensions: Array<string>,
@@ -742,14 +743,14 @@ export class CubeEvaluator extends CubeSymbols {
 
     if (aggregation.timeDimensionReference) {
       timeDimensions.push({
-        dimension: this.evaluateReferences(cube, aggregation.timeDimensionReference),
+        dimension: this.evaluateReferences(cube, aggregation.timeDimensionReference, { collectJoinHints: true }),
         granularity: aggregation.granularity
       });
     } else if (aggregation.timeDimensionReferences) {
       // eslint-disable-next-line guard-for-in
       for (const timeDimensionReference of aggregation.timeDimensionReferences) {
         timeDimensions.push({
-          dimension: this.evaluateReferences(cube, timeDimensionReference.dimension),
+          dimension: this.evaluateReferences(cube, timeDimensionReference.dimension, { collectJoinHints: true }),
           granularity: timeDimensionReference.granularity
         });
       }
@@ -758,12 +759,12 @@ export class CubeEvaluator extends CubeSymbols {
     return {
       allowNonStrictDateRangeMatch: aggregation.allowNonStrictDateRangeMatch,
       dimensions:
-        (aggregation.dimensionReferences && this.evaluateReferences(cube, aggregation.dimensionReferences) || [])
+        (aggregation.dimensionReferences && this.evaluateReferences(cube, aggregation.dimensionReferences, { collectJoinHints: true }) || [])
           .concat(
-            aggregation.segmentReferences && this.evaluateReferences(cube, aggregation.segmentReferences) || []
+            aggregation.segmentReferences && this.evaluateReferences(cube, aggregation.segmentReferences, { collectJoinHints: true }) || []
           ),
       measures:
-        aggregation.measureReferences && this.evaluateReferences(cube, aggregation.measureReferences) || [],
+        (aggregation.measureReferences && this.evaluateReferences(cube, aggregation.measureReferences, { collectJoinHints: true }) || []),
       timeDimensions,
       rollups:
         aggregation.rollupReferences && this.evaluateReferences(cube, aggregation.rollupReferences, { originalSorting: true }) || [],
diff --git a/packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-join-path.test.ts b/packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-join-path.test.ts
diff --git a/packages/cubejs-server-core/src/core/CompilerApi.js b/packages/cubejs-server-core/src/core/CompilerApi.js
