diff --git a/packages/cubejs-backend-native/src/bridge_test_exports.rs b/packages/cubejs-backend-native/src/bridge_test_exports.rs index dd4501b264c5d..82510e7ce86e5 100644 --- a/packages/cubejs-backend-native/src/bridge_test_exports.rs +++ b/packages/cubejs-backend-native/src/bridge_test_exports.rs @@ -668,6 +668,7 @@ fn invoke_driver_tools(b: &NativeDriverTools) -> InvokeResul r.record("date_time_cast", b.date_time_cast(s())); r.record("in_db_time_zone", b.in_db_time_zone(s())); r.record("get_allocated_params", b.get_allocated_params()); + r.record("should_reuse_params", b.should_reuse_params()); r.record("subtract_interval", b.subtract_interval(s(), s())); r.record("add_interval", b.add_interval(s(), s())); r.record("interval_string", b.interval_string(s())); diff --git a/packages/cubejs-backend-native/test/bridge/bridge-fixtures.ts b/packages/cubejs-backend-native/test/bridge/bridge-fixtures.ts index 3828f518d4186..872c8adc65adb 100644 --- a/packages/cubejs-backend-native/test/bridge/bridge-fixtures.ts +++ b/packages/cubejs-backend-native/test/bridge/bridge-fixtures.ts @@ -225,6 +225,7 @@ export const driverToolsFixture = (): unknown => ({ dateTimeCast: () => 'dt', inDbTimeZone: () => 'tz', getAllocatedParams: () => [], + shouldReuseParams: false, subtractInterval: () => 'd', addInterval: () => 'd', intervalString: () => 's', diff --git a/packages/cubejs-backend-native/test/bridge/object-bridges-coverage.test.ts b/packages/cubejs-backend-native/test/bridge/object-bridges-coverage.test.ts index 9a3717ad3a8b1..29db799bfe717 100644 --- a/packages/cubejs-backend-native/test/bridge/object-bridges-coverage.test.ts +++ b/packages/cubejs-backend-native/test/bridge/object-bridges-coverage.test.ts @@ -155,6 +155,7 @@ const BRIDGES: BridgeSpec[] = [ 'in_db_time_zone', 'interval_and_minimal_time_unit', 'interval_string', + 'should_reuse_params', 'sql_templates', 'subtract_interval', 'support_generated_series_for_custom_td', diff --git a/packages/cubejs-schema-compiler/test/unit/positional-params.test.ts b/packages/cubejs-schema-compiler/test/unit/positional-params.test.ts new file mode 100644 index 0000000000000..e20ff044ff27d --- /dev/null +++ b/packages/cubejs-schema-compiler/test/unit/positional-params.test.ts @@ -0,0 +1,140 @@ +import fs from 'fs'; +import path from 'path'; +import { BigqueryQuery } from '../../src/adapter/BigqueryQuery'; +import { PostgresQuery } from '../../src/adapter/PostgresQuery'; +import { prepareJsCompiler } from './PrepareCompiler'; + +// `?` placeholders are positional: a value referenced from two places in the +// generated SQL needs one entry in the params array per placeholder. A security +// context value used twice inside a single member's SQL is the shortest way to +// get such a repeated reference — the value is recorded once and the same +// placeholder is spliced at both occurrences. +const model = [ + 'cube(\'orders\', {', + // eslint-disable-next-line no-template-curly-in-string + ' sql: `SELECT * FROM orders WHERE ${SECURITY_CONTEXT.tenantId.filter(t => `(tenant_id = ${t} OR parent_tenant_id = ${t})`)}`,', + ' measures: {', + ' count: {', + ' type: `count`', + ' }', + ' },', + ' dimensions: {', + ' id: {', + ' sql: `id`,', + ' type: `number`,', + ' primaryKey: true', + ' },', + ' createdAt: {', + ' sql: `created_at`,', + ' type: `time`', + ' }', + ' },', + ' preAggregations: {', + ' main: {', + ' measures: [CUBE.count],', + ' timeDimension: CUBE.createdAt,', + ' granularity: `day`,', + ' partitionGranularity: `month`', + ' }', + ' }', + '});', +].join('\n'); + +async function queryFor(QueryClass, useNativeSqlPlanner: boolean, options = {}) { + const { compiler, joinGraph, cubeEvaluator } = prepareJsCompiler(model); + await compiler.compile(); + + return new QueryClass({ joinGraph, cubeEvaluator, compiler }, { + measures: ['orders.count'], + timezone: 'UTC', + contextSymbols: { + securityContext: { tenantId: 'acme' }, + }, + useNativeSqlPlanner, + ...options, + }); +} + +function bigQueryFor(useNativeSqlPlanner: boolean, options = {}) { + return queryFor(BigqueryQuery, useNativeSqlPlanner, options); +} + +function placeholdersCount(sql: string) { + return (sql.match(/\?/g) || []).length; +} + +// Read off the directory rather than listed by hand, so a dialect added later +// cannot quietly escape the invariant below. +const ADAPTER_DIR = path.join(__dirname, '..', '..', 'src', 'adapter'); + +function allDialects() { + const classes = fs.readdirSync(ADAPTER_DIR) + // Tests run from `dist`, so the adapter dir holds `.js`; `.ts` keeps this + // working if they are ever run from source. + .map(file => file.match(/^(\w+Query)\.(?:ts|js)$/)?.[1]) + .filter((name): name is string => !!name && name !== 'BaseQuery') + // eslint-disable-next-line global-require, import/no-dynamic-require + .map(name => require(path.join(ADAPTER_DIR, name))[name]); + + expect(classes.length).toBeGreaterThan(10); + return classes; +} + +describe('positional params', () => { + // Dialects whose placeholder carries the param index are free to share a param + // between placeholders; those rendering a bare placeholder are not, since the + // placeholder then says nothing about which value it binds. + it('never reuses params on dialects whose placeholder omits the param index', async () => { + const { compiler, joinGraph, cubeEvaluator } = prepareJsCompiler(model); + await compiler.compile(); + + const reusingPositionalDialects = allDialects().filter(QueryClass => { + const query = new QueryClass({ joinGraph, cubeEvaluator, compiler }, { + measures: ['orders.count'], + timezone: 'UTC', + }); + const indexedPlaceholder = query.sqlTemplates().params.param.includes('param_index'); + + return !indexedPlaceholder && query.shouldReuseParams; + }).map(QueryClass => QueryClass.name); + + expect(reusingPositionalDialects).toEqual([]); + }); + + describe.each([ + ['legacy planner', false], + ['native planner', true], + ])('%s', (_name, useNativeSqlPlanner) => { + it('allocates a param per placeholder in a query', async () => { + const query = await bigQueryFor(useNativeSqlPlanner, { dimensions: ['orders.id'] }); + const [sql, params] = query.buildSqlAndParams(); + + expect(placeholdersCount(sql)).toEqual(params.length); + expect(params).toEqual(['acme', 'acme']); + }); + + it('shares one param between placeholders when the placeholder carries its index', async () => { + const query = await queryFor(PostgresQuery, useNativeSqlPlanner, { dimensions: ['orders.id'] }); + const [sql, params] = query.buildSqlAndParams(); + + // `$1` names the value it binds, so both occurrences can point at it. + expect((sql.match(/\$1\b/g) || []).length).toEqual(2); + expect(params).toEqual(['acme']); + }); + + it('allocates a param per placeholder in a pre-aggregation build query', async () => { + const query = await bigQueryFor(useNativeSqlPlanner, { + timeDimensions: [{ + dimension: 'orders.createdAt', + granularity: 'day', + dateRange: ['2024-01-01', '2024-01-31'], + }], + }); + const [description]: any = query.preAggregations?.preAggregationsDescription(); + const [loadSql, params] = description.loadSql; + + expect(placeholdersCount(loadSql)).toEqual(params.length); + expect(params).toEqual(['acme', 'acme', '__FROM_PARTITION_RANGE', '__TO_PARTITION_RANGE']); + }); + }); +}); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/cube_bridge/driver_tools.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/cube_bridge/driver_tools.rs index 2bb6cd8eff12a..f9e86568c1c38 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/cube_bridge/driver_tools.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/cube_bridge/driver_tools.rs @@ -26,6 +26,13 @@ pub trait DriverTools { fn date_time_cast(&self, field: String) -> Result; //TODO move to templates fn in_db_time_zone(&self, date: String) -> Result; fn get_allocated_params(&self) -> Result, CubeError>; + /// The dialect's own answer to whether one param may back several + /// placeholders. It is an opt-in, not a property of the rendered + /// placeholder: a dialect whose placeholder omits the param index + /// (positional `?`) cannot opt in, while one that carries the index is free + /// to stay opted out and get a param per placeholder instead. + #[nbridge(field)] + fn should_reuse_params(&self) -> Result; fn subtract_interval(&self, date: String, interval: String) -> Result; fn add_interval(&self, date: String, interval: String) -> Result; fn interval_string(&self, interval: String) -> Result; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/base_query.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/base_query.rs index 2e7004b68fa44..f592113e67ab4 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/base_query.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/base_query.rs @@ -87,9 +87,7 @@ impl BaseQuery { }; let templates = self.query_tools.plan_sql_templates(is_external)?; - let (result_sql, params) = self - .query_tools - .build_sql_and_params(&sql, true, &templates)?; + let (result_sql, params) = self.query_tools.build_sql_and_params(&sql, &templates)?; // For single usage, strip __usage_N suffix from SQL to maintain backward compat let final_sql = if usages.len() == 1 { diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/params_allocator.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/params_allocator.rs index 5401a4c0f7ead..db9ba412bde01 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/params_allocator.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/params_allocator.rs @@ -52,31 +52,34 @@ impl ParamsAllocator { let mut param_index_map: HashMap = HashMap::new(); let mut error = None; + let placeholder = |index: usize, error: &mut Option| { + if self.export_annotated_sql { + return format!("${}$", index); + } + match templates.param(index) { + Ok(res) => res, + Err(e) => { + if error.is_none() { + *error = Some(e); + } + "$error$".to_string() + } + } + }; + let result_sql = if should_reuse_params { PARAMS_MATCH_RE .replace_all(&sql, |caps: &Captures| { let ind: usize = caps[1].to_string().parse().unwrap(); let new_index = if let Some(index) = param_index_map.get(&ind) { - index.clone() + *index } else { let index = params_in_sql_order.len(); params_in_sql_order.push(params[ind].clone()); param_index_map.insert(ind, index); index }; - if self.export_annotated_sql { - format!("${}$", new_index) - } else { - match templates.param(new_index) { - Ok(res) => res, - Err(e) => { - if error.is_none() { - error = Some(e); - } - "$error$".to_string() - } - } - } + placeholder(new_index, &mut error) }) .to_string() } else { @@ -85,15 +88,7 @@ impl ParamsAllocator { let ind: usize = caps[1].to_string().parse().unwrap(); let index = params_in_sql_order.len(); params_in_sql_order.push(params[ind].clone()); - match templates.param(index) { - Ok(res) => res, - Err(e) => { - if error.is_none() { - error = Some(e); - } - "$error$".to_string() - } - } + placeholder(index, &mut error) }) .to_string() }; @@ -139,6 +134,84 @@ impl ParamsAllocator { #[cfg(test)] mod tests { use super::*; + use crate::test_fixtures::cube_bridge::{MockDriverTools, MockSqlTemplatesRender}; + use std::rc::Rc; + + /// Dialect rendering params as positional `?`, i.e. everything but Postgres. + fn positional_templates() -> PlanSqlTemplates { + let driver_tools = MockDriverTools::with_sql_templates( + MockSqlTemplatesRender::default_templates_with_positional_params(), + ) + .without_params_reuse(); + PlanSqlTemplates::try_new(Rc::new(driver_tools), false).unwrap() + } + + fn allocator_with_two_params(export_annotated_sql: bool) -> ParamsAllocator { + let mut allocator = ParamsAllocator::new(export_annotated_sql); + allocator.allocate_param("alpha"); + allocator.allocate_param("beta"); + allocator + } + + #[test] + fn positional_params_render_one_placeholder_per_param() { + let allocator = allocator_with_two_params(false); + + let (sql, params) = allocator + .build_sql_and_params( + "SELECT $_0_$, $_0_$, $_1_$", + vec![], + false, + &positional_templates(), + ) + .unwrap(); + + assert_eq!(sql, "SELECT ?, ?, ?"); + assert_eq!( + params, + vec![ + FilterValue::Str("alpha".to_string()), + FilterValue::Str("alpha".to_string()), + FilterValue::Str("beta".to_string()), + ] + ); + } + + #[test] + fn annotated_sql_keeps_placeholders_unrendered_without_reuse() { + let allocator = allocator_with_two_params(true); + + // The SQL API asks for annotated SQL so that cubesql can re-allocate the + // params itself; the dialect placeholder must not be rendered here. + let (sql, params) = allocator + .build_sql_and_params( + "SELECT $_0_$, $_0_$, $_1_$", + vec![], + false, + &positional_templates(), + ) + .unwrap(); + + assert_eq!(sql, "SELECT $0$, $1$, $2$"); + assert_eq!(params.len(), 3); + } + + #[test] + fn annotated_sql_keeps_placeholders_unrendered_with_reuse() { + let allocator = allocator_with_two_params(true); + + let (sql, params) = allocator + .build_sql_and_params( + "SELECT $_0_$, $_0_$, $_1_$", + vec![], + true, + &positional_templates(), + ) + .unwrap(); + + assert_eq!(sql, "SELECT $0$, $0$, $1$"); + assert_eq!(params.len(), 2); + } #[test] fn allocated_params_enter_channel_as_str() { diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_tools.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_tools.rs index 42853bcb2dfff..d8d62ad8c365b 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_tools.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_tools.rs @@ -192,17 +192,19 @@ impl QueryTools { pub fn get_allocated_params(&self) -> Vec { self.params_allocator.borrow().get_params().clone() } + /// Resolves param placeholders for the dialect the SQL is rendered for. + /// Params may only be shared between placeholders when the dialect's + /// placeholder addresses its value by index. pub fn build_sql_and_params( &self, sql: &str, - should_reuse_params: bool, templates: &PlanSqlTemplates, ) -> Result<(String, Vec), CubeError> { let native_allocated_params = self.base_tools.get_allocated_params()?; self.params_allocator.borrow().build_sql_and_params( sql, native_allocated_params, - should_reuse_params, + templates.should_reuse_params()?, templates, ) } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs index 0f31f1590f1a5..c671d13f51af0 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs @@ -88,6 +88,10 @@ impl PlanSqlTemplates { self.driver_tools.timestamp_precision() } + pub fn should_reuse_params(&self) -> Result { + self.driver_tools.should_reuse_params() + } + pub fn time_stamp_cast(&self, field: String) -> Result { self.driver_tools.time_stamp_cast(field) } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_driver_tools.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_driver_tools.rs index 1bfc5637f9b26..ebeff4e1786b4 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_driver_tools.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_driver_tools.rs @@ -21,6 +21,9 @@ pub struct MockDriverTools { /// When set, dialect-specific native methods mirror CubeStoreQuery /// instead of the Postgres defaults (used for external pre-aggregations). cubestore: bool, + /// Mirrors the JS `shouldReuseParams` dialect flag: `true` for the Postgres + /// defaults (`$1` addresses its value), `false` for positional `?`. + should_reuse_params: bool, } impl MockDriverTools { @@ -31,6 +34,7 @@ impl MockDriverTools { sql_templates: Rc::new(MockSqlTemplatesRender::default_templates()), visible_in_db_time_zone: false, cubestore: false, + should_reuse_params: true, } } @@ -42,6 +46,7 @@ impl MockDriverTools { sql_templates: Rc::new(MockSqlTemplatesRender::default_templates()), visible_in_db_time_zone: false, cubestore: false, + should_reuse_params: true, } } @@ -53,6 +58,7 @@ impl MockDriverTools { sql_templates: Rc::new(sql_templates), visible_in_db_time_zone: false, cubestore: false, + should_reuse_params: true, } } @@ -66,9 +72,18 @@ impl MockDriverTools { sql_templates: Rc::new(sql_templates), visible_in_db_time_zone: false, cubestore: false, + should_reuse_params: true, } } + /// Renders params as positional `?`, so a value cannot be shared between + /// placeholders. + #[allow(dead_code)] + pub fn without_params_reuse(mut self) -> Self { + self.should_reuse_params = false; + self + } + #[allow(dead_code)] pub fn with_visible_in_db_time_zone(mut self) -> Self { self.visible_in_db_time_zone = true; @@ -205,6 +220,10 @@ impl DriverTools for MockDriverTools { Ok(Vec::new()) } + fn should_reuse_params(&self) -> Result { + Ok(self.should_reuse_params) + } + fn subtract_interval(&self, date: String, interval: String) -> Result { let interval_str = self.interval_string(interval)?; Ok(format!("{} - interval {}", date, interval_str)) diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_member_sql.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_member_sql.rs index 95fb8fb92a975..8afd5dd575806 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_member_sql.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_member_sql.rs @@ -139,6 +139,15 @@ impl MockMemberSql { ))); } + // `{SECURITY_VALUE:}` records a security context value and + // yields `{sv:N}`. Equal values collapse to one index, the way the + // JS member-sql compiler dedups them. + if let Some(value) = path.strip_prefix("SECURITY_VALUE:") { + let index = args.insert_security_context_value(value.to_string()); + result.push_str(&format!("{{sv:{}}}", index)); + continue; + } + // Parse the path and add to symbol_paths let path_parts: Vec = path.split('.').map(|s| s.to_string()).collect(); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_sql_templates_render.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_sql_templates_render.rs index 5ee13ed510e83..9ba7235168279 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_sql_templates_render.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_sql_templates_render.rs @@ -27,6 +27,11 @@ impl MockSqlTemplatesRender { } pub fn default_templates() -> Self { + Self::try_new(Self::default_templates_map()) + .expect("Default templates should always parse successfully") + } + + fn default_templates_map() -> HashMap { let mut templates = HashMap::new(); // Functions - based on BaseQuery.js:4241-4315 @@ -536,7 +541,16 @@ impl MockSqlTemplatesRender { "{% if original_sql %}{{ original_sql }}\n{% endif %}{% for group in groups %}{% if original_sql or not loop.first %}CROSS JOIN\n{% endif %}(\n{% for value in group.values %}SELECT {{ value }} as {{ group.name }}{% if not loop.last %} UNION ALL\n{% endif %}{% endfor %}) AS {{ group.alias }}\n{% endfor %}".to_string(), ); - Self::try_new(templates).expect("Default templates should always parse successfully") + templates + } + + /// Postgres defaults with positional `?` params instead of `$1`, mirroring + /// BigQuery, Snowflake, MySQL and the other `?` dialects. + pub fn default_templates_with_positional_params() -> Self { + let mut templates = Self::default_templates_map(); + templates.insert("params/param".to_string(), "?".to_string()); + Self::try_new(templates) + .expect("Positional params templates should always parse successfully") } pub fn default_templates_with_generated_time_series() -> Self { @@ -588,8 +602,9 @@ impl MockSqlTemplatesRender { /// Templates matching `CubeStoreQuery.sqlTemplates()` from the JS /// schema-compiler: BaseQuery defaults plus CubeStore-specific overrides. pub fn cubestore_templates() -> Self { - let render = Self::default_templates(); - let mut templates = render.templates; + let mut templates = Self::default_templates_map(); + // CubeStoreQuery keeps the base dialect's positional `?` params. + templates.insert("params/param".to_string(), "?".to_string()); templates.insert( "statements/time_series_select".to_string(), concat!( diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/test_utils/test_context.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/test_utils/test_context.rs index 26e5afe1b8c60..ff0e4f4a2e2ee 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/test_utils/test_context.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/test_utils/test_context.rs @@ -38,6 +38,9 @@ pub struct TestContext { /// dialect templates, so queries fully covered by external /// pre-aggregations render CubeStore SQL. external_cubestore: bool, + /// Mirrors the dialect's `shouldReuseParams`. Carried through `for_options` + /// rebuilds together with `custom_sql_templates`. + should_reuse_params: bool, } impl TestContext { @@ -64,6 +67,7 @@ impl TestContext { false, self.custom_sql_templates.clone(), true, + self.should_reuse_params, ) } @@ -72,10 +76,14 @@ impl TestContext { schema: MockSchema, base_tools: MockBaseTools, ) -> Result { + use crate::cube_bridge::base_tools::BaseTools; let join_graph = Rc::new(schema.create_join_graph()?); let evaluator = schema.clone().create_evaluator(); let security_context: Rc = Rc::new(MockSecurityContext); + // Taken from the tools the caller built, so a `for_options` rebuild can't + // silently disagree with them. + let should_reuse_params = base_tools.driver_tools(false)?.should_reuse_params()?; let query_tools = State::try_new( evaluator, @@ -95,18 +103,48 @@ impl TestContext { security_context, custom_sql_templates: None, external_cubestore: false, + should_reuse_params, }) } #[allow(dead_code)] pub fn new_with_generated_time_series(schema: MockSchema) -> Result { - use crate::test_fixtures::cube_bridge::{MockDriverTools, MockSqlTemplatesRender}; - let sql_templates = MockSqlTemplatesRender::default_templates_with_generated_time_series(); - let driver_tools = MockDriverTools::with_sql_templates(sql_templates.clone()); - let base_tools = schema.create_base_tools_with_driver(driver_tools)?; - let mut ctx = Self::new_with_base_tools(schema, base_tools)?; - ctx.custom_sql_templates = Some(sql_templates); - Ok(ctx) + use crate::test_fixtures::cube_bridge::MockSqlTemplatesRender; + Self::new_with_custom_templates( + schema, + MockSqlTemplatesRender::default_templates_with_generated_time_series(), + true, + ) + } + + /// Context rendering params as positional `?` (BigQuery, Snowflake, MySQL, ...) + /// instead of the Postgres `$1` default. + #[allow(dead_code)] + pub fn new_with_positional_params(schema: MockSchema) -> Result { + use crate::test_fixtures::cube_bridge::MockSqlTemplatesRender; + Self::new_with_custom_templates( + schema, + MockSqlTemplatesRender::default_templates_with_positional_params(), + false, + ) + } + + fn new_with_custom_templates( + schema: MockSchema, + sql_templates: crate::test_fixtures::cube_bridge::MockSqlTemplatesRender, + should_reuse_params: bool, + ) -> Result { + Self::new_with_options_internal_ext( + schema, + Tz::UTC, + None, + None, + false, + false, + Some(sql_templates), + false, + should_reuse_params, + ) } #[allow(dead_code)] @@ -154,6 +192,7 @@ impl TestContext { .unwrap_or(false), self.custom_sql_templates.clone(), self.external_cubestore, + self.should_reuse_params, ) } @@ -174,6 +213,7 @@ impl TestContext { convert_tz_for_raw_time_dimension, None, false, + true, ) } @@ -187,22 +227,28 @@ impl TestContext { convert_tz_for_raw_time_dimension: bool, custom_sql_templates: Option, external_cubestore: bool, + should_reuse_params: bool, ) -> Result { use crate::test_fixtures::cube_bridge::MockDriverTools; - let mut base_tools = if let Some(templates) = custom_sql_templates.clone() { - let driver_tools = - MockDriverTools::with_sql_templates_and_timezone(templates, timezone.to_string()); - schema.create_base_tools_with_driver(driver_tools)? - } else { - schema.create_base_tools_with_timezone(timezone.to_string())? + let mut driver_tools = match custom_sql_templates.clone() { + Some(templates) => { + MockDriverTools::with_sql_templates_and_timezone(templates, timezone.to_string()) + } + None => MockDriverTools::with_timezone(timezone.to_string()), }; + if !should_reuse_params { + driver_tools = driver_tools.without_params_reuse(); + } + let mut base_tools = schema.create_base_tools_with_driver(driver_tools)?; if external_cubestore { use crate::test_fixtures::cube_bridge::MockSqlTemplatesRender; let driver_tools = MockDriverTools::with_sql_templates_and_timezone( MockSqlTemplatesRender::cubestore_templates(), timezone.to_string(), ) - .with_cubestore_dialect(); + .with_cubestore_dialect() + // CubeStoreQuery renders positional `?`, so it never reuses params. + .without_params_reuse(); base_tools.set_external_driver_tools(Rc::new(driver_tools)); } let join_graph = Rc::new(schema.create_join_graph()?); @@ -228,6 +274,7 @@ impl TestContext { security_context, custom_sql_templates, external_cubestore, + should_reuse_params, }) } @@ -521,6 +568,24 @@ impl TestContext { Ok(sql) } + /// Renders the query the way `BaseQuery::build_sql_and_params` does: plan, + /// then resolve param placeholders with the dialect the SQL is rendered for. + #[allow(dead_code)] + pub fn build_sql_and_params( + &self, + query: &str, + ) -> Result<(String, Vec), CubeError> { + let options = self.create_query_options_from_yaml(query); + let ctx = self.for_options(options.as_ref())?; + let request = QueryPropertiesCompiler::new(ctx.query_tools.clone()).build(options)?; + let planner = TopLevelPlanner::new(request, ctx.query_tools.clone(), true); + let (raw_sql, usages) = planner.plan()?; + + let is_external = !usages.is_empty() && usages.iter().all(|u| u.pre_aggregation.external()); + let templates = ctx.query_tools.plan_sql_templates(is_external)?; + ctx.query_tools.build_sql_and_params(&raw_sql, &templates) + } + #[allow(dead_code)] pub fn build_sql_from_options( &self, @@ -627,7 +692,7 @@ impl TestContext { .expect("Failed to get SQL templates"); let (sql, params) = ctx .query_tools - .build_sql_and_params(&raw_sql, true, &templates) + .build_sql_and_params(&raw_sql, &templates) .expect("Failed to build SQL and params"); // Strip __usage_N suffixes from SQL, same as base_query.rs does for single usage @@ -752,7 +817,7 @@ impl TestContext { .expect("Failed to get SQL templates"); let (sql, params) = pa_ctx .query_tools - .build_sql_and_params(&raw_sql, true, &templates) + .build_sql_and_params(&raw_sql, &templates) .expect("Failed to build pre-agg SQL and params"); Self::inline_params(&sql, ¶ms) } @@ -949,7 +1014,7 @@ impl TestContext { .expect("Failed to get SQL templates"); let (sql, params) = ctx .query_tools - .build_sql_and_params(&raw_sql, true, &templates) + .build_sql_and_params(&raw_sql, &templates) .expect("Failed to build SQL and params"); let sql = pre_aggregations @@ -1311,19 +1376,77 @@ impl TestContext { } } - #[cfg(feature = "integration-postgres")] + /// Inlines params as literals so the SQL can run without a bind protocol. + /// Both placeholder forms the dialects render are handled: indexed `$N`, and + /// positional `?` consumed in occurrence order. + /// + /// Placeholders are recognized anywhere in the text, so SQL carrying `?` or + /// `$1` inside a string literal would be rewritten — no fixture does that, + /// and the values inlined here are never rescanned. fn inline_params(sql: &str, params: &[FilterValue]) -> String { - let mut result = sql.to_string(); - for (i, param) in params.iter().enumerate().rev() { - let placeholder = format!("${}", i + 1); - // `Null` must inline as the bare SQL keyword; every other variant is - // rendered through its canonical string form and quoted. - let literal = match param.to_param_string() { - Some(value) => format!("'{}'", value.replace('\'', "''")), - None => "NULL".to_string(), - }; - result = result.replace(&placeholder, &literal); + // `Null` must inline as the bare SQL keyword; every other variant is + // rendered through its canonical string form and quoted. + let literal = |param: &FilterValue| match param.to_param_string() { + Some(value) => format!("'{}'", value.replace('\'', "''")), + None => "NULL".to_string(), + }; + let at = |index: usize| { + params.get(index).unwrap_or_else(|| { + panic!( + "Placeholder refers to param {} but only {} were built:\n{}", + index + 1, + params.len(), + sql + ) + }) + }; + + let mut result = String::with_capacity(sql.len()); + let mut next_positional = 0; + let mut chars = sql.chars().peekable(); + + while let Some(ch) = chars.next() { + match ch { + '?' => { + result.push_str(&literal(at(next_positional))); + next_positional += 1; + } + '$' if chars.peek().is_some_and(|c| c.is_ascii_digit()) => { + let mut index = String::new(); + while let Some(c) = chars.peek() { + if !c.is_ascii_digit() { + break; + } + index.push(*c); + chars.next(); + } + let index: usize = index.parse().expect("digits only"); + // Dialect placeholders are 1-based. `$0` means this is + // annotated SQL (`$0$`), whose indexes live in another space + // and must not be inlined as if they were dialect params. + let index = index.checked_sub(1).unwrap_or_else(|| { + panic!("Annotated SQL cannot be inlined, got `$0` in:\n{}", sql) + }); + result.push_str(&literal(at(index))); + } + _ => result.push(ch), + } } + + // Every placeholder form must be consumed above. MSSQL's `@_N` has no + // fixture driving it yet, so catch it here instead of letting it reach + // the database as text. + if let Some(leftover) = regex::Regex::new(r"@_\d+") + .expect("Failed to build placeholder regex") + .find(&result) + { + panic!( + "Placeholder {} was left unresolved in:\n{}", + leftover.as_str(), + result + ); + } + result } @@ -1387,6 +1510,44 @@ mod tests { use super::*; use crate::test_fixtures::cube_bridge::MockSchema; + #[test] + fn inline_params_handles_both_placeholder_forms() { + let params = vec![ + FilterValue::Str("a'b".to_string()), + FilterValue::Null, + FilterValue::Num(42.0), + ]; + + assert_eq!( + TestContext::inline_params("SELECT $1, $2, $3, $1", ¶ms), + "SELECT 'a''b', NULL, '42', 'a''b'" + ); + // Positional placeholders bind by occurrence, so a value repeated in the + // SQL is repeated in the params. + assert_eq!( + TestContext::inline_params("SELECT ?, ?, ?", ¶ms), + "SELECT 'a''b', NULL, '42'" + ); + } + + #[test] + #[should_panic(expected = "Placeholder refers to param 2 but only 1 were built")] + fn inline_params_rejects_more_placeholders_than_params() { + TestContext::inline_params("SELECT ?, ?", &[FilterValue::Str("a".to_string())]); + } + + #[test] + #[should_panic(expected = "Annotated SQL cannot be inlined")] + fn inline_params_rejects_annotated_sql() { + TestContext::inline_params("SELECT $0$", &[FilterValue::Str("a".to_string())]); + } + + #[test] + #[should_panic(expected = "Placeholder @_1 was left unresolved")] + fn inline_params_rejects_unknown_placeholder_form() { + TestContext::inline_params("SELECT @_1", &[FilterValue::Str("a".to_string())]); + } + #[test] fn test_yaml_filter_parsing() { use indoc::indoc; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/mod.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/mod.rs index fdb0df6ccf57b..61fcbbf3a235b 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/mod.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/mod.rs @@ -10,6 +10,7 @@ mod join_hints_collector; mod measure_symbol; mod member_expressions_on_views; mod no_query_tools_leak; +mod positional_params; mod string_measures; mod subquery_dimensions; mod utils; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/positional_params.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/positional_params.rs new file mode 100644 index 0000000000000..072ba003256f6 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/positional_params.rs @@ -0,0 +1,112 @@ +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; + +/// The cube SQL reads one security context value from two places. Equal values +/// collapse to a single recorded value, so the planner splices the same param +/// placeholder at both occurrences. +fn schema() -> MockSchema { + MockSchema::from_yaml(indoc! {" + cubes: + - name: orders + sql: \"SELECT * FROM orders WHERE tenant_id = {SECURITY_VALUE:acme} OR parent_tenant_id = {SECURITY_VALUE:acme}\" + dimensions: + - name: id + type: number + sql: id + primary_key: true + measures: + - name: count + type: count + "}) + .unwrap() +} + +const QUERY: &str = indoc! {" + measures: + - orders.count +"}; + +#[test] +fn positional_params_get_one_param_per_placeholder() { + let ctx = TestContext::new_with_positional_params(schema()).unwrap(); + + let (sql, params) = ctx.build_sql_and_params(QUERY).unwrap(); + + // `?` carries no index, so both placeholders need their own value. + assert_eq!( + sql.matches('?').count(), + params.len(), + "sql: {}\nparams: {:?}", + sql, + params + ); + assert_eq!( + params, + vec![ + FilterValue::Str("acme".to_string()), + FilterValue::Str("acme".to_string()) + ] + ); +} + +#[test] +fn indexed_params_are_reused_across_placeholders() { + let ctx = TestContext::new(schema()).unwrap(); + + let (sql, params) = ctx.build_sql_and_params(QUERY).unwrap(); + + // `$1` addresses its value, so both occurrences share one param — and stay + // textually equal, which Postgres requires from repeated expressions. + assert_eq!(sql.matches("$1").count(), 2, "sql: {}", sql); + assert_eq!(params, vec![FilterValue::Str("acme".to_string())]); +} + +/// A query served from an external pre-aggregation is rendered with the CubeStore +/// dialect, so params must follow *its* placeholder form — CubeStore accepts only +/// positional `?`, consumed one per occurrence, whether they are bound over the +/// WS protocol or inlined by the driver. +#[test] +fn external_pre_aggregation_renders_params_with_the_cubestore_dialect() { + let ctx = TestContext::new_with_external_cubestore(MockSchema::from_yaml_file( + "common/integration_cubestore_basic.yaml", + )) + .unwrap(); + + let query = indoc! {" + measures: + - visitors.count + dimensions: + - visitors.source + filters: + - dimension: visitors.source + operator: equals + values: + - some + "}; + + let (_, pre_aggregations) = ctx.build_sql_with_used_pre_aggregations(query).unwrap(); + assert!( + pre_aggregations + .iter() + .all(|u| u.pre_aggregation.external()), + "the query must be served from an external pre-aggregation" + ); + + let (sql, params) = ctx.build_sql_and_params(query).unwrap(); + + assert!( + !sql.contains("$1"), + "params must not render in the source dialect's indexed form\nsql: {}", + sql + ); + assert_eq!( + sql.matches('?').count(), + params.len(), + "sql: {}\nparams: {:?}", + sql, + params + ); + assert_eq!(params, vec![FilterValue::Str("some".to_string())]); +}