From 7385fb6c1ecf2f2d495a0b767a8c11b7b1d243e4 Mon Sep 17 00:00:00 2001 From: Mikhail Cheshkov Date: Mon, 19 May 2025 23:35:33 +0200 Subject: [PATCH] fix(schema-compiler): Avoid mutating context on first occasion of TD with granularity Context change can alter behaviour for other TD references in same context, specifically in same member expression, rendering different expression --- .../src/adapter/BaseQuery.js | 6 ++- .../postgresql/schema/Orders.js | 15 +++++--- .../__snapshots__/smoke-cubesql.test.ts.snap | 38 +++++++++++++++++-- .../cubejs-testing/test/smoke-cubesql.test.ts | 28 ++++++++++++++ 4 files changed, 76 insertions(+), 11 deletions(-) diff --git a/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js b/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js index a5937458d9c9a..58737882810a3 100644 --- a/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js +++ b/packages/cubejs-schema-compiler/src/adapter/BaseQuery.js @@ -2918,8 +2918,10 @@ export class BaseQuery { }); // for time dimension with granularity convertedToTz() is called internally in dimensionSql() flow, // so we need to ignore convertTz later even if context convertTzForRawTimeDimension is set to true - this.safeEvaluateSymbolContext().ignoreConvertTzForTimeDimension = true; - return td.dimensionSql(); + return this.evaluateSymbolSqlWithContext( + () => td.dimensionSql(), + { ignoreConvertTzForTimeDimension: true }, + ); } else { let res = this.autoPrefixAndEvaluateSql(cubeName, symbol.sql, isMemberExpr); const memPath = this.cubeEvaluator.pathFromArray([cubeName, name]); diff --git a/packages/cubejs-testing/birdbox-fixtures/postgresql/schema/Orders.js b/packages/cubejs-testing/birdbox-fixtures/postgresql/schema/Orders.js index f7a51d286e613..290d68cae17f3 100644 --- a/packages/cubejs-testing/birdbox-fixtures/postgresql/schema/Orders.js +++ b/packages/cubejs-testing/birdbox-fixtures/postgresql/schema/Orders.js @@ -1,14 +1,14 @@ cube(`Orders`, { sql: ` - select 1 as id, 100 as amount, 'new' status, '2024-01-01'::timestamptz created_at + select 1 as id, 100 as amount, 'new' status, '2024-01-01'::timestamptz created_at, '2025-01-01'::timestamptz updated_at UNION ALL - select 2 as id, 200 as amount, 'new' status, '2024-01-02'::timestamptz created_at + select 2 as id, 200 as amount, 'new' status, '2024-01-02'::timestamptz created_at, '2025-01-02'::timestamptz updated_at UNION ALL - select 3 as id, 300 as amount, 'processed' status, '2024-01-03'::timestamptz created_at + select 3 as id, 300 as amount, 'processed' status, '2024-01-03'::timestamptz created_at, '2025-01-03'::timestamptz updated_at UNION ALL - select 4 as id, 500 as amount, 'processed' status, '2024-01-04'::timestamptz created_at + select 4 as id, 500 as amount, 'processed' status, '2024-01-04'::timestamptz created_at, '2025-01-04'::timestamptz updated_at UNION ALL - select 5 as id, 600 as amount, 'shipped' status, '2024-01-05'::timestamptz created_at + select 5 as id, 600 as amount, 'shipped' status, '2024-01-05'::timestamptz created_at, '2025-01-05'::timestamptz updated_at `, joins: { OrderItems: { @@ -122,6 +122,11 @@ cube(`Orders`, { createdAt: { sql: `created_at`, type: `time` + }, + + updatedAt: { + sql: `updated_at`, + type: `time` } }, }); diff --git a/packages/cubejs-testing/test/__snapshots__/smoke-cubesql.test.ts.snap b/packages/cubejs-testing/test/__snapshots__/smoke-cubesql.test.ts.snap index 31b4d1d7df097..2705de899b213 100644 --- a/packages/cubejs-testing/test/__snapshots__/smoke-cubesql.test.ts.snap +++ b/packages/cubejs-testing/test/__snapshots__/smoke-cubesql.test.ts.snap @@ -42,9 +42,9 @@ Object { "sql": Object { "query_type": "pushdown", "sql": Array [ - "SELECT \\"t\\".\\"avg_t_total_\\" \\"avg_t_total_\\" + "SELECT \\"t\\".\\"avg_t_total_\\" \\"avg_t_total_\\" FROM ( - SELECT AVG(\\"t\\".\\"total\\") \\"avg_t_total_\\" + SELECT AVG(\\"t\\".\\"total\\") \\"avg_t_total_\\" FROM ( SELECT \\"orders\\".status \\"status\\", sum(\\"orders\\".amount) \\"total\\" @@ -344,7 +344,7 @@ Object { "sql": Object { "query_type": "pushdown", "sql": Array [ - "SELECT \\"Orders\\".\\"sum_orders_total\\" \\"total\\" + "SELECT \\"Orders\\".\\"sum_orders_total\\" \\"total\\" FROM ( SELECT sum(\\"orders\\".amount) \\"sum_orders_total\\" @@ -399,7 +399,7 @@ Object { "sql": Object { "query_type": "pushdown", "sql": Array [ - "SELECT \\"Orders\\".\\"sum_orders_total\\" \\"total\\" + "SELECT \\"Orders\\".\\"sum_orders_total\\" \\"total\\" FROM ( SELECT sum(\\"orders\\".amount) \\"sum_orders_total\\" @@ -591,6 +591,36 @@ Array [ ] `; +exports[`SQL API Postgres (Data) member expression with granularity and raw time dimensions 1`] = ` +Array [ + Object { + "quarter": 2024-01-01T00:00:00.000Z, + "total": 100, + "updatedAt": 2025-01-01T00:00:00.000Z, + }, + Object { + "quarter": 2024-01-01T00:00:00.000Z, + "total": 300, + "updatedAt": 2025-01-02T00:00:00.000Z, + }, + Object { + "quarter": 2024-01-01T00:00:00.000Z, + "total": 600, + "updatedAt": 2025-01-03T00:00:00.000Z, + }, + Object { + "quarter": 2024-01-01T00:00:00.000Z, + "total": 1100, + "updatedAt": 2025-01-04T00:00:00.000Z, + }, + Object { + "quarter": 2024-01-01T00:00:00.000Z, + "total": 1700, + "updatedAt": 2025-01-05T00:00:00.000Z, + }, +] +`; + exports[`SQL API Postgres (Data) metabase max number: metabase max number 1`] = ` Array [ Object { diff --git a/packages/cubejs-testing/test/smoke-cubesql.test.ts b/packages/cubejs-testing/test/smoke-cubesql.test.ts index 429ef30814fac..f7a23bfe2b9f4 100644 --- a/packages/cubejs-testing/test/smoke-cubesql.test.ts +++ b/packages/cubejs-testing/test/smoke-cubesql.test.ts @@ -886,5 +886,33 @@ filter_subq AS ( const res = await connection.query(query); expect(res.rows).toMatchSnapshot('measure-with-ad-hoc-filters-and-original-measure'); }); + + /// Query references `updatedAt` in three places: in outer projection, in grouping key and in window + /// Incoming query is consistent: all three references same column + /// This tests that generated SQL for pushdown remains consistent: + /// whatever is present in grouping key should be present in window expression + /// Interesting part here is that single column (and member expression) contains + /// two different TD, one with granularity and one without, and both have to match grouping key + test('member expression with granularity and raw time dimensions', async () => { + const query = ` + SELECT + DATE_TRUNC('qtr', createdAt) AS quarter, + updatedAt, + SUM(CASE + WHEN sum(totalAmount) IS NOT NULL THEN sum(totalAmount) + ELSE 0 + END) OVER ( + PARTITION BY DATE_TRUNC('qtr', createdAt) + ORDER BY updatedAt + ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW + ) AS total + FROM Orders + GROUP BY + 1, 2 + `; + + const res = await connection.query(query); + expect(res.rows).toMatchSnapshot(); + }); }); });