Skip to content

fix(schema-compiler): Fix incorrect join path resolving for views #9995

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+388 −111
Open

fix(schema-compiler): Fix incorrect join path resolving for views #9995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
23f8c9d
add tests
KSDaemon Sep 24, 2025
bd65122
enable view join tests for tesseract
KSDaemon Sep 24, 2025
2deabfc
fix test
KSDaemon Sep 24, 2025
106bc7a
temp comment out tests
KSDaemon Sep 26, 2025
420ac10
correct additional hints
KSDaemon Sep 25, 2025
edb7c3c
trying to adopt loop test
KSDaemon Oct 6, 2025
66072c9
linter fix
KSDaemon Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 20 additions & 70 deletions packages/cubejs-schema-compiler/src/adapter/BaseQuery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {
localTimestampToUtc,
timeSeries as timeSeriesBase,
timeSeriesFromCustomInterval,
parseSqlInterval,
findMinGranularityDimension
} from '@cubejs-backend/shared';

Expand Down Expand Up @@ -436,81 +435,32 @@ export class BaseQuery {
*/
get allJoinHints() {
if (!this.collectedJoinHints) {
const [rootOfJoin, ...allMembersJoinHints] = this.collectJoinHintsFromMembers(this.allMembersConcat(false));
const allMembersJoinHints = this.collectJoinHintsFromMembers(this.allMembersConcat(false));
const customSubQueryJoinHints = this.collectJoinHintsFromMembers(this.joinMembersFromCustomSubQuery());
let joinMembersJoinHints = this.collectJoinHintsFromMembers(this.joinMembersFromJoin(this.join));

// One cube may join the other cube via transitive joined cubes,
// members from which are referenced in the join `on` clauses.
// We need to collect such join hints and push them upfront of the joining one
// but only if they don't exist yet. Cause in other case we might affect what
// join path will be constructed in join graph.
// It is important to use queryLevelJoinHints during the calculation if it is set.

const constructJH = () => {
const filteredJoinMembersJoinHints = joinMembersJoinHints.filter(m => !allMembersJoinHints.includes(m));
return [
...this.queryLevelJoinHints,
...(rootOfJoin ? [rootOfJoin] : []),
...filteredJoinMembersJoinHints,
...allMembersJoinHints,
...customSubQueryJoinHints,
];
};

let prevJoins = this.join;
let prevJoinMembersJoinHints = joinMembersJoinHints;
let newJoin = this.joinGraph.buildJoin(constructJH());

const isOrderPreserved = (base, updated) => {
const common = base.filter(value => updated.includes(value));
const bFiltered = updated.filter(value => common.includes(value));

return common.every((x, i) => x === bFiltered[i]);
};

const isJoinTreesEqual = (a, b) => {
if (!a || !b || a.root !== b.root || a.joins.length !== b.joins.length) {
return false;
}

// We don't care about the order of joins on the same level, so
// we can compare them as sets.
const aJoinsSet = new Set(a.joins.map(j => `${j.originalFrom}->${j.originalTo}`));
const bJoinsSet = new Set(b.joins.map(j => `${j.originalFrom}->${j.originalTo}`));

if (aJoinsSet.size !== bJoinsSet.size) {
return false;
}

for (const val of aJoinsSet) {
if (!bJoinsSet.has(val)) {
return false;
}
}

return true;
};
const allJoinHints = [
...this.queryLevelJoinHints,
...allMembersJoinHints,
...customSubQueryJoinHints,
];

// Safeguard against infinite loop in case of cyclic joins somehow managed to slip through
let cnt = 0;
const tempJoin = this.joinGraph.buildJoin(allJoinHints);

while (newJoin?.joins.length > 0 && !isJoinTreesEqual(prevJoins, newJoin) && cnt < 10000) {
prevJoins = newJoin;
joinMembersJoinHints = this.collectJoinHintsFromMembers(this.joinMembersFromJoin(newJoin));
if (!isOrderPreserved(prevJoinMembersJoinHints, joinMembersJoinHints)) {
throw new UserError(`Can not construct joins for the query, potential loop detected: ${prevJoinMembersJoinHints.join('->')} vs ${joinMembersJoinHints.join('->')}`);
}
newJoin = this.joinGraph.buildJoin(constructJH());
prevJoinMembersJoinHints = joinMembersJoinHints;
cnt++;
if (!tempJoin) {
this.collectedJoinHints = allJoinHints;
return allJoinHints;
}

if (cnt >= 10000) {
throw new UserError('Can not construct joins for the query, potential loop detected');
}
const joinMembersJoinHints = this.collectJoinHintsFromMembers(this.joinMembersFromJoin(tempJoin));
const allJoinHintsFlatten = new Set(allJoinHints.flat());
const newCollectedHints = joinMembersJoinHints.filter(j => !allJoinHintsFlatten.has(j));

this.collectedJoinHints = R.uniq(constructJH());
this.collectedJoinHints = [
...this.queryLevelJoinHints,
tempJoin.root,
...newCollectedHints,
...allMembersJoinHints,
...customSubQueryJoinHints,
];
}
return this.collectedJoinHints;
}
Expand Down
73 changes: 36 additions & 37 deletions packages/cubejs-schema-compiler/test/integration/postgres/sql-generation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5198,7 +5198,7 @@ cubes:
sql: amount
type: sum

# Join loop for testing transitive joins
# Model for testing multiple joins to the same cube via transitive joins
- name: alpha_facts
sql: |
(
Expand Down Expand Up @@ -5256,9 +5256,9 @@ cubes:
- name: gamma_dims
sql: |
(
SELECT 10 AS b_id, 'Beta1' AS b_name
SELECT 10 AS b_id, 'Beta1' AS b_name, 'Gamma1' AS c_name
UNION ALL
SELECT 20 AS b_id, 'Beta2' AS b_name
SELECT 20 AS b_id, 'Beta2' AS b_name, 'Gamma2' AS c_name
)
dimensions:
- name: b_id
Expand All @@ -5272,16 +5272,16 @@ cubes:
- name: delta_bridge
sql: |
(
SELECT 'Alpha1' AS a_name, 'Beta1' AS b_name, 'Organic' AS channel
SELECT 'Alpha1' AS a_name, 'Beta1' AS b_name, 'Gamma1' AS c_name, 'Organic' AS channel
UNION ALL
SELECT 'Alpha1' AS a_name, 'Beta2' AS b_name, 'Paid' AS channel
SELECT 'Alpha1' AS a_name, 'Beta2' AS b_name, 'Gamma2' AS c_name, 'Paid' AS channel
UNION ALL
SELECT 'Alpha2' AS a_name, 'Beta1' AS b_name, 'Referral' AS channel
SELECT 'Alpha2' AS a_name, 'Beta1' AS b_name, 'Gamma3' AS c_name, 'Referral' AS channel
)
joins:
- name: gamma_dims
relationship: many_to_one
sql: "{CUBE}.b_name = {gamma_dims.b_name}"
sql: "{CUBE}.c_name = {gamma_dims.c_name}"
dimensions:
- name: a_name
sql: a_name
Expand All @@ -5290,7 +5290,9 @@ cubes:
- name: b_name
sql: "{gamma_dims.b_name}"
type: string
primary_key: true
- name: c_name
sql: c_name
type: string
- name: channel
sql: channel
type: string
Expand Down Expand Up @@ -5343,34 +5345,31 @@ cubes:
});
}

if (!getEnv('nativeSqlPlanner')) {
it('querying cube with transitive joins with loop', async () => {
await compiler.compile();

try {
const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, {
measures: [],
dimensions: [
'alpha_facts.reporting_date',
'delta_bridge.b_name',
'alpha_facts.channel'
],
order: [{
id: 'alpha_facts.reporting_date'
}],
timezone: 'America/Los_Angeles'
});

await dbRunner.testQuery(query.buildSqlAndParams());
throw new Error('Should have thrown an error');
} catch (err: any) {
expect(err.message).toContain('Can not construct joins for the query, potential loop detected');
}
});
} else {
it.skip('FIXME(tesseract): querying cube dimension that require transitive joins', async () => {
// FIXME should be implemented in Tesseract
});
}
it('querying cube with transitive joins with a few joins to same cube', async () => {
// TODO: This is not supported atm, but it's a good case, so keeeping this test
// for the future implementation

// await compiler.compile();
//
// const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, {
// measures: [],
// dimensions: [
// 'alpha_facts.reporting_date',
// 'delta_bridge.b_name',
// 'alpha_facts.channel'
// ],
// order: [{
// id: 'alpha_facts.reporting_date'
// }],
// timezone: 'America/Los_Angeles'
// });
//
// const res = await dbRunner.testQuery(query.buildSqlAndParams());
// console.log(JSON.stringify(res));
//
// expect(res).toEqual([
// // Fill
// ]);
});
});
});
4 changes: 2 additions & 2 deletions packages/cubejs-schema-compiler/test/integration/postgres/views-join-order-2.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,14 @@ cube('A', {

cube('B', {
sql: \`
SELECT 2 id, 'b'::text as "name"\`,
SELECT 2 id, 4 as fk_e, 'b'::text as "name"\`,
joins: {
A: {
relationship: \`many_to_one\`,
sql: \`\${CUBE.fk} = \${A.id}\`,
},
E: {
sql: \`\${CUBE.name} = \${E.id}\`,
sql: \`\${CUBE}.fk_e = \${E.id}\`,
relationship: \`many_to_one\`,
},
},
Expand Down
Loading
Loading