From 0e4593f90a6126c485c9b72de34d030257792946 Mon Sep 17 00:00:00 2001 From: Aleksandr Romanenko Date: Fri, 7 Aug 2026 16:52:59 +0200 Subject: [PATCH 1/4] fix(tesseract): parenthesize member SQL spliced into filter templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A filter template places its own operator next to the member's rendered SQL (`{{ column }} = {{ value }}`, `{{ column }} IS NOT NULL`, …). A member whose `sql` is a bare expression then re-associates: when its own top-level operator binds weaker than the template's, that operator captures only the tail of the member expression. A measure `sql: "{total} IS NOT NULL"` filtered by `equals true` rendered `HAVING (sum(...) IS NOT NULL = CAST(? AS BOOLEAN))`, which Trino and Athena reject. Aggregate-typed measures were safe by accident, being wrapped in their own function call. Where the member's top level is `AND`/`OR` the mis-parse stays valid SQL and silently returns a different row set — in `WHERE` over a dimension as much as in `HAVING` over a measure. Render the member as one operand when `FilterSqlContext` is built, so every operator receives it already pinned, and keep the field private so a new call site cannot bypass it. Atomicity is decided by the existing expression scanner: plain columns, aggregates, casts and CASE keep their shape, so the wrapping does not spread through filters that never had the hazard. An expression ending in a line comment gets its closing parenthesis on a line of its own — on the same line the comment would swallow it. Co-Authored-By: Claude Opus 5 (1M context) --- .../postgres/filter-member-sql-parens.test.ts | 345 ++++++++++++++++++ .../test/unit/base-query.test.ts | 2 +- .../filter/operators/comparison.rs | 8 +- .../filter/operators/date_range.rs | 4 +- .../filter/operators/date_single.rs | 8 +- .../filter/operators/equality.rs | 4 +- .../filter/operators/filter_sql_context.rs | 44 ++- .../physical_plan/filter/operators/in_list.rs | 9 +- .../physical_plan/filter/operators/like.rs | 4 +- .../filter/operators/nullability.rs | 5 +- .../filter/operators/rolling_window.rs | 4 +- .../operators/to_date_rolling_window.rs | 2 +- .../src/physical_plan/filter/typed_filter.rs | 28 +- .../src/utils/sql_expression_scanner.rs | 51 ++- 14 files changed, 474 insertions(+), 44 deletions(-) create mode 100644 packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts diff --git a/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts b/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts new file mode 100644 index 0000000000000..63a0c0544d259 --- /dev/null +++ b/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts @@ -0,0 +1,345 @@ +import { getEnv } from '@cubejs-backend/shared'; +import { PostgresQuery } from '../../../src/adapter'; +import { prepareYamlCompiler } from '../../unit/PrepareCompiler'; +import { dbRunner } from './PostgresDBRunner'; + +// A member's `sql` is an arbitrary expression, and a filter template splices it +// next to an operator of its own. When the member's own top-level operator binds +// weaker than the filter's, the unparenthesized splice re-associates: the filter +// operator captures only the tail of the member expression. That is a syntax +// error on some dialects and — for `AND`/`OR` members — valid SQL over a +// different row set on all of them. +// +// Inline data (6 rows): +// id grp amount flag note +// 1 g1 100 true 'alpha' +// 2 g1 10 true NULL +// 3 g2 200 false 'beta' +// 4 g2 10 false NULL +// 5 g3 100 NULL 'gamma' +// 6 g3 10 NULL NULL +// +// `big_and_flag` = `amount > 50 AND flag` evaluates to +// 1: true 2: false 3: false 4: false 5: NULL 6: false +// and `big_or_flag` = `amount > 50 OR flag` to +// 1: true 2: true 3: true 4: false 5: true 6: NULL +describe('Filter member SQL parenthesization', () => { + jest.setTimeout(200000); + + const compilers = prepareYamlCompiler(` +cubes: + - name: orders + sql: > + SELECT * FROM (VALUES + (1, 'g1', 100, TRUE, 'alpha', '2024-01-01'::timestamp), + (2, 'g1', 10, TRUE, NULL, '2024-01-02'::timestamp), + (3, 'g2', 200, FALSE, 'beta', '2024-01-03'::timestamp), + (4, 'g2', 10, FALSE, NULL, '2024-01-04'::timestamp), + (5, 'g3', 100, NULL, 'gamma', '2024-01-05'::timestamp), + (6, 'g3', 10, NULL, NULL, '2024-01-06'::timestamp) + ) AS t(id, grp, amount, flag, note, created_at) + + dimensions: + - name: id + sql: id + type: number + primary_key: true + public: true + + - name: grp + sql: grp + type: string + + - name: amount + sql: amount + type: number + + # Top-level AND — binds weaker than every filter operator. + - name: big_and_flag + sql: "amount > 50 AND flag" + type: boolean + + # Top-level OR — same, and keeps NULL propagation visible. + - name: big_or_flag + sql: "amount > 50 OR flag" + type: boolean + + # Top-level comparison — Postgres rejects a second comparison next to it. + - name: big + sql: "amount > 50" + type: boolean + + - name: amount_plus + sql: "amount + 1" + type: number + + - name: note_tagged + sql: "note || '-tag'" + type: string + + - name: amount_plus_commented + sql: "amount + 1 -- one more" + type: number + + - name: shifted_at + sql: "created_at + interval '1 day'" + type: time + + measures: + - name: count + type: count + + - name: total + sql: amount + type: sum + + - name: flag_any + sql: "bool_or(flag)" + type: boolean + + # Top-level AND over two aggregates. + - name: busy + sql: "{total} > 150 AND {flag_any}" + type: boolean + + # Top-level comparison over an aggregate. + - name: total_over_150 + sql: "{total} > 150" + type: boolean + + # The reported model's shape: a calculated boolean over a plain measure. + - name: total_is_set + sql: "{total} IS NOT NULL" + type: boolean + `); + + async function buildSql(q: any): Promise<[string, any[]]> { + await compilers.compiler.compile(); + const query = new PostgresQuery(compilers, { + ...q, + timezone: 'UTC', + preAggregationsSchema: '', + }); + return query.buildSqlAndParams() as [string, any[]]; + } + + // Executes the query and compares the row set. Returns the generated SQL so a + // caller can additionally assert its shape. + async function expectRows(q: any, expected: any[]): Promise { + const sqlAndParams = await buildSql(q); + const res = await dbRunner.testQuery(sqlAndParams); + expect(res).toEqual(expected); + return sqlAndParams[0]; + } + + const idRows = (...ids: number[]) => ids.map(id => ({ orders__id: id, orders__count: '1' })); + + const byId = (filters: any[]) => ({ + measures: ['orders.count'], + dimensions: ['orders.id'], + filters, + order: [{ id: 'orders.id' }], + }); + + describe('WHERE — dimension whose SQL is a top-level AND/OR', () => { + it('equals', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byId([{ member: 'orders.big_and_flag', operator: 'equals', values: ['false'] }]), + idRows(2, 3, 4, 6) + ); + }); + + it('notEquals', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byId([{ member: 'orders.big_and_flag', operator: 'notEquals', values: ['true'] }]), + idRows(2, 3, 4, 5, 6) + ); + }); + + it('equals with several values (IN list)', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byId([{ member: 'orders.big_and_flag', operator: 'equals', values: ['true', 'false'] }]), + idRows(1, 2, 3, 4, 6) + ); + }); + + it('notEquals with several values (NOT IN list)', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byId([{ member: 'orders.big_or_flag', operator: 'notEquals', values: ['true', 'false'] }]), + idRows(6) + ); + }); + + it('set', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byId([{ member: 'orders.big_and_flag', operator: 'set' }]), + idRows(1, 2, 3, 4, 6) + ); + }); + + it('notSet', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byId([{ member: 'orders.big_or_flag', operator: 'notSet' }]), + idRows(6) + ); + }); + }); + + describe('WHERE — dimension whose SQL is a top-level comparison', () => { + // Unparenthesized this renders `amount > 50 = CAST($1 AS BOOLEAN)`, which + // Postgres rejects outright ("syntax error at or near ="). + it('equals', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byId([{ member: 'orders.big', operator: 'equals', values: ['false'] }]), + idRows(2, 4, 6) + ); + }); + + it('notEquals', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byId([{ member: 'orders.big', operator: 'notEquals', values: ['true'] }]), + idRows(2, 4, 6) + ); + }); + }); + + describe('HAVING — measure whose SQL is a top-level AND/comparison', () => { + const byGrp = (filters: any[]) => ({ + measures: ['orders.total'], + dimensions: ['orders.grp'], + filters, + order: [{ id: 'orders.grp' }], + }); + + it('equals on a top-level AND measure', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byGrp([{ member: 'orders.busy', operator: 'equals', values: ['false'] }]), + [ + { orders__grp: 'g1', orders__total: '110' }, + { orders__grp: 'g2', orders__total: '210' }, + { orders__grp: 'g3', orders__total: '110' }, + ] + ); + }); + + it('equals on a top-level comparison measure', async () => { + if (!getEnv('nativeSqlPlanner')) return; + await expectRows( + byGrp([{ member: 'orders.total_over_150', operator: 'equals', values: ['true'] }]), + [{ orders__grp: 'g2', orders__total: '210' }] + ); + }); + + // The reported model: `sum(...) IS NOT NULL = CAST($1 AS BOOLEAN)` is a + // syntax error on Trino/Athena. Postgres happens to parse it the intended + // way, so only the emitted shape can be asserted here. + it('equals on a top-level IS NOT NULL measure', async () => { + if (!getEnv('nativeSqlPlanner')) return; + const sql = await expectRows( + byGrp([{ member: 'orders.total_is_set', operator: 'equals', values: ['true'] }]), + [ + { orders__grp: 'g1', orders__total: '110' }, + { orders__grp: 'g2', orders__total: '210' }, + { orders__grp: 'g3', orders__total: '110' }, + ] + ); + expect(sql).toContain('(sum("orders".amount) IS NOT NULL) ='); + }); + }); + + // The remaining operators cannot be made to diverge on Postgres — its + // precedence happens to agree with the intended reading — but they splice the + // member the same way, so the wrapping is asserted on the emitted SQL. + describe('emitted SQL wraps a compound member for every operator', () => { + // Each expectation is the whole rendered predicate, so a case also pins the + // operator and pattern it belongs to rather than only the parentheses. + const NUM = '(amount + 1)'; + const STR = '(note || \'-tag\')'; + const TS = '(created_at + interval \'1 day\')'; + + const cases: Array<[string, any, string]> = [ + ['gt', { member: 'orders.amount_plus', operator: 'gt', values: ['10'] }, `WHERE (${NUM} > $1)`], + ['gte', { member: 'orders.amount_plus', operator: 'gte', values: ['10'] }, `WHERE (${NUM} >= $1)`], + ['lt', { member: 'orders.amount_plus', operator: 'lt', values: ['10'] }, `WHERE (${NUM} < $1)`], + ['lte', { member: 'orders.amount_plus', operator: 'lte', values: ['10'] }, `WHERE (${NUM} <= $1)`], + ['contains', { member: 'orders.note_tagged', operator: 'contains', values: ['alpha'] }, + `WHERE ((${STR} ILIKE '%' || $1|| '%'))`], + ['notContains', { member: 'orders.note_tagged', operator: 'notContains', values: ['alpha'] }, + `WHERE ((${STR} NOT ILIKE '%' || $1|| '%') OR ${STR} IS NULL)`], + ['startsWith', { member: 'orders.note_tagged', operator: 'startsWith', values: ['alpha'] }, + `WHERE ((${STR} ILIKE $1|| '%'))`], + ['endsWith', { member: 'orders.note_tagged', operator: 'endsWith', values: ['tag'] }, + `WHERE ((${STR} ILIKE '%' || $1))`], + ['inDateRange', { + member: 'orders.shifted_at', + operator: 'inDateRange', + values: ['2024-01-01T00:00:00.000', '2024-01-31T23:59:59.999'], + }, `WHERE (${TS} >= $1::timestamptz AND ${TS} <= $2::timestamptz)`], + ['notInDateRange', { + member: 'orders.shifted_at', + operator: 'notInDateRange', + values: ['2024-01-01T00:00:00.000', '2024-01-31T23:59:59.999'], + }, `WHERE (${TS} < $1::timestamptz OR ${TS} > $2::timestamptz)`], + ['beforeDate', { + member: 'orders.shifted_at', + operator: 'beforeDate', + values: ['2024-01-31T23:59:59.999'], + }, `WHERE (${TS} < $1::timestamptz)`], + ['afterDate', { + member: 'orders.shifted_at', + operator: 'afterDate', + values: ['2024-01-01T00:00:00.000'], + }, `WHERE (${TS} > $1::timestamptz)`], + ]; + + it.each(cases)('%s', async (_name, filter, expected) => { + if (!getEnv('nativeSqlPlanner')) return; + const [sql] = await buildSql(byId([filter])); + expect(sql).toContain(expected); + }); + }); + + // A member SQL ending in a line comment would comment out the closing + // parenthesis, so it gets a line of its own. + it('wraps a member whose SQL ends in a line comment', async () => { + if (!getEnv('nativeSqlPlanner')) return; + const sql = await expectRows( + byId([{ member: 'orders.amount_plus_commented', operator: 'gt', values: ['50'] }]), + idRows(1, 3, 5) + ); + expect(sql).toContain('(amount + 1 -- one more\n) > $1'); + }); + + // The wrapping must stay off atomic members, or every filter in every model + // would change shape. + describe('atomic members stay unwrapped', () => { + it('plain column dimension', async () => { + if (!getEnv('nativeSqlPlanner')) return; + const [sql] = await buildSql(byId([ + { member: 'orders.amount', operator: 'equals', values: ['100'] }, + ])); + expect(sql).toContain('"orders".amount = '); + expect(sql).not.toContain('("orders".amount) = '); + }); + + it('aggregate measure', async () => { + if (!getEnv('nativeSqlPlanner')) return; + const [sql] = await buildSql({ + measures: ['orders.total'], + dimensions: ['orders.grp'], + filters: [{ member: 'orders.total', operator: 'gt', values: ['100'] }], + }); + expect(sql).toContain('sum("orders".amount) > '); + expect(sql).not.toContain('(sum("orders".amount)) > '); + }); + }); +}); 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 f575e562453a9..5b8871b0245b1 100644 --- a/packages/cubejs-schema-compiler/test/unit/base-query.test.ts +++ b/packages/cubejs-schema-compiler/test/unit/base-query.test.ts @@ -2109,7 +2109,7 @@ describe('SQL Generation', () => { const queryString = queryAndParams[0]; expect(queryString).toContain('1 = 1 "order__proxied"'); expect(queryString).toContain('(select * from order where 1 = 1) AS "order"'); - expect(queryString).toContain('WHERE (1 = 1 = ?)'); + expect(queryString).toContain('WHERE ((1 = 1) = ?)'); }); it('correctly substitute filter params in cube\'s query measure used in filter', async () => { diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/comparison.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/comparison.rs index 084b748dbe81a..f7522f5ad6c24 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/comparison.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/comparison.rs @@ -6,10 +6,10 @@ impl FilterOperationSql for ComparisonOp { fn to_sql(&self, ctx: &FilterSqlContext) -> Result { let param = ctx.allocate_and_cast(&self.value, &self.member_type)?; match self.kind { - ComparisonKind::Gt => ctx.plan_templates.gt(ctx.member_sql.to_string(), param), - ComparisonKind::Gte => ctx.plan_templates.gte(ctx.member_sql.to_string(), param), - ComparisonKind::Lt => ctx.plan_templates.lt(ctx.member_sql.to_string(), param), - ComparisonKind::Lte => ctx.plan_templates.lte(ctx.member_sql.to_string(), param), + ComparisonKind::Gt => ctx.plan_templates.gt(ctx.member_sql().to_string(), param), + ComparisonKind::Gte => ctx.plan_templates.gte(ctx.member_sql().to_string(), param), + ComparisonKind::Lt => ctx.plan_templates.lt(ctx.member_sql().to_string(), param), + ComparisonKind::Lte => ctx.plan_templates.lte(ctx.member_sql().to_string(), param), } } } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/date_range.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/date_range.rs index e9d53c851642c..0ffcccb0ac84c 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/date_range.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/date_range.rs @@ -8,12 +8,12 @@ impl FilterOperationSql for DateRangeOp { let to_param = ctx.format_and_allocate_to_date(&self.to)?; match self.kind { DateRangeKind::InRange => ctx.plan_templates.time_range_filter( - ctx.member_sql.to_string(), + ctx.member_sql().to_string(), from_param, to_param, ), DateRangeKind::NotInRange => ctx.plan_templates.time_not_in_range_filter( - ctx.member_sql.to_string(), + ctx.member_sql().to_string(), from_param, to_param, ), diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/date_single.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/date_single.rs index 803f27cd65816..39d724297b2ed 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/date_single.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/date_single.rs @@ -9,10 +9,10 @@ impl FilterOperationSql for DateSingleOp { let param = ctx.format_and_allocate_from_date(&self.value)?; match self.kind { DateSingleKind::Before => { - ctx.plan_templates.lt(ctx.member_sql.to_string(), param) + ctx.plan_templates.lt(ctx.member_sql().to_string(), param) } DateSingleKind::AfterOrOn => { - ctx.plan_templates.gte(ctx.member_sql.to_string(), param) + ctx.plan_templates.gte(ctx.member_sql().to_string(), param) } _ => unreachable!(), } @@ -21,10 +21,10 @@ impl FilterOperationSql for DateSingleOp { let param = ctx.format_and_allocate_to_date(&self.value)?; match self.kind { DateSingleKind::BeforeOrOn => { - ctx.plan_templates.lte(ctx.member_sql.to_string(), param) + ctx.plan_templates.lte(ctx.member_sql().to_string(), param) } DateSingleKind::After => { - ctx.plan_templates.gt(ctx.member_sql.to_string(), param) + ctx.plan_templates.gt(ctx.member_sql().to_string(), param) } _ => unreachable!(), } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/equality.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/equality.rs index 52ed6682cc146..2d6a049eb8f21 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/equality.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/equality.rs @@ -9,10 +9,10 @@ impl FilterOperationSql for EqualityOp { let need_null_check = self.negated; if self.negated { ctx.plan_templates - .not_equals(ctx.member_sql.to_string(), param, need_null_check) + .not_equals(ctx.member_sql().to_string(), param, need_null_check) } else { ctx.plan_templates - .equals(ctx.member_sql.to_string(), param, false) + .equals(ctx.member_sql().to_string(), param, false) } } } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs index 1fac6523df484..f54dc69bf4c34 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs @@ -2,6 +2,7 @@ use crate::cube_bridge::base_query_options::FilterValue; use crate::planner::query_tools::QueryTools; use crate::planner::sql_templates::{PlanSqlTemplates, TemplateProjectionColumn}; use crate::planner::QueryDateTimeHelper; +use crate::utils::sql_expression_scanner::{ends_in_line_comment, is_top_level_compound}; use cubenativeutils::CubeError; use std::rc::Rc; @@ -15,7 +16,7 @@ enum DateBound { } pub struct FilterSqlContext<'a> { - pub member_sql: &'a str, + member_sql: String, pub query_tools: &'a Rc, pub plan_templates: &'a PlanSqlTemplates, pub use_db_time_zone: bool, @@ -23,6 +24,47 @@ pub struct FilterSqlContext<'a> { } impl<'a> FilterSqlContext<'a> { + pub fn new( + member_sql: &str, + query_tools: &'a Rc, + plan_templates: &'a PlanSqlTemplates, + use_db_time_zone: bool, + use_raw_values: bool, + ) -> Self { + Self { + member_sql: Self::as_operand(member_sql), + query_tools, + plan_templates, + use_db_time_zone, + use_raw_values, + } + } + + /// The member's SQL as a single operand: safe to place next to an operator + /// of any precedence. + pub fn member_sql(&self) -> &str { + &self.member_sql + } + + // A member's SQL is an expression of unknown shape. When its own top-level + // operator binds weaker than the operator a filter template puts beside it, + // the bare splice re-associates and that operator captures only the tail of + // the member expression — a syntax error on some dialects, a silently + // different predicate on the rest. Parentheses pin the whole expression as + // one operand; an atomic expression needs none and keeps its shape. + fn as_operand(member_sql: &str) -> String { + if !is_top_level_compound(member_sql) { + return member_sql.to_string(); + } + // An expression ending in a line comment would swallow the closing + // parenthesis, so that one gets a line of its own. + if ends_in_line_comment(member_sql) { + format!("({}\n)", member_sql) + } else { + format!("({})", member_sql) + } + } + pub fn allocate_param(&self, value: &str) -> String { self.query_tools.allocate_param(value) } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/in_list.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/in_list.rs index bf75934571463..1845c8764f4d7 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/in_list.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/in_list.rs @@ -9,11 +9,14 @@ impl FilterOperationSql for InListOp { let allocated = ctx.allocate_and_cast_values(&self.values, &self.member_type)?; if self.negated { - ctx.plan_templates - .not_in_where(ctx.member_sql.to_string(), allocated, need_null_check) + ctx.plan_templates.not_in_where( + ctx.member_sql().to_string(), + allocated, + need_null_check, + ) } else { ctx.plan_templates - .in_where(ctx.member_sql.to_string(), allocated, need_null_check) + .in_where(ctx.member_sql().to_string(), allocated, need_null_check) } } } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/like.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/like.rs index 1c8f491a6c11b..f94500c74a9e5 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/like.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/like.rs @@ -20,7 +20,7 @@ impl FilterOperationSql for LikeOp { .into_iter() .map(|v| { ctx.plan_templates.ilike( - ctx.member_sql, + ctx.member_sql(), &v, self.start_wild, self.end_wild, @@ -37,7 +37,7 @@ impl FilterOperationSql for LikeOp { }; let null_check = if need_null_check { ctx.plan_templates - .or_is_null_check(ctx.member_sql.to_string())? + .or_is_null_check(ctx.member_sql().to_string())? } else { "".to_string() }; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/nullability.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/nullability.rs index 5ee721955a062..6d7c295b17063 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/nullability.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/nullability.rs @@ -5,9 +5,10 @@ use cubenativeutils::CubeError; impl FilterOperationSql for NullabilityOp { fn to_sql(&self, ctx: &FilterSqlContext) -> Result { if self.negated { - ctx.plan_templates.not_set_where(ctx.member_sql.to_string()) + ctx.plan_templates + .not_set_where(ctx.member_sql().to_string()) } else { - ctx.plan_templates.set_where(ctx.member_sql.to_string()) + ctx.plan_templates.set_where(ctx.member_sql().to_string()) } } } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/rolling_window.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/rolling_window.rs index c77063dfde259..88d62d5e7f8b3 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/rolling_window.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/rolling_window.rs @@ -11,7 +11,7 @@ impl FilterOperationSql for RegularRollingWindowOp { let from = ctx.extend_date_range_bound(from, &self.trailing, true)?; let to = ctx.extend_date_range_bound(to, &self.leading, false)?; - let date_field = ctx.convert_tz(ctx.member_sql)?; + let date_field = ctx.convert_tz(ctx.member_sql())?; match (&from, &to) { (Some(from), Some(to)) => { @@ -28,7 +28,7 @@ impl FilterOperationSql for RegularRollingWindowOp { impl FilterOperationSql for RollingWindowOffsetOp { fn to_sql(&self, ctx: &FilterSqlContext) -> Result { let from_start = self.offset == "start"; - let member = ctx.member_sql.to_string(); + let member = ctx.member_sql().to_string(); // Anchor: range start (formatted to start-of-day) for offset 'start', // range end (formatted to end-of-day) for 'end'. Both bounds share it. diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/to_date_rolling_window.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/to_date_rolling_window.rs index e3a1b2f5883f6..69ea9a7bb1144 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/to_date_rolling_window.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/to_date_rolling_window.rs @@ -10,7 +10,7 @@ impl FilterOperationSql for ToDateRollingWindowOp { .granularity .apply_to_input_sql(ctx.plan_templates, from)?; - let date_field = ctx.convert_tz(ctx.member_sql)?; + let date_field = ctx.convert_tz(ctx.member_sql())?; ctx.plan_templates.time_range_filter(date_field, from, to) } } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/typed_filter.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/typed_filter.rs index bb9acc21f293e..237608a105a94 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/typed_filter.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/typed_filter.rs @@ -34,13 +34,13 @@ impl ToSql for TypedFilter { let resolved = resolve_base_symbol(self.member_evaluator()); let member_sql = visitor.apply_for_filter(&resolved, node_processor, templates)?; - let ctx = FilterSqlContext { - member_sql: &member_sql, - query_tools: &query_tools, - plan_templates: templates, - use_db_time_zone: !filters_ctx.use_local_tz, - use_raw_values: self.use_raw_values(), - }; + let ctx = FilterSqlContext::new( + &member_sql, + &query_tools, + templates, + !filters_ctx.use_local_tz, + self.use_raw_values(), + ); dispatch_to_sql(self.operation(), &ctx) } @@ -76,13 +76,13 @@ impl TypedFilter { } else { column_sql.as_str() }; - let ctx = FilterSqlContext { + let ctx = FilterSqlContext::new( member_sql, query_tools, plan_templates, use_db_time_zone, - use_raw_values: self.use_raw_values(), - }; + self.use_raw_values(), + ); dispatch_to_sql(self.operation(), &ctx) } FilterParamsColumn::Compiled(compiled) => { @@ -141,13 +141,13 @@ impl TypedFilter { // RollingWindowOffset carries [from, to, trailing, leading, offset]; // only the from/to dates are filter-param args for the callback. FilterOp::DateRange(_) | FilterOp::DateSingle(_) | FilterOp::RollingWindowOffset(_) => { - let ctx = FilterSqlContext { - member_sql: "", + let ctx = FilterSqlContext::new( + "", query_tools, plan_templates, use_db_time_zone, - use_raw_values: self.use_raw_values(), - }; + self.use_raw_values(), + ); let from = self .values() .first() diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/utils/sql_expression_scanner.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/utils/sql_expression_scanner.rs index e66ce6e319d8c..7005d3e82c5c2 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/utils/sql_expression_scanner.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/utils/sql_expression_scanner.rs @@ -57,6 +57,10 @@ struct Tokenizer<'a> { bytes: &'a [u8], pos: usize, depth: usize, + // Set when a line comment ran to the end of the input, cleared when one ends + // at its newline. Whatever the last line comment left here says whether the + // input as a whole ends inside a comment. + open_line_comment: bool, } impl<'a> Tokenizer<'a> { @@ -66,6 +70,7 @@ impl<'a> Tokenizer<'a> { bytes: src.as_bytes(), pos: 0, depth: 0, + open_line_comment: false, } } @@ -88,16 +93,12 @@ impl<'a> Tokenizer<'a> { } b'-' if self.peek(1) == Some(b'-') => { self.pos += 2; - while !self.at_eof() && self.peek(0) != Some(b'\n') { - self.pos += 1; - } + self.skip_to_line_end(); } b'/' if self.peek(1) == Some(b'/') => { // Line comment variant in BigQuery and Snowflake. self.pos += 2; - while !self.at_eof() && self.peek(0) != Some(b'\n') { - self.pos += 1; - } + self.skip_to_line_end(); } b'/' if self.peek(1) == Some(b'*') => { self.pos += 2; @@ -122,6 +123,13 @@ impl<'a> Tokenizer<'a> { } } + fn skip_to_line_end(&mut self) { + while !self.at_eof() && self.peek(0) != Some(b'\n') { + self.pos += 1; + } + self.open_line_comment = self.at_eof(); + } + fn next_token(&mut self) -> Option> { self.skip_trivia(); if self.at_eof() { @@ -574,6 +582,14 @@ pub fn is_top_level_compound(sql: &str) -> bool { false } +/// Returns `true` if `sql` ends inside a line comment, so anything appended on +/// the same line would be commented out. +pub fn ends_in_line_comment(sql: &str) -> bool { + let mut tokenizer = Tokenizer::new(sql); + while tokenizer.next_token().is_some() {} + tokenizer.open_line_comment +} + // ---------- Template analyzer: compile-time placeholder contexts ---------- /// Analyses an `SqlCall` template and returns, for each `{arg:N}` index present, @@ -857,6 +873,29 @@ mod tests { assert!(!is_top_level_compound("{user_id:Int64}")); } + // ----- ends_in_line_comment ----- + + #[test] + fn line_comment_running_to_the_end() { + assert!(ends_in_line_comment("a + b -- note")); + assert!(ends_in_line_comment("a + b // note")); + assert!(ends_in_line_comment("a -- note\n + b -- tail")); + } + + #[test] + fn line_comment_closed_by_newline() { + assert!(!ends_in_line_comment("a -- note\n + b")); + assert!(!ends_in_line_comment("a + b -- note\n")); + } + + #[test] + fn no_line_comment_at_all() { + assert!(!ends_in_line_comment("a + b")); + assert!(!ends_in_line_comment("")); + assert!(!ends_in_line_comment("'-- not a comment'")); + assert!(!ends_in_line_comment("a /* block */")); + } + #[test] fn nested_case_is_atomic() { assert!(!is_top_level_compound( From c260acd8a5cbe32db681f4b2565a56b25f337522 Mon Sep 17 00:00:00 2001 From: Aleksandr Romanenko Date: Mon, 10 Aug 2026 16:06:57 +0200 Subject: [PATCH 2/4] fix(tesseract): wrap a filter member whose SQL ends in a line comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deciding atomicity first left the line-comment guard unreachable for the case that needs it just as much: `sql: "amount -- note"` has no top-level operator, so it went in bare and the template appended its operator to the commented line — `amount -- note > $1`, where the predicate silently disappears. Ask the comment question first and let it force the wrapping regardless of atomicity, since the reason is the closing parenthesis rather than precedence. Gate the test file on the planner once at the describe level: a bare `return` per test reported as a pass under the legacy planner, so a regression that read the flag as false everywhere would have turned the file green instead of red. Co-Authored-By: Claude Opus 5 (1M context) --- .../postgres/filter-member-sql-parens.test.ts | 53 ++++++++++--------- .../filter/operators/filter_sql_context.rs | 14 ++--- 2 files changed, 36 insertions(+), 31 deletions(-) diff --git a/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts b/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts index 63a0c0544d259..2facfe967f4dc 100644 --- a/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts +++ b/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts @@ -23,7 +23,11 @@ import { dbRunner } from './PostgresDBRunner'; // 1: true 2: false 3: false 4: false 5: NULL 6: false // and `big_or_flag` = `amount > 50 OR flag` to // 1: true 2: true 3: true 4: false 5: true 6: NULL -describe('Filter member SQL parenthesization', () => { + +// The fix lives in the Tesseract planner; the legacy planner is out of scope. +const tesseract = getEnv('nativeSqlPlanner'); + +(tesseract ? describe : describe.skip)('Filter member SQL parenthesization', () => { jest.setTimeout(200000); const compilers = prepareYamlCompiler(` @@ -81,6 +85,11 @@ cubes: sql: "amount + 1 -- one more" type: number + # Atomic, but still swallows whatever a template appends on that line. + - name: amount_commented + sql: "amount -- as is" + type: number + - name: shifted_at sql: "created_at + interval '1 day'" type: time @@ -143,7 +152,6 @@ cubes: describe('WHERE — dimension whose SQL is a top-level AND/OR', () => { it('equals', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byId([{ member: 'orders.big_and_flag', operator: 'equals', values: ['false'] }]), idRows(2, 3, 4, 6) @@ -151,7 +159,6 @@ cubes: }); it('notEquals', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byId([{ member: 'orders.big_and_flag', operator: 'notEquals', values: ['true'] }]), idRows(2, 3, 4, 5, 6) @@ -159,7 +166,6 @@ cubes: }); it('equals with several values (IN list)', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byId([{ member: 'orders.big_and_flag', operator: 'equals', values: ['true', 'false'] }]), idRows(1, 2, 3, 4, 6) @@ -167,7 +173,6 @@ cubes: }); it('notEquals with several values (NOT IN list)', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byId([{ member: 'orders.big_or_flag', operator: 'notEquals', values: ['true', 'false'] }]), idRows(6) @@ -175,7 +180,6 @@ cubes: }); it('set', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byId([{ member: 'orders.big_and_flag', operator: 'set' }]), idRows(1, 2, 3, 4, 6) @@ -183,7 +187,6 @@ cubes: }); it('notSet', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byId([{ member: 'orders.big_or_flag', operator: 'notSet' }]), idRows(6) @@ -195,7 +198,6 @@ cubes: // Unparenthesized this renders `amount > 50 = CAST($1 AS BOOLEAN)`, which // Postgres rejects outright ("syntax error at or near ="). it('equals', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byId([{ member: 'orders.big', operator: 'equals', values: ['false'] }]), idRows(2, 4, 6) @@ -203,7 +205,6 @@ cubes: }); it('notEquals', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byId([{ member: 'orders.big', operator: 'notEquals', values: ['true'] }]), idRows(2, 4, 6) @@ -220,7 +221,6 @@ cubes: }); it('equals on a top-level AND measure', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byGrp([{ member: 'orders.busy', operator: 'equals', values: ['false'] }]), [ @@ -232,7 +232,6 @@ cubes: }); it('equals on a top-level comparison measure', async () => { - if (!getEnv('nativeSqlPlanner')) return; await expectRows( byGrp([{ member: 'orders.total_over_150', operator: 'equals', values: ['true'] }]), [{ orders__grp: 'g2', orders__total: '210' }] @@ -243,7 +242,6 @@ cubes: // syntax error on Trino/Athena. Postgres happens to parse it the intended // way, so only the emitted shape can be asserted here. it('equals on a top-level IS NOT NULL measure', async () => { - if (!getEnv('nativeSqlPlanner')) return; const sql = await expectRows( byGrp([{ member: 'orders.total_is_set', operator: 'equals', values: ['true'] }]), [ @@ -302,28 +300,36 @@ cubes: ]; it.each(cases)('%s', async (_name, filter, expected) => { - if (!getEnv('nativeSqlPlanner')) return; const [sql] = await buildSql(byId([filter])); expect(sql).toContain(expected); }); }); - // A member SQL ending in a line comment would comment out the closing - // parenthesis, so it gets a line of its own. - it('wraps a member whose SQL ends in a line comment', async () => { - if (!getEnv('nativeSqlPlanner')) return; - const sql = await expectRows( - byId([{ member: 'orders.amount_plus_commented', operator: 'gt', values: ['50'] }]), - idRows(1, 3, 5) - ); - expect(sql).toContain('(amount + 1 -- one more\n) > $1'); + // A member SQL ending in a line comment swallows whatever the template + // appends on that line, so the closing parenthesis gets a line of its own — + // and an atomic expression needs the wrapping for that reason alone. + describe('member SQL ending in a line comment', () => { + it('compound expression', async () => { + const sql = await expectRows( + byId([{ member: 'orders.amount_plus_commented', operator: 'gt', values: ['50'] }]), + idRows(1, 3, 5) + ); + expect(sql).toContain('(amount + 1 -- one more\n) > $1'); + }); + + it('atomic expression', async () => { + const sql = await expectRows( + byId([{ member: 'orders.amount_commented', operator: 'gt', values: ['50'] }]), + idRows(1, 3, 5) + ); + expect(sql).toContain('(amount -- as is\n) > $1'); + }); }); // The wrapping must stay off atomic members, or every filter in every model // would change shape. describe('atomic members stay unwrapped', () => { it('plain column dimension', async () => { - if (!getEnv('nativeSqlPlanner')) return; const [sql] = await buildSql(byId([ { member: 'orders.amount', operator: 'equals', values: ['100'] }, ])); @@ -332,7 +338,6 @@ cubes: }); it('aggregate measure', async () => { - if (!getEnv('nativeSqlPlanner')) return; const [sql] = await buildSql({ measures: ['orders.total'], dimensions: ['orders.grp'], diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs index f54dc69bf4c34..0599f18b0d81c 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs @@ -53,16 +53,16 @@ impl<'a> FilterSqlContext<'a> { // different predicate on the rest. Parentheses pin the whole expression as // one operand; an atomic expression needs none and keeps its shape. fn as_operand(member_sql: &str) -> String { + // An expression ending in a line comment swallows whatever the template + // appends on that line, so it needs the wrapping — and a line of its own + // for the closing parenthesis — however atomic it otherwise is. + if ends_in_line_comment(member_sql) { + return format!("({}\n)", member_sql); + } if !is_top_level_compound(member_sql) { return member_sql.to_string(); } - // An expression ending in a line comment would swallow the closing - // parenthesis, so that one gets a line of its own. - if ends_in_line_comment(member_sql) { - format!("({}\n)", member_sql) - } else { - format!("({})", member_sql) - } + format!("({})", member_sql) } pub fn allocate_param(&self, value: &str) -> String { From 8f11f9df8cd25444554f4029437b1b7515968968 Mon Sep 17 00:00:00 2001 From: Aleksandr Romanenko Date: Tue, 18 Aug 2026 18:30:34 +0200 Subject: [PATCH 3/4] test(tesseract): pin the filter operand contract in the crate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `as_operand` decides the comment question before the atomicity one, and that ordering is what keeps a trailing line comment from swallowing the closing parenthesis. Only the Postgres integration suite covered it, so swapping the two blocks back would compile and pass `cargo test`. The function is a pure `&str -> String`, so state the contract next to it: atomic stays bare, compound gets wrapped, and a trailing line comment forces the wrap with the parenthesis on a line of its own however atomic the expression is. Drop the data table from the integration test's header comment. It restated the inline `VALUES` twenty lines below, plus the per-row truth values derived from them — three copies of one fact, and the copy nobody runs is the one that rots. Co-Authored-By: Claude Opus 5 (1M context) --- .../postgres/filter-member-sql-parens.test.ts | 14 ------- .../filter/operators/filter_sql_context.rs | 37 +++++++++++++++++++ 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts b/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts index 2facfe967f4dc..3ed1794fab559 100644 --- a/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts +++ b/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts @@ -9,20 +9,6 @@ import { dbRunner } from './PostgresDBRunner'; // operator captures only the tail of the member expression. That is a syntax // error on some dialects and — for `AND`/`OR` members — valid SQL over a // different row set on all of them. -// -// Inline data (6 rows): -// id grp amount flag note -// 1 g1 100 true 'alpha' -// 2 g1 10 true NULL -// 3 g2 200 false 'beta' -// 4 g2 10 false NULL -// 5 g3 100 NULL 'gamma' -// 6 g3 10 NULL NULL -// -// `big_and_flag` = `amount > 50 AND flag` evaluates to -// 1: true 2: false 3: false 4: false 5: NULL 6: false -// and `big_or_flag` = `amount > 50 OR flag` to -// 1: true 2: true 3: true 4: false 5: true 6: NULL // The fix lives in the Tesseract planner; the legacy planner is out of scope. const tesseract = getEnv('nativeSqlPlanner'); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs index 0599f18b0d81c..473a5f8db9804 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/operators/filter_sql_context.rs @@ -260,3 +260,40 @@ impl<'a> FilterSqlContext<'a> { pub trait FilterOperationSql { fn to_sql(&self, ctx: &FilterSqlContext) -> Result; } + +#[cfg(test)] +mod tests { + use super::FilterSqlContext; + + fn as_operand(member_sql: &str) -> String { + FilterSqlContext::as_operand(member_sql) + } + + #[test] + fn atomic_expression_stays_bare() { + assert_eq!(as_operand("amount"), "amount"); + assert_eq!(as_operand("sum(amount)"), "sum(amount)"); + assert_eq!(as_operand(""), ""); + } + + #[test] + fn compound_expression_is_wrapped() { + assert_eq!(as_operand("amount > 50"), "(amount > 50)"); + assert_eq!( + as_operand("sum(amount) IS NOT NULL"), + "(sum(amount) IS NOT NULL)" + ); + } + + // The closing parenthesis, not precedence, is what a trailing line comment + // threatens, so the comment decides before atomicity does. + #[test] + fn trailing_line_comment_wraps_on_its_own_line() { + assert_eq!(as_operand("amount -- as is"), "(amount -- as is\n)"); + assert_eq!( + as_operand("amount + 1 -- one more"), + "(amount + 1 -- one more\n)" + ); + assert_eq!(as_operand("amount -- note\n + 1"), "(amount -- note\n + 1)"); + } +} From 641645bdb5951f01a4cbaa1e048db12e6337ce1e Mon Sep 17 00:00:00 2001 From: Aleksandr Romanenko Date: Tue, 18 Aug 2026 20:17:41 +0200 Subject: [PATCH 4/4] test(tesseract): cover the filter member operand in the crate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fix was only exercised from the JS side, behind a native rebuild and an env-gated Postgres suite. Two crate-level layers cover it where it lives. `tests/filter/member_operand.rs` pins the rendered filter SQL per operator over a compound member — equality, IN lists, nullability, comparison, LIKE, date range, and the reported model's calculated boolean in HAVING — plus the two members that must stay bare and the one ending in a line comment. No database, so it runs in every `cargo test`. `tests/integration/filter_member_operand.rs` checks rows instead, because the dangerous form of the mis-parse is valid SQL over a different row set and the emitted text cannot tell it from the intended reading. The AND member returns 5 rows where the mis-parse silently returns none, and the OR member under `notSet` returns the one customer whose expression is NULL where the mis-parse returns the two it degenerates to. Both row sets were derived independently against the seed before the snapshots were accepted. The AND member carries parentheses around its second operand on purpose: without them the mis-parse chains two comparisons, which Postgres rejects outright, and the test would prove the error case the comparison member already covers instead of the silent one. Co-Authored-By: Claude Opus 5 (1M context) --- .../yaml_files/common/filter_operand.yaml | 64 ++++ .../yaml_files/common/integration_basic.yaml | 19 ++ .../src/tests/filter/member_operand.rs | 290 ++++++++++++++++++ .../cubesqlplanner/src/tests/filter/mod.rs | 1 + .../integration/filter_member_operand.rs | 122 ++++++++ .../src/tests/integration/mod.rs | 1 + ...perand__equals_false_on_and_dimension.snap | 7 + ..._equals_on_calculated_boolean_measure.snap | 9 + ...__equals_true_on_comparison_dimension.snap | 7 + ...mber_operand__not_set_on_or_dimension.snap | 7 + 10 files changed, 527 insertions(+) create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/filter_operand.yaml create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter/member_operand.rs create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/filter_member_operand.rs create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_false_on_and_dimension.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_on_calculated_boolean_measure.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_true_on_comparison_dimension.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__not_set_on_or_dimension.snap diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/filter_operand.yaml b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/filter_operand.yaml new file mode 100644 index 0000000000000..7fe1d35d36883 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/filter_operand.yaml @@ -0,0 +1,64 @@ +cubes: + - name: orders + sql: "SELECT * FROM orders" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: amount + type: number + sql: amount + - name: flag + type: boolean + sql: flag + - name: note + type: string + sql: note + - name: created_at + type: time + sql: created_at + + # Top-level AND/OR — bind weaker than every filter operator. + - name: big_and_flag + type: boolean + sql: "{CUBE}.amount > 50 AND {CUBE}.flag" + - name: big_or_flag + type: boolean + sql: "{CUBE}.amount > 50 OR {CUBE}.flag" + + # Top-level comparison — a second one next to it is a syntax error on + # Postgres and Trino alike. + - name: big + type: boolean + sql: "{CUBE}.amount > 50" + + - name: amount_plus + type: number + sql: "{CUBE}.amount + 1" + - name: note_tagged + type: string + sql: "{CUBE}.note || '-tag'" + - name: shifted_at + type: time + sql: "{CUBE}.created_at + INTERVAL '1 day'" + + # Atomic, but a trailing line comment still swallows whatever the + # template appends on that line. + - name: amount_commented + type: number + sql: "{CUBE}.amount -- as is" + + measures: + - name: count + type: count + - name: total + type: sum + sql: amount + # The reported model's shape: a calculated boolean over a plain measure. + - name: total_is_set + type: boolean + sql: "{total} IS NOT NULL" + - name: total_over_150 + type: boolean + sql: "{total} > 150" diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/integration_basic.yaml b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/integration_basic.yaml index 88ee2c318db8d..11d26426fe97e 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/integration_basic.yaml +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/integration_basic.yaml @@ -15,6 +15,11 @@ cubes: - name: full_location type: string sql: "CONCAT({CUBE}.name, ' from ', {CUBE}.city)" + # Top-level OR over a nullable column, so the whole expression can be + # NULL while neither operand's own IS NULL is true. + - name: is_ny_or_alice + type: boolean + sql: "{CUBE}.city = 'New York' OR {CUBE}.name LIKE 'Alice%'" measures: - name: count type: count @@ -51,12 +56,26 @@ cubes: - name: customer_id type: number sql: customer_id + # Top-level AND — binds weaker than the filter's own operator. The + # second operand carries its own parentheses so the mis-parse stays + # valid SQL and shows up as wrong rows rather than a syntax error. + - name: is_big_completed + type: boolean + sql: "{CUBE}.amount > 100 AND ({CUBE}.status = 'completed')" + # Top-level comparison — a second one next to it is a syntax error. + - name: is_big + type: boolean + sql: "{CUBE}.amount > 100" measures: - name: count type: count - name: total_amount type: sum sql: amount + # The reported model's shape: a calculated boolean over a measure. + - name: total_amount_is_set + type: boolean + sql: "{total_amount} IS NOT NULL" - name: avg_amount type: avg sql: amount diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter/member_operand.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter/member_operand.rs new file mode 100644 index 0000000000000..7de56350e0c7c --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter/member_operand.rs @@ -0,0 +1,290 @@ +//! A filter template splices the member's rendered SQL next to an operator of +//! its own, so the member has to arrive as a single operand. These tests pin the +//! parenthesization per operator: a compound member is wrapped, an atomic one is +//! left alone, and a trailing line comment forces the wrap on its own line. + +use super::assert_filter; +use crate::cube_bridge::base_query_options::FilterValue; +use crate::test_fixtures::cube_bridge::MockSchema; +use crate::test_fixtures::test_utils::TestContext; +use indoc::indoc; + +fn build(filter_yaml: &str) -> (String, Vec) { + let schema = MockSchema::from_yaml_file("common/filter_operand.yaml"); + let ctx = TestContext::new(schema).unwrap(); + let query = format!("measures:\n - orders.count\n{}", filter_yaml); + ctx.build_filter_sql(&query) + .expect("Should generate filter SQL") +} + +const AND_MEMBER: &str = r#"("orders".amount > 50 AND "orders".flag)"#; +const NUM_MEMBER: &str = r#"("orders".amount + 1)"#; +const STR_MEMBER: &str = r#"("orders".note || '-tag')"#; +const TS_MEMBER: &str = r#"("orders".created_at + INTERVAL '1 day')"#; + +// ── equality ──────────────────────────────────────────────────────────────── + +#[test] +fn test_equals_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.big_and_flag + operator: equals + values: + - \"true\" + "}); + assert_filter( + &result, + &format!("({AND_MEMBER} = $_0_$::boolean)"), + &["true"], + ); +} + +#[test] +fn test_not_equals_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.big_and_flag + operator: notEquals + values: + - \"true\" + "}); + assert_filter( + &result, + &format!("({AND_MEMBER} <> $_0_$::boolean OR {AND_MEMBER} IS NULL)"), + &["true"], + ); +} + +#[test] +fn test_in_list_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.big_and_flag + operator: equals + values: + - \"true\" + - \"false\" + "}); + assert_filter( + &result, + &format!("({AND_MEMBER} IN ($_0_$::boolean, $_1_$::boolean))"), + &["true", "false"], + ); +} + +#[test] +fn test_not_in_list_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.big_and_flag + operator: notEquals + values: + - \"true\" + - \"false\" + "}); + assert_filter( + &result, + &format!("({AND_MEMBER} NOT IN ($_0_$::boolean, $_1_$::boolean) OR {AND_MEMBER} IS NULL)"), + &["true", "false"], + ); +} + +// ── nullability ───────────────────────────────────────────────────────────── + +#[test] +fn test_set_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.big_and_flag + operator: set + "}); + assert_filter(&result, &format!("({AND_MEMBER} IS NOT NULL)"), &[]); +} + +#[test] +fn test_not_set_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.big_or_flag + operator: notSet + "}); + assert_filter( + &result, + r#"(("orders".amount > 50 OR "orders".flag) IS NULL)"#, + &[], + ); +} + +// ── comparison ────────────────────────────────────────────────────────────── + +#[test] +fn test_gt_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.amount_plus + operator: gt + values: + - \"10\" + "}); + assert_filter( + &result, + &format!("({NUM_MEMBER} > $_0_$::numeric)"), + &["10"], + ); +} + +#[test] +fn test_lte_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.amount_plus + operator: lte + values: + - \"10\" + "}); + assert_filter( + &result, + &format!("({NUM_MEMBER} <= $_0_$::numeric)"), + &["10"], + ); +} + +// ── like ──────────────────────────────────────────────────────────────────── + +#[test] +fn test_contains_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.note_tagged + operator: contains + values: + - alpha + "}); + assert_filter( + &result, + &format!("(({STR_MEMBER} ILIKE '%' || $_0_$|| '%'))"), + &["alpha"], + ); +} + +#[test] +fn test_starts_with_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.note_tagged + operator: startsWith + values: + - alpha + "}); + assert_filter( + &result, + &format!("(({STR_MEMBER} ILIKE $_0_$|| '%'))"), + &["alpha"], + ); +} + +// ── date ──────────────────────────────────────────────────────────────────── + +#[test] +fn test_in_date_range_compound_member() { + let result = build(indoc! {" + filters: + - dimension: orders.shifted_at + operator: inDateRange + values: + - '2024-01-01T00:00:00.000' + - '2024-01-31T23:59:59.999' + "}); + assert_filter( + &result, + &format!("({TS_MEMBER} >= $_0_$::timestamptz AND {TS_MEMBER} <= $_1_$::timestamptz)"), + &["2024-01-01T00:00:00.000", "2024-01-31T23:59:59.999"], + ); +} + +// ── HAVING: measures ──────────────────────────────────────────────────────── + +// The reported model: unparenthesized this is `sum(...) IS NOT NULL = ...`, +// which Trino and Athena reject. +#[test] +fn test_equals_compound_measure() { + let result = build(indoc! {" + filters: + - member: orders.total_is_set + operator: equals + values: + - \"true\" + "}); + assert_filter( + &result, + r#"((sum("orders".amount) IS NOT NULL) = $_0_$::boolean)"#, + &["true"], + ); +} + +#[test] +fn test_equals_comparison_measure() { + let result = build(indoc! {" + filters: + - member: orders.total_over_150 + operator: equals + values: + - \"true\" + "}); + assert_filter( + &result, + r#"((sum("orders".amount) > 150) = $_0_$::boolean)"#, + &["true"], + ); +} + +// ── members that must NOT be wrapped ──────────────────────────────────────── + +#[test] +fn test_atomic_dimension_is_not_wrapped() { + let result = build(indoc! {" + filters: + - dimension: orders.amount + operator: equals + values: + - \"100\" + "}); + assert_filter(&result, r#"("orders".amount = $_0_$::numeric)"#, &["100"]); +} + +#[test] +fn test_aggregate_measure_is_not_wrapped() { + let result = build(indoc! {" + filters: + - member: orders.total + operator: gt + values: + - \"100\" + "}); + assert_filter( + &result, + r#"(sum("orders".amount) > $_0_$::numeric)"#, + &["100"], + ); +} + +// ── trailing line comment ─────────────────────────────────────────────────── + +// The closing parenthesis, not precedence, is what the comment threatens, so an +// atomic member needs the wrap too — with the parenthesis on its own line. +#[test] +fn test_atomic_member_ending_in_line_comment() { + let result = build(indoc! {" + filters: + - dimension: orders.amount_commented + operator: gt + values: + - \"50\" + "}); + assert_filter( + &result, + "((\"orders\".amount -- as is\n) > $_0_$::numeric)", + &["50"], + ); +} diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter/mod.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter/mod.rs index debdf0463eebe..3362a5d4c0539 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter/mod.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter/mod.rs @@ -1,3 +1,4 @@ +mod member_operand; mod partition_range; mod to_sql; mod to_sql_timezone; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/filter_member_operand.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/filter_member_operand.rs new file mode 100644 index 0000000000000..52909cb381a55 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/filter_member_operand.rs @@ -0,0 +1,122 @@ +//! A filter template splices the member's rendered SQL next to an operator of +//! its own. Left unparenthesized, a member whose own top-level operator binds +//! weaker re-associates: the filter operator captures only the tail of the +//! member expression. These tests check the rows rather than the SQL, because +//! the dangerous form of the mis-parse is valid SQL over a different row set — +//! the emitted text alone cannot tell that one from the intended reading. The +//! parentheses themselves are pinned per operator in +//! `tests/filter/member_operand.rs`. + +use crate::test_fixtures::cube_bridge::MockSchema; +use crate::test_fixtures::test_utils::TestContext; +use indoc::indoc; + +const SEED: &str = "integration_basic_tables.sql"; + +fn create_context() -> TestContext { + let schema = MockSchema::from_yaml_file("common/integration_basic.yaml"); + TestContext::new(schema).unwrap() +} + +// `is_big_completed` = `amount > 100 AND (status = 'completed')` is false for +// orders 1, 3, 5, 7, 9 → count=5. The mis-parse reads `amount > 100 AND +// ((status = 'completed') = false)`, i.e. big-but-not-completed, which matches +// nothing — valid SQL, silently zero rows. +#[tokio::test(flavor = "multi_thread")] +async fn test_equals_false_on_and_dimension() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.count + filters: + - member: orders.is_big_completed + operator: equals + values: + - "false" + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// `is_ny_or_alice` = `city = 'New York' OR name LIKE 'Alice%'` is NULL only for +// Charlie Brown, whose city is NULL and whose name does not match → customer 3. +// The mis-parse reads `city = 'New York' OR (name LIKE 'Alice%') IS NULL`, whose +// right side is never NULL, so it degenerates to the New York customers. +#[tokio::test(flavor = "multi_thread")] +async fn test_not_set_on_or_dimension() { + let ctx = create_context(); + + let query = indoc! {r#" + dimensions: + - customers.id + filters: + - member: customers.is_ny_or_alice + operator: notSet + order: + - id: customers.id + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// `is_big` = `amount > 100`. Unparenthesized this renders +// `amount > 100 = $1::boolean`, which Postgres rejects outright — comparison +// operators do not associate — so reaching any rows at all is the assertion. +#[tokio::test(flavor = "multi_thread")] +async fn test_equals_true_on_comparison_dimension() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.count + filters: + - member: orders.is_big + operator: equals + values: + - "true" + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// The HAVING counterpart of the reported model: `sum(...) IS NOT NULL` compared +// to a boolean, which is what Trino and Athena reject. Postgres happens to parse +// the unparenthesized form the intended way, so this one cannot tell the two +// apart on rows — it guards the shape against a future regression that would +// reach further than precedence, and the operator matrix in +// `tests/filter/member_operand.rs` is what pins the parentheses here. +#[tokio::test(flavor = "multi_thread")] +async fn test_equals_on_calculated_boolean_measure() { + let ctx = create_context(); + + let query = indoc! {r#" + dimensions: + - orders.status + filters: + - member: orders.total_amount_is_set + operator: equals + values: + - "true" + order: + - id: orders.status + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/mod.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/mod.rs index c2c09b60c98de..6a85e4341d90b 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/mod.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/mod.rs @@ -7,6 +7,7 @@ mod chained_subquery; mod combinations; mod cubestore; mod custom_granularities; +mod filter_member_operand; mod filtered_measures; mod filters_segments; mod joins; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_false_on_and_dimension.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_false_on_and_dimension.snap new file mode 100644 index 0000000000000..23c794675209b --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_false_on_and_dimension.snap @@ -0,0 +1,7 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/filter_member_operand.rs +expression: result +--- +orders__count +------------- +5 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_on_calculated_boolean_measure.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_on_calculated_boolean_measure.snap new file mode 100644 index 0000000000000..49007ad6b3021 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_on_calculated_boolean_measure.snap @@ -0,0 +1,9 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/filter_member_operand.rs +expression: result +--- +orders__status +-------------- +cancelled +completed +pending diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_true_on_comparison_dimension.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_true_on_comparison_dimension.snap new file mode 100644 index 0000000000000..6f751708f8bf1 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__equals_true_on_comparison_dimension.snap @@ -0,0 +1,7 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/filter_member_operand.rs +expression: result +--- +orders__count +------------- +4 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__not_set_on_or_dimension.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__not_set_on_or_dimension.snap new file mode 100644 index 0000000000000..787dd9f9e2f2d --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__filter_member_operand__not_set_on_or_dimension.snap @@ -0,0 +1,7 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/filter_member_operand.rs +expression: result +--- +customers__id +------------- +3