Skip to content

Commit 3df5613

Browse files
mcheshkovmcheshkov
authored
Revert "fix(schema-compiler): Use join paths from pre-aggregation declaration instead of building join tree from scratch (#9431)" (#9448)
This reverts commit aea3a6c, PR #9431 It contains a bug in pre-aggregation matching, rendering any pre-agg with join path unusable
1 parent 575abca commit 3df5613

File tree

9 files changed

+38
-418
lines changed

9 files changed

+38
-418
lines changed

‎packages/cubejs-schema-compiler/src/adapter/BaseDimension.ts‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@ export class BaseDimension {
1010

1111
public readonly isMemberExpression: boolean = false;
1212

13-
public readonly joinHint: Array<string> = [];
14-
1513
public constructor(
1614
protected readonly query: BaseQuery,
1715
public readonly dimension: any
@@ -22,14 +20,6 @@ export class BaseDimension {
2220
// In case of SQL push down expressionName doesn't contain cube name. It's just a column name.
2321
this.expressionName = dimension.expressionName || `${dimension.cubeName}.${dimension.name}`;
2422
this.isMemberExpression = !!dimension.definition;
25-
} else {
26-
// TODO move this `as` to static types
27-
const dimensionPath = dimension as string | null;
28-
if (dimensionPath !== null) {
29-
const { path, joinHint } = this.query.cubeEvaluator.joinHintFromPath(dimensionPath);
30-
this.dimension = path;
31-
this.joinHint = joinHint;
32-
}
3323
}
3424
}
3525

‎packages/cubejs-schema-compiler/src/adapter/BaseMeasure.ts‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@ export class BaseMeasure {
1313

1414
protected readonly patchedMeasure: MeasureDefinition | null = null;
1515

16-
public readonly joinHint: Array<string> = [];
17-
1816
protected preparePatchedMeasure(sourceMeasure: string, newMeasureType: string | null, addFilters: Array<{sql: Function}>): MeasureDefinition {
1917
const source = this.query.cubeEvaluator.measureByPath(sourceMeasure);
2018

@@ -125,12 +123,6 @@ export class BaseMeasure {
125123
measure.expression.addFilters,
126124
);
127125
}
128-
} else {
129-
// TODO move this `as` to static types
130-
const measurePath = measure as string;
131-
const { path, joinHint } = this.query.cubeEvaluator.joinHintFromPath(measurePath);
132-
this.measure = path;
133-
this.joinHint = joinHint;
134126
}
135127
}
136128

‎packages/cubejs-schema-compiler/src/adapter/BaseQuery.js‎

Lines changed: 17 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -307,16 +307,16 @@ export class BaseQuery {
307307
}
308308

309309
prebuildJoin() {
310-
try {
311-
// TODO allJoinHints should contain join hints form pre-agg
312-
this.join = this.joinGraph.buildJoin(this.allJoinHints);
313-
} catch (e) {
314-
if (this.useNativeSqlPlanner) {
315-
// Tesseract doesn't require join to be prebuilt and there's a case where single join can't be built for multi-fact query
316-
// 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).
317-
} else {
318-
throw e;
310+
if (this.useNativeSqlPlanner) {
311+
// Tesseract doesn't require join to be prebuilt and there's a case where single join can't be built for multi-fact query
312+
// 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).
313+
try {
314+
this.join = this.joinGraph.buildJoin(this.allJoinHints);
315+
} catch (e) {
316+
// Ignore
319317
}
318+
} else {
319+
this.join = this.joinGraph.buildJoin(this.allJoinHints);
320320
}
321321
}
322322

@@ -363,10 +363,6 @@ export class BaseQuery {
363363
return this.collectedCubeNames;
364364
}
365365

366-
/**
367-
*
368-
* @returns {Array<Array<string>>}
369-
*/
370366
get allJoinHints() {
371367
if (!this.collectedJoinHints) {
372368
this.collectedJoinHints = this.collectJoinHints();
@@ -1207,16 +1203,7 @@ export class BaseQuery {
12071203

12081204
collectAllMultiStageMembers(allMemberChildren) {
12091205
const allMembers = R.uniq(R.flatten(Object.keys(allMemberChildren).map(k => [k].concat(allMemberChildren[k]))));
1210-
return R.fromPairs(allMembers.map(m => {
1211-
// When `m` is coming from `collectAllMemberChildren`, it can contain `granularities.customGranularityName` in path
1212-
// And it would mess up with join hints detection
1213-
const trimmedPath = this
1214-
.cubeEvaluator
1215-
.parsePathAnyType(m)
1216-
.slice(0, 2)
1217-
.join('.');
1218-
return [m, this.memberInstanceByPath(trimmedPath).isMultiStage()];
1219-
}));
1206+
return R.fromPairs(allMembers.map(m => ([m, this.memberInstanceByPath(m).isMultiStage()])));
12201207
}
12211208

12221209
memberInstanceByPath(m) {
@@ -2005,7 +1992,7 @@ export class BaseQuery {
20051992
);
20061993

20071994
if (shouldBuildJoinForMeasureSelect) {
2008-
const joinHints = this.collectJoinHintsFromMembers(measures);
1995+
const joinHints = this.collectFrom(measures, this.collectJoinHintsFor.bind(this), 'collectJoinHintsFor');
20091996
const measuresJoin = this.joinGraph.buildJoin(joinHints);
20101997
if (measuresJoin.multiplicationFactor[keyCubeName]) {
20111998
throw new UserError(
@@ -2059,11 +2046,6 @@ export class BaseQuery {
20592046
(!this.safeEvaluateSymbolContext().ungrouped && this.aggregateSubQueryGroupByClause() || '');
20602047
}
20612048

2062-
/**
2063-
* @param {Array<BaseMeasure>} measures
2064-
* @param {string} keyCubeName
2065-
* @returns {boolean}
2066-
*/
20672049
checkShouldBuildJoinForMeasureSelect(measures, keyCubeName) {
20682050
// When member expression references view, it would have to collect join hints from view
20692051
// Consider join A->B, as many-to-one, so B is multiplied and A is not, and member expression like SUM(AB_view.dimB)
@@ -2085,11 +2067,7 @@ export class BaseQuery {
20852067
.filter(member => member.definition().ownedByCube);
20862068

20872069
const cubes = this.collectFrom(nonViewMembers, this.collectCubeNamesFor.bind(this), 'collectCubeNamesFor');
2088-
// Not using `collectJoinHintsFromMembers([measure])` because it would collect too many join hints from view
2089-
const joinHints = [
2090-
measure.joinHint,
2091-
...this.collectJoinHintsFromMembers(nonViewMembers),
2092-
];
2070+
const joinHints = this.collectFrom(nonViewMembers, this.collectJoinHintsFor.bind(this), 'collectJoinHintsFor');
20932071
if (R.any(cubeName => keyCubeName !== cubeName, cubes)) {
20942072
const measuresJoin = this.joinGraph.buildJoin(joinHints);
20952073
if (measuresJoin.multiplicationFactor[keyCubeName]) {
@@ -2208,29 +2186,12 @@ export class BaseQuery {
22082186
);
22092187
}
22102188

2211-
/**
2212-
*
2213-
* @param {boolean} [excludeTimeDimensions=false]
2214-
* @returns {Array<Array<string>>}
2215-
*/
22162189
collectJoinHints(excludeTimeDimensions = false) {
2217-
const membersToCollectFrom = this.allMembersConcat(excludeTimeDimensions)
2218-
.concat(this.join ? this.join.joins.map(j => ({
2219-
getMembers: () => [{
2220-
path: () => null,
2221-
cube: () => this.cubeEvaluator.cubeFromPath(j.originalFrom),
2222-
definition: () => j.join,
2223-
}]
2224-
})) : []);
2225-
2226-
return this.collectJoinHintsFromMembers(membersToCollectFrom);
2227-
}
2228-
2229-
collectJoinHintsFromMembers(members) {
2230-
return [
2231-
...members.map(m => m.joinHint).filter(h => h?.length > 0),
2232-
...this.collectFrom(members, this.collectJoinHintsFor.bind(this), 'collectJoinHintsFromMembers'),
2233-
];
2190+
return this.collectFromMembers(
2191+
excludeTimeDimensions,
2192+
this.collectJoinHintsFor.bind(this),
2193+
'collectJoinHintsFor'
2194+
);
22342195
}
22352196

22362197
collectFromMembers(excludeTimeDimensions, fn, methodName) {
@@ -2245,11 +2206,6 @@ export class BaseQuery {
22452206
return this.collectFrom(membersToCollectFrom, fn, methodName);
22462207
}
22472208

2248-
/**
2249-
*
2250-
* @param {boolean} excludeTimeDimensions
2251-
* @returns {Array<BaseMeasure | BaseDimension | BaseSegment>}
2252-
*/
22532209
allMembersConcat(excludeTimeDimensions) {
22542210
return this.measures
22552211
.concat(this.dimensions)

‎packages/cubejs-schema-compiler/src/adapter/BaseSegment.ts‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@ export class BaseSegment {
99

1010
public readonly isMemberExpression: boolean = false;
1111

12-
public readonly joinHint: Array<string> = [];
13-
1412
public constructor(
1513
protected readonly query: BaseQuery,
1614
public readonly segment: string | any
@@ -21,12 +19,6 @@ export class BaseSegment {
2119
// In case of SQL push down expressionName doesn't contain cube name. It's just a column name.
2220
this.expressionName = segment.expressionName || `${segment.cubeName}.${segment.name}`;
2321
this.isMemberExpression = !!segment.definition;
24-
} else {
25-
// TODO move this `as` to static types
26-
const segmentPath = segment as string;
27-
const { path, joinHint } = this.query.cubeEvaluator.joinHintFromPath(segmentPath);
28-
this.segment = path;
29-
this.joinHint = joinHint;
3022
}
3123
}
3224

‎packages/cubejs-schema-compiler/src/adapter/PreAggregations.js‎

Lines changed: 16 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -169,18 +169,13 @@ export class PreAggregations {
169169
const timeDimensionsReference =
170170
foundPreAggregation.preAggregation.rollupLambdaTimeDimensionsReference ||
171171
foundPreAggregation.references.timeDimensions;
172-
const timeDimensionReference = timeDimensionsReference[0];
173172

174-
// timeDimensionsReference[*].dimension can contain full join path, so we should trim it
175-
// TODO check full join path match here
176-
const timeDimensionReferenceDimension = this.query.cubeEvaluator.joinHintFromPath(timeDimensionReference.dimension).path;
177-
178-
if (td.dimension === timeDimensionReferenceDimension) {
173+
if (td.dimension === timeDimensionsReference[0].dimension) {
179174
return true;
180175
}
181176

182177
// Handling for views
183-
return td.dimension === allBackAliasMembers[timeDimensionReferenceDimension];
178+
return td.dimension === allBackAliasMembers[timeDimensionsReference[0].dimension];
184179
});
185180

186181
const filters = preAggregation.partitionGranularity && this.query.filters.filter(td => {
@@ -490,10 +485,6 @@ export class PreAggregations {
490485
* @returns {function(preagg: Object): boolean}
491486
*/
492487
static canUsePreAggregationForTransformedQueryFn(transformedQuery, refs) {
493-
// TODO this needs to check not only members list, but their join paths as well:
494-
// query can have same members as pre-agg, but different calculated join path
495-
// `refs` will come from preagg references, and would contain full join paths
496-
497488
/**
498489
* Returns an array of 2-elements arrays with the dimension and granularity
499490
* sorted by the concatenated dimension + granularity key.
@@ -1070,35 +1061,22 @@ export class PreAggregations {
10701061
}
10711062

10721063
rollupPreAggregationQuery(cube, aggregation) {
1073-
// `this.evaluateAllReferences` will retain not only members, but their join path as well, and pass join hints
1074-
// to subquery. Otherwise, members in subquery would regenerate new join tree from clean state,
1075-
// and it can be different from expected by join path in pre-aggregation declaration
10761064
const references = this.evaluateAllReferences(cube, aggregation);
10771065
const cubeQuery = this.query.newSubQueryForCube(cube, {});
1078-
return this.query.newSubQueryForCube(cube, {
1079-
rowLimit: null,
1080-
offset: null,
1081-
measures: references.measures,
1082-
dimensions: references.dimensions,
1083-
timeDimensions: this.mergePartitionTimeDimensions(
1084-
references,
1085-
aggregation.partitionTimeDimensions
1086-
),
1087-
preAggregationQuery: true,
1088-
useOriginalSqlPreAggregationsInPreAggregation:
1089-
aggregation.useOriginalSqlPreAggregations,
1090-
ungrouped:
1091-
cubeQuery.preAggregationAllowUngroupingWithPrimaryKey(
1092-
cube,
1093-
aggregation
1094-
) &&
1095-
!!references.dimensions.find((d) => {
1096-
// `d` can contain full join path, so we should trim it
1097-
// TODO check full join path match here
1098-
const trimmedDimension = this.query.cubeEvaluator.joinHintFromPath(d).path;
1099-
return this.query.cubeEvaluator.dimensionByPath(trimmedDimension).primaryKey;
1100-
}),
1101-
});
1066+
return this.query.newSubQueryForCube(
1067+
cube,
1068+
{
1069+
rowLimit: null,
1070+
offset: null,
1071+
measures: references.measures,
1072+
dimensions: references.dimensions,
1073+
timeDimensions: this.mergePartitionTimeDimensions(references, aggregation.partitionTimeDimensions),
1074+
preAggregationQuery: true,
1075+
useOriginalSqlPreAggregationsInPreAggregation: aggregation.useOriginalSqlPreAggregations,
1076+
ungrouped: cubeQuery.preAggregationAllowUngroupingWithPrimaryKey(cube, aggregation) &&
1077+
!!references.dimensions.find(d => this.query.cubeEvaluator.dimensionByPath(d).primaryKey)
1078+
}
1079+
);
11021080
}
11031081

11041082
autoRollupPreAggregationQuery(cube, aggregation) {
@@ -1123,7 +1101,6 @@ export class PreAggregations {
11231101
}
11241102
return aggregation.timeDimensions.map(d => {
11251103
const toMerge = partitionTimeDimensions.find(
1126-
// Both qd and d comes from PreaggregationReferences
11271104
qd => qd.dimension === d.dimension
11281105
);
11291106
return toMerge ? { ...d, dateRange: toMerge.dateRange, boundaryDateRange: toMerge.boundaryDateRange } : d;
@@ -1142,13 +1119,6 @@ export class PreAggregations {
11421119
.toLowerCase();
11431120
}
11441121

1145-
/**
1146-
*
1147-
* @param {string} cube
1148-
* @param aggregation
1149-
* @param {string} [preAggregationName]
1150-
* @returns {PreAggregationReferences}
1151-
*/
11521122
evaluateAllReferences(cube, aggregation, preAggregationName) {
11531123
const evaluateReferences = () => {
11541124
const references = this.query.cubeEvaluator.evaluatePreAggregationReferences(cube, aggregation);

‎packages/cubejs-schema-compiler/src/compiler/CubeEvaluator.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@ type PreAggregationTimeDimensionReference = {
8282
granularity: string,
8383
};
8484

85-
/// Strings in `dimensions`, `measures` and `timeDimensions[*].dimension` can contain full join path, not just `cube.member`
8685
type PreAggregationReferences = {
8786
allowNonStrictDateRangeMatch?: boolean,
8887
dimensions: Array<string>,
@@ -744,14 +743,14 @@ export class CubeEvaluator extends CubeSymbols {
744743

745744
if (aggregation.timeDimensionReference) {
746745
timeDimensions.push({
747-
dimension: this.evaluateReferences(cube, aggregation.timeDimensionReference, { collectJoinHints: true }),
746+
dimension: this.evaluateReferences(cube, aggregation.timeDimensionReference),
748747
granularity: aggregation.granularity
749748
});
750749
} else if (aggregation.timeDimensionReferences) {
751750
// eslint-disable-next-line guard-for-in
752751
for (const timeDimensionReference of aggregation.timeDimensionReferences) {
753752
timeDimensions.push({
754-
dimension: this.evaluateReferences(cube, timeDimensionReference.dimension, { collectJoinHints: true }),
753+
dimension: this.evaluateReferences(cube, timeDimensionReference.dimension),
755754
granularity: timeDimensionReference.granularity
756755
});
757756
}
@@ -760,12 +759,12 @@ export class CubeEvaluator extends CubeSymbols {
760759
return {
761760
allowNonStrictDateRangeMatch: aggregation.allowNonStrictDateRangeMatch,
762761
dimensions:
763-
(aggregation.dimensionReferences && this.evaluateReferences(cube, aggregation.dimensionReferences, { collectJoinHints: true }) || [])
762+
(aggregation.dimensionReferences && this.evaluateReferences(cube, aggregation.dimensionReferences) || [])
764763
.concat(
765-
aggregation.segmentReferences && this.evaluateReferences(cube, aggregation.segmentReferences, { collectJoinHints: true }) || []
764+
aggregation.segmentReferences && this.evaluateReferences(cube, aggregation.segmentReferences) || []
766765
),
767766
measures:
768-
(aggregation.measureReferences && this.evaluateReferences(cube, aggregation.measureReferences, { collectJoinHints: true }) || []),
767+
aggregation.measureReferences && this.evaluateReferences(cube, aggregation.measureReferences) || [],
769768
timeDimensions,
770769
rollups:
771770
aggregation.rollupReferences && this.evaluateReferences(cube, aggregation.rollupReferences, { originalSorting: true }) || [],

‎packages/cubejs-schema-compiler/src/compiler/CubeSymbols.ts‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -609,27 +609,6 @@ export class CubeSymbols {
609609
return array.join('.');
610610
}
611611

612-
/**
613-
* Split join path to member to join hint and member path: `A.B.C.D.E.dim` => `[A, B, C, D, E]` + `E.dim`
614-
* @param path
615-
*/
616-
public joinHintFromPath(path: string): { path: string, joinHint: Array<string> } {
617-
const parts = path.split('.');
618-
if (parts.length > 2) {
619-
// Path contains join path
620-
const joinHint = parts.slice(0, -1);
621-
return {
622-
path: parts.slice(-2).join('.'),
623-
joinHint,
624-
};
625-
} else {
626-
return {
627-
path,
628-
joinHint: [],
629-
};
630-
}
631-
}
632-
633612
protected resolveSymbolsCall<T>(
634613
func: (...args: Array<unknown>) => T | DynamicReference<T>,
635614
nameResolver: (id: string) => unknown,

0 commit comments

Comments
 (0)