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..3ed1794fab559 --- /dev/null +++ b/packages/cubejs-schema-compiler/test/integration/postgres/filter-member-sql-parens.test.ts @@ -0,0 +1,336 @@ +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. + +// 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(` +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 + + # 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 + + 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 () => { + await expectRows( + byId([{ member: 'orders.big_and_flag', operator: 'equals', values: ['false'] }]), + idRows(2, 3, 4, 6) + ); + }); + + it('notEquals', async () => { + 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 () => { + 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 () => { + await expectRows( + byId([{ member: 'orders.big_or_flag', operator: 'notEquals', values: ['true', 'false'] }]), + idRows(6) + ); + }); + + it('set', async () => { + await expectRows( + byId([{ member: 'orders.big_and_flag', operator: 'set' }]), + idRows(1, 2, 3, 4, 6) + ); + }); + + it('notSet', async () => { + 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 () => { + await expectRows( + byId([{ member: 'orders.big', operator: 'equals', values: ['false'] }]), + idRows(2, 4, 6) + ); + }); + + it('notEquals', async () => { + 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 () => { + 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 () => { + 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 () => { + 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) => { + const [sql] = await buildSql(byId([filter])); + expect(sql).toContain(expected); + }); + }); + + // 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 () => { + 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 () => { + 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..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 @@ -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 { + // 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(); + } + format!("({})", member_sql) + } + pub fn allocate_param(&self, value: &str) -> String { self.query_tools.allocate_param(value) } @@ -218,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)"); + } +} 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/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 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(