From 0889d02d3ac087d23659bb99d45a1c5153a8c81e Mon Sep 17 00:00:00 2001 From: Konstantin Burkalev Date: Fri, 25 Oct 2024 20:14:23 +0300 Subject: [PATCH 1/3] fix(schema-compiler): fix Maximum call stack size exceeded if FILTER_PARAMS are used inside dimensions/measures --- .../src/adapter/BaseQuery.js | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js b/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js index bf1f5254efe8a..eac76aee8fe9f 100644 --- a/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js +++ b/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js @@ -2240,6 +2240,14 @@ export class BaseQuery { this.safeEvaluateSymbolContext().memberChildren[parentMember].push(memberPath); } } + + // This is a special recursion guard that might happen sometimes, like + // during alias members collection which invokes sql evaluation of all members + // when FILTER_PARAMS is proxied for SQL evaluation. + if (parentMember === memberPath && this.safeEvaluateSymbolContext().aliasGathering) { + return ''; + } + this.safeEvaluateSymbolContext().currentMember = memberPath; try { if (type === 'measure') { @@ -3882,6 +3890,12 @@ export class BaseQuery { // collectFrom() -> traverseSymbol() -> evaluateSymbolSql() -> // evaluateSql() -> resolveSymbolsCall() -> cubeReferenceProxy->toString() -> // evaluateSymbolSql() -> evaluateSql()... -> and got here again + // + // When FILTER_PARAMS is used in dimension/measure SQL - we also hit recursive loop: + // allBackAliasMembersExceptSegments() -> collectFrom() -> traverseSymbol() -> evaluateSymbolSql() -> + // autoPrefixAndEvaluateSql() -> evaluateSql() -> filterProxyFromAllFilters->Proxy->toString() + // and so on... + // For this case there is a recursion guard added to this.evaluateSymbolSql() const aliases = allFilters ? allFilters .map(v => (v.query ? v.query.allBackAliasMembersExceptSegments() : {})) @@ -3932,8 +3946,10 @@ export class BaseQuery { const query = this; return members.map( member => { - const collectedMembers = query - .collectFrom([member], query.collectMemberNamesFor.bind(query), 'collectMemberNamesFor'); + const collectedMembers = query.evaluateSymbolSqlWithContext( + () => query.collectFrom([member], query.collectMemberNamesFor.bind(query), 'collectMemberNamesFor'), + { aliasGathering: true } + ); const memberPath = member.expressionPath(); let nonAliasSeen = false; return collectedMembers From 1a9b1f45d0e5811e18803254a715f04c97e44f7e Mon Sep 17 00:00:00 2001 From: Konstantin Burkalev Date: Fri, 8 Nov 2024 14:07:15 +0200 Subject: [PATCH 2/3] add tests --- .../test/unit/base-query.test.ts | 87 +++++++++++++++++-- 1 file changed, 78 insertions(+), 9 deletions(-) diff --git a/packages/cubejs-schema-compiler/test/unit/base-query.test.ts b/packages/cubejs-schema-compiler/test/unit/base-query.test.ts index bd57b53da0c4e..15c715a700a69 100644 --- a/packages/cubejs-schema-compiler/test/unit/base-query.test.ts +++ b/packages/cubejs-schema-compiler/test/unit/base-query.test.ts @@ -1586,15 +1586,37 @@ describe('SQL Generation', () => { cubes: [{ name: 'Order', sql: 'select * from order where {FILTER_PARAMS.Order.type.filter(\'type\')}', - measures: [{ - name: 'count', - type: 'count', - }], - dimensions: [{ - name: 'type', - sql: 'type', - type: 'string' - }] + measures: [ + { + name: 'count', + type: 'count', + }, + { + name: 'avg_filtered', + sql: 'product_id', + type: 'avg', + filters: [ + { sql: `{FILTER_PARAMS.Order.category.filter('category')}` } + ] + } + ], + dimensions: [ + { + name: 'type', + sql: 'type', + type: 'string' + }, + { + name: 'category', + sql: 'category', + type: 'string' + }, + { + name: 'proxied', + sql: `{FILTER_PARAMS.Order.type.filter("x => type = 'online'")}`, + type: 'boolean', + } + ] }], views: [{ name: 'orders_view', @@ -1829,6 +1851,53 @@ describe('SQL Generation', () => { const queryString = queryAndParams[0]; expect(/select\s+\*\s+from\s+order\s+where\s+\(\(type\s=\s\?\)\)/.test(queryString)).toBeTruthy(); }); + + it('correctly substitute filter params in cube\'s query dimension used in filter', async () => { + await compilers.compiler.compile(); + const query = new BaseQuery(compilers, { + measures: ['Order.count'], + dimensions: ['Order.proxied'], + filters: [ + { + member: 'Order.proxied', + operator: 'equals', + values: [true], + }, + ], + }); + const queryAndParams = query.buildSqlAndParams(); + const queryString = queryAndParams[0]; + expect(queryString).toContain(`SELECT + (1 = 1) "order__proxied", count(*) "order__count" + FROM + (select * from order where (1 = 1)) AS "order" WHERE ((1 = 1) = ?)`); + }); + + it('correctly substitute filter params in cube\'s query measure used in filter', async () => { + await compilers.compiler.compile(); + const query = new BaseQuery(compilers, { + measures: ['Order.avg_filtered'], + dimensions: ['Order.type'], + filters: [ + { + member: 'Order.type', + operator: 'equals', + values: ['online'], + }, + { + member: 'Order.category', + operator: 'equals', + values: ['category'], + }, + ], + }); + const queryAndParams = query.buildSqlAndParams(); + const queryString = queryAndParams[0]; + expect(queryString).toContain(`SELECT + "order".type "order__type", avg(CASE WHEN ((category = ?)) THEN "order".product_id END) "order__avg_filtered" + FROM + (select * from order where (type = ?)) AS "order" WHERE ("order".type = ?) AND ("order".category = ?)`); + }); }); describe('FILTER_GROUP', () => { From 042ba7d3c0218b8ff2badc3b009954d15c47320b Mon Sep 17 00:00:00 2001 From: Konstantin Burkalev Date: Fri, 8 Nov 2024 20:51:23 +0200 Subject: [PATCH 3/3] move guard to the filter_params proxy --- .../cubejs-schema-compiler/src/adapter/BaseQuery.js | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js b/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js index eac76aee8fe9f..d22716b4e10b4 100644 --- a/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js +++ b/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js @@ -2241,13 +2241,6 @@ export class BaseQuery { } } - // This is a special recursion guard that might happen sometimes, like - // during alias members collection which invokes sql evaluation of all members - // when FILTER_PARAMS is proxied for SQL evaluation. - if (parentMember === memberPath && this.safeEvaluateSymbolContext().aliasGathering) { - return ''; - } - this.safeEvaluateSymbolContext().currentMember = memberPath; try { if (type === 'measure') { @@ -3895,10 +3888,11 @@ export class BaseQuery { // allBackAliasMembersExceptSegments() -> collectFrom() -> traverseSymbol() -> evaluateSymbolSql() -> // autoPrefixAndEvaluateSql() -> evaluateSql() -> filterProxyFromAllFilters->Proxy->toString() // and so on... - // For this case there is a recursion guard added to this.evaluateSymbolSql() + // For this case aliasGathering flag is added to the context in first iteration and + // is checked below to prevent looping. const aliases = allFilters ? allFilters - .map(v => (v.query ? v.query.allBackAliasMembersExceptSegments() : {})) + .map(v => (v.query && !v.query.safeEvaluateSymbolContext().aliasGathering ? v.query.allBackAliasMembersExceptSegments() : {})) .reduce((a, b) => ({ ...a, ...b }), {}) : {}; // Filtering aliases that somehow relate to this group member