[pull] master from cube-js:master

packages/cubejs-backend-native/src/bridge_test_exports.rs

@@ -668,6 +668,7 @@ fn invoke_driver_tools<IT: InnerTypes>(b: &NativeDriverTools<IT>) -> 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()));

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',

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',

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']);
+    });
+  });
+});

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<String, CubeError>; //TODO move to templates
     fn in_db_time_zone(&self, date: String) -> Result<String, CubeError>;
     fn get_allocated_params(&self) -> Result<Vec<String>, 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<bool, CubeError>;
     fn subtract_interval(&self, date: String, interval: String) -> Result<String, CubeError>;
     fn add_interval(&self, date: String, interval: String) -> Result<String, CubeError>;
     fn interval_string(&self, interval: String) -> Result<String, CubeError>;

rust/cube/cubesqlplanner/cubesqlplanner/src/planner/base_query.rs

@@ -87,9 +87,7 @@ impl<IT: InnerTypes> BaseQuery<IT> {
         };
 
         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 {

rust/cube/cubesqlplanner/cubesqlplanner/src/planner/params_allocator.rs

@@ -52,31 +52,34 @@ impl ParamsAllocator {
         let mut param_index_map: HashMap<usize, usize> = HashMap::new();
         let mut error = None;
 
+        let placeholder = |index: usize, error: &mut Option<CubeError>| {
+            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() {

rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_tools.rs

@@ -192,17 +192,19 @@ impl QueryTools {
     pub fn get_allocated_params(&self) -> Vec<FilterValue> {
         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<FilterValue>), 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,
         )
     }

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<bool, CubeError> {
+        self.driver_tools.should_reuse_params()
+    }
+
     pub fn time_stamp_cast(&self, field: String) -> Result<String, CubeError> {
         self.driver_tools.time_stamp_cast(field)
     }