From f5b2533d4b58274c353ffcda6494a5037f742375 Mon Sep 17 00:00:00 2001 From: waralexrom <108349432+waralexrom@users.noreply.github.com> Date: Fri, 7 Aug 2026 11:12:28 +0200 Subject: [PATCH 1/3] fix(tesseract): resolve FILTER_PARAMS column callback references (#11460) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(tesseract): cover FILTER_PARAMS column callbacks referencing members A `FILTER_PARAMS.….filter(cb)` column callback is invoked at render time, and the member references inside it have to resolve to that member's SQL — the same as when the column is handed over as a template string. The two `native planner` cases fail with `Placeholder {arg:0} out of bounds` and stay red until the planner resolves those references; the `legacy planner` cases pin the SQL that the fix has to produce. Also adds FILTER_PARAMS support to the Rust mock member templates — `{FILTER_PARAMS:.:}`, where `[path]` inside the column is a member reference and `%N` is the Nth filter value — plus a planner-level test that a callback column's placeholder is resolved against the dependency list. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): compile a FILTER_PARAMS column callback into a call of its own A column callback is a SQL function in its own right: it has its own body, its own member references and its own parameters. It was handed to the planner as an opaque JS function and invoked at render time, at which point the references it touched were recorded into a dependency list the planner had already read — so the placeholders it emitted indexed nothing and rendering failed with `Placeholder {arg:0} out of bounds`. The callback is now compiled like any other member sql, with its declared parameters bound to `{fpv:N}` value placeholders, and becomes a `SqlCall` with its own template, dependencies and parenthesisation contexts. Rendering substitutes the filter values into `{fpv:N}` and its own dependencies into `{arg:N}`, so a reference resolves the same whichever way it is spelled, and a compound member is parenthesised according to the context it lands in. Two places keep the callback and render it as-is: a cube's own `sql`, which is the innermost FROM and has no member in scope to resolve against, and a callback taking its values through a rest parameter, whose count only the query knows. A column renders only when its filter reaches the query, so the members it reads are not dependencies of the enclosing member and cannot pull a cube into the join. Reading outside the owning cube is therefore reported when the column renders — leaving queries that never use that filter unaffected. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): reject member references in a cube's own sql A cube's `sql` builds the table the query reads from, so nothing a member reference could resolve against is in scope inside it. Resolving one anyway recursed between the cube table and the member until the stack ran out; the legacy planner does not recurse but emits a qualifier for a table that is not in the query, which the database rejects. All three spellings — a direct `${CUBE.dimension}`, a string `FILTER_PARAMS` column and a `FILTER_PARAMS` column callback — now report the reference instead. The path is classified without building any symbol, so the report replaces the recursion rather than following it. A column callback itself stays allowed, and so does everything in it that needs no member in scope: the filter values it takes and any security context value, which becomes a query param. That keeps the row-level-security shape working, as well as a reference to another cube's `sql`, which resolves to that cube's own table expression. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): bind a compiled FILTER_PARAMS column only to values it can take A compiled column carries one value placeholder per parameter its callback declares, while a filter supplies as many values as its operator has. The two were never compared, so an operator carrying fewer values left trailing placeholders unbound and the query failed. A filter carrying no values now applies nothing, which is what `set` or `notSet` on the filtered member amounts to and what the legacy planner does. Fewer values than the column takes is reported: the legacy planner fills the missing bound in with the current time, quietly widening the predicate to a range the filter never asked for, which is worse than saying so. Three further holes the same reading turned up: - A cube named directly in a callback passed the own-cube check, since only member dependencies were examined and cube references were dropped. It now renders no qualifier for a table the query does not read. A cube's table expression stays exempt — it inlines the whole expression and needs no join. - The parameter list is read out of the callback's source, which comes up short for a bound or native function and for a `)` inside a comment or a string default. Too few placeholders would have rendered the missing values as `undefined`, so a count that cannot account for every parameter now leaves the callback to render time instead. - A compiled column no longer silently ignores the time shift the surrounding query applies, which would restrict a shifted CTE to the current period and empty it. The cube reported by the own-cube check is picked deterministically; the names arrive unordered. Co-Authored-By: Claude Opus 5 (1M context) * feat(tesseract): let an active FILTER_PARAMS column contribute its dependencies A column renders only where its filter reaches the query, so the members it reads belong to the enclosing member's dependencies exactly there. Marking each binding by whether the query filters the member it names lets an active column's dependencies flow into join hints, which is what puts the cube it reads into the join — and keeps them out everywhere else, so a query that never uses the filter plans as though the column were not there. That removes the restriction to the owning cube: a column may read whatever its filter's arrival justifies joining. When the join cannot be built the join graph says so on its own, in the same terms as for any other reference — row multiplication for a hasMany, no join path when there is none — so the restriction needed no replacement. Activity is settled per query and again per multi-stage subquery, since `filter: include` lets a subquery filter a member the query around it does not. It starts off, so a symbol compiled for comparison rather than for a query carries no dependency the query would not — a pre-aggregation is matched against the same shape either way. Rewrites reach into an inactive column all the same, so it never keeps a symbol every other reference to it has replaced. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): settle FILTER_PARAMS activity for every symbol that renders one A column renders wherever the symbol carrying it does, and a query reaches symbols through more than its selected members. Activity was settled for dimensions, time dimensions and measures only, so a column inside a segment, a dimension named only in a filter, or a measure named only in a having filter rendered while contributing nothing — and the cube it reads went unjoined, leaving a qualifier with no table behind it. Filters, segments and order items are now settled too, the way case pruning already covered them. A `FILTER_GROUP` renders as one predicate, matched against every member it names, while activity was decided per member. Since partial matching applies to AND groups only, an OR group could match as a whole while a single member of it did not, so the group rendered and none of its members contributed. Activity for a group is now decided from the group's whole member list, which is the same question the render asks — the two can no longer disagree. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): trust a column callback's parameter count only when something vouches for it `Function.length` was the witness that the parameter list had been read in full, but it drops to zero at the first defaulted parameter and can vouch for nothing after that — so a `)` inside a string default of the first parameter closed the list early, the count came back short, and the trailing value rendered as the literal `undefined`. Where `Function.length` cannot speak, the text itself does: a list holding neither a string nor a comment has nothing for the scan to trip over. Also builds the filter set matched against once per pass instead of once per symbol, and records on the pass itself where a multi-stage stage settles activity against the wider set it may filter. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): stop trusting a parameter count past the first default `Function.length` counts the parameters before the first defaulted one, so it catches a scan that stopped short of that point and nothing after it. Reading it as a witness for the whole list left a `)` inside any later default closing the scan early, the count coming back short, and the trailing value rendering as the literal `undefined`. Past the first default only the text can vouch, so the check now applies whatever `Function.length` says: a parameter list holding a string or a comment holds the callback back to the render-time path, where every list that cannot be read in full already goes. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): apply a FILTER_PARAMS column only as far as its filter reaches A column applies what its filter supplies, and nothing when the filter cannot supply what the column takes. That already covered an operator carrying no values; it now covers one carrying fewer than the column declares — a one-sided date operator against a column taking both bounds — instead of reporting it. The filter still reaches the query on its own, so only its restatement inside the member's SQL is dropped. That is narrower than binding a bound the filter never gave: the legacy planner fills the missing one in with the current time, which for `beforeDate` lands the given value on the opposite side of the range and leaves the measure empty for every row the query keeps, and makes the SQL for one query change from day to day. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../src/adapter/MemberSqlTemplateCompiler.js | 157 ++++++- .../filter-params-callback-column.test.ts | 437 ++++++++++++++++++ .../unit/member-sql-template-compiler.test.ts | 94 +++- .../src/cube_bridge/member_sql.rs | 72 ++- .../src/physical_plan/filter/base_filter.rs | 6 +- .../src/physical_plan/filter/typed_filter.rs | 120 +++-- .../src/physical_plan/visitor_context.rs | 2 +- .../cubesqlplanner/src/planner/compiler.rs | 24 + .../multi_stage/multi_stage_query_planner.rs | 6 + .../src/planner/query_properties.rs | 59 ++- .../cubesqlplanner/src/planner/sql_call.rs | 94 +++- .../src/planner/sql_call_builder.rs | 75 ++- .../src/planner/symbols/cube_symbol.rs | 2 +- .../src/planner/symbols/deps.rs | 11 + .../symbols/transforms/static_filter.rs | 66 +++ .../src/planner/visitor_context.rs | 4 +- .../mock_filter_params_callback.rs | 62 +++ .../cube_bridge/mock_member_sql.rs | 85 +++- .../src/test_fixtures/cube_bridge/mod.rs | 2 + .../tests/filter_params_callback_column.rs | 113 +++++ .../cubesqlplanner/src/tests/mod.rs | 1 + 21 files changed, 1397 insertions(+), 95 deletions(-) create mode 100644 packages/cubejs-schema-compiler/test/unit/filter-params-callback-column.test.ts create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_filter_params_callback.rs create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter_params_callback_column.rs diff --git a/packages/cubejs-schema-compiler/src/adapter/MemberSqlTemplateCompiler.js b/packages/cubejs-schema-compiler/src/adapter/MemberSqlTemplateCompiler.js index 09e5c5fcb5554..21c3320d6db29 100644 --- a/packages/cubejs-schema-compiler/src/adapter/MemberSqlTemplateCompiler.js +++ b/packages/cubejs-schema-compiler/src/adapter/MemberSqlTemplateCompiler.js @@ -11,15 +11,19 @@ * { * template: string | string[], * symbolPaths: string[][], // {arg:N} - * filterParams: [{ cube_name, name, column }], // {fp:N}, column = fn|string + * filterParams: [{ cube_name, name, column }], // {fp:N} * filterGroups: [{ filterParams: [...] }], // {fg:N} * securityContextValues: string[] // {sv:N} * } * * Member references are returned as recorded paths — the caller resolves them - * to symbols. FILTER_PARAMS column callbacks are deferred (returned as the raw - * JS function for the caller to invoke at render time); SECURITY_CONTEXT is - * resolved eagerly here against the provided context. + * to symbols. SECURITY_CONTEXT is resolved eagerly here against the provided + * context. + * + * A FILTER_PARAMS `column` is a plain string, or — when the data model gave a + * callback — a recording of its own in the same shape, with the filter values it + * takes as `{fpv:N}` placeholders and `valueParamsCount` of them. A callback + * taking its values through a rest parameter is returned as the bare function. * * The module holds no planner state — `securityContext` and `sqlUtils` are * passed in — so it can be unit-tested in isolation. @@ -29,11 +33,25 @@ const ARG_PREFIX = 'arg'; const FILTER_PARAM_PREFIX = 'fp'; const FILTER_GROUP_PREFIX = 'fg'; const SECURITY_VALUE_PREFIX = 'sv'; +const FILTER_VALUE_PREFIX = 'fpv'; function placeholder(prefix, index) { return `{${prefix}:${index}}`; } +// The proxies handed to the member's `sql` function are captured by any +// `FILTER_PARAMS` column callback it declares, so a nested compile cannot hand +// them a different state. They record into `state.target`, which +// `compileColumnCallback` swaps for the duration of that compile. +function emptyRecording() { + return { + symbolPaths: [], + filterParams: [], + filterGroups: [], + securityContextValues: [], + }; +} + // Returns the index of an equal path if it already exists, otherwise appends // and returns the new index. Paths are short arrays of strings, compared by // value, so repeated references collapse to a single placeholder. @@ -68,12 +86,12 @@ function memberReferenceProxy(path, state) { return undefined; } if (prop === 'sql') { - const index = uniqueInsertPath(state.symbolPaths, [...path, '__sql_fn']); + const index = uniqueInsertPath(state.target.symbolPaths, [...path, '__sql_fn']); const ph = placeholder(ARG_PREFIX, index); return () => ph; } if (prop === 'toString' || prop === 'valueOf') { - const index = uniqueInsertPath(state.symbolPaths, path); + const index = uniqueInsertPath(state.target.symbolPaths, path); const ph = placeholder(ARG_PREFIX, index); return () => ph; } @@ -84,13 +102,112 @@ function memberReferenceProxy(path, state) { // ---- FILTER_PARAMS / FILTER_GROUP ------------------------------------------ +// Declared parameters of a column callback: how many filter values it can take, +// and whether it takes them as a rest parameter. `Function.length` stops at the +// first defaulted parameter, so the parameter list is read from the source. +function declaredValueParams(fn) { + const source = fn.toString(); + const open = source.indexOf('('); + const arrow = source.indexOf('=>'); + if (open === -1 || (arrow !== -1 && arrow < open)) { + // `v => …`, a single parameter without parentheses. + return { count: 1, rest: false, inner: '' }; + } + + let depth = 0; + let close = -1; + for (let i = open; i < source.length; i++) { + const ch = source[i]; + if (ch === '(' || ch === '[' || ch === '{') depth++; + else if (ch === ')' || ch === ']' || ch === '}') { + depth--; + if (depth === 0 && ch === ')') { + close = i; + break; + } + } + } + if (close === -1) { + return { count: 0, rest: false, inner: source }; + } + + const inner = source.slice(open + 1, close); + const params = []; + let start = 0; + depth = 0; + for (let i = 0; i <= inner.length; i++) { + const ch = inner[i]; + if (ch === '(' || ch === '[' || ch === '{') depth++; + else if (ch === ')' || ch === ']' || ch === '}') depth--; + if (i === inner.length || (ch === ',' && depth === 0)) { + const param = inner.slice(start, i).trim(); + if (param) params.push(param); + start = i + 1; + } + } + + return { count: params.length, rest: params.some(p => p.startsWith('...')), inner }; +} + +// Parsing the parameter list can come up short — a bound or native function +// exposes no list, and a `)` inside a comment or a string default ends the scan +// early. Too few placeholders would render the missing values as `undefined`, so +// a count is trusted only when nothing could have thrown the scan off. +// +// `Function.length` counts the parameters before the first defaulted one, so it +// catches a scan that stopped short of that point and nothing after it. Past +// there only the text can vouch: a list holding neither a string nor a comment +// has nothing for the scan to trip over. A list that does holds a callback back +// to the render-time path, where every unreadable list already goes. +function valueParamsAreCertain(fn, count, inner) { + if (fn.toString().includes('[native code]')) { + return false; + } + if (count < fn.length) { + return false; + } + return !/['"`]|\/\*|\/\//.test(inner); +} + +// Compiles a column callback into a template of its own: its filter values +// become `{fpv:N}` placeholders and whatever it references is recorded into its +// own lists, so the placeholders it emits index its own dependencies rather than +// the enclosing member's. A rest parameter consumes as many values as the query +// happens to supply, which a fixed set of placeholders cannot express, so such a +// callback is left for the caller to invoke at render time. +function compileColumnCallback(column, state) { + const { count, rest, inner } = declaredValueParams(column); + if (rest || !valueParamsAreCertain(column, count, inner)) { + return column; + } + + const values = []; + for (let i = 0; i < count; i++) { + values.push(placeholder(FILTER_VALUE_PREFIX, i)); + } + + const recording = emptyRecording(); + const outer = state.target; + state.target = recording; + try { + const template = parseTemplateResult(column(...values)); + return { template, valueParamsCount: count, ...recording }; + } finally { + state.target = outer; + } +} + function filterParamsItemProxy(cubeName, name, state) { return { filter(column) { - const item = { cube_name: cubeName, name, column }; + const item = { + cube_name: cubeName, + name, + column: typeof column === 'function' ? compileColumnCallback(column, state) : column, + }; const toString = () => { - const index = state.filterParams.length; - state.filterParams.push(item); + const index = state.target.filterParams.length; + state.target.filterParams.push(item); return placeholder(FILTER_PARAM_PREFIX, index); }; // `__member` lets FILTER_GROUP recover the item; `toString` records and @@ -120,8 +237,8 @@ function filterGroupFn(state) { } return arg.__member; }); - const index = state.filterGroups.length; - state.filterGroups.push({ filterParams }); + const index = state.target.filterGroups.length; + state.target.filterGroups.push({ filterParams }); return placeholder(FILTER_GROUP_PREFIX, index); }; } @@ -164,7 +281,7 @@ function coerceToStringValue(value) { } function recordSecurityValue(value, state) { - return placeholder(SECURITY_VALUE_PREFIX, uniqueInsertString(state.securityContextValues, value)); + return placeholder(SECURITY_VALUE_PREFIX, uniqueInsertString(state.target.securityContextValues, value)); } function securityFilterFn(value, required, state) { @@ -247,23 +364,13 @@ function parseTemplateResult(result) { * @param {object} sqlUtils the SQL_UTILS object passed through to the template */ function compileMemberSql(sqlFn, argNames, securityContext, sqlUtils) { - const state = { - symbolPaths: [], - filterParams: [], - filterGroups: [], - securityContextValues: [], - }; + const root = emptyRecording(); + const state = { target: root }; const args = argNames.map(name => buildArg(name, state, securityContext, sqlUtils)); const template = parseTemplateResult(sqlFn(...args)); - return { - template, - symbolPaths: state.symbolPaths, - filterParams: state.filterParams, - filterGroups: state.filterGroups, - securityContextValues: state.securityContextValues, - }; + return { template, ...root }; } exports.compileMemberSql = compileMemberSql; diff --git a/packages/cubejs-schema-compiler/test/unit/filter-params-callback-column.test.ts b/packages/cubejs-schema-compiler/test/unit/filter-params-callback-column.test.ts new file mode 100644 index 0000000000000..a19cfd44801d7 --- /dev/null +++ b/packages/cubejs-schema-compiler/test/unit/filter-params-callback-column.test.ts @@ -0,0 +1,437 @@ +/* eslint-disable no-template-curly-in-string */ +import { PostgresQuery } from '../../src/adapter/PostgresQuery'; +import { prepareJsCompiler } from './PrepareCompiler'; + +// A `FILTER_PARAMS.….filter(cb)` column callback runs at render time, and the +// member references inside it must resolve to that member's SQL — the same as +// when the column is handed over as a template string. Here the callback is the +// only place referencing the time dimension, and it sits in a measure `filters:` +// entry, so nothing else in that SQL function records the reference. +const model = [ + 'cube(\'commission\', {', + ' sql: `SELECT * FROM commission`,', + ' measures: {', + ' dailyMrr: {', + ' sql: `${CUBE}.total`,', + ' type: `sum`,', + ' filters: [', + ' { sql: `${CUBE.pricingDuration} = \'DAILY\'` },', + ' { sql: `${FILTER_PARAMS.commission.reconciliationDate.filter((from, to) => `${CUBE.reconciliationDate} >= ${from} AND ${CUBE.reconciliationDate} < ${to}`)}` }', + ' ]', + ' }', + ' },', + ' dimensions: {', + ' id: {', + ' sql: `id`,', + ' type: `number`,', + ' primaryKey: true', + ' },', + ' partner: {', + ' sql: `partner`,', + ' type: `string`', + ' },', + ' currency: {', + ' sql: `currency`,', + ' type: `string`', + ' },', + ' pricingDuration: {', + ' sql: `pricing_duration`,', + ' type: `string`', + ' },', + ' reconciliationDate: {', + ' sql: `reconciliation_date`,', + ' type: `time`', + ' }', + ' },', + ' preAggregations: {', + ' main: {', + ' measures: [CUBE.dailyMrr],', + ' dimensions: [CUBE.partner, CUBE.currency],', + ' timeDimension: CUBE.reconciliationDate,', + ' granularity: `month`,', + ' partitionGranularity: `year`', + ' }', + ' }', + '});', +].join('\n'); + +// The callback's own SQL, with the time dimension resolved and the date bounds +// bound as params. +const PUSHED_DOWN_PREDICATE = /"commission"\.reconciliation_date >= \$\d+ AND "commission"\.reconciliation_date < \$\d+/; + +async function queryFor(useNativeSqlPlanner: boolean, options = {}) { + const { compiler, joinGraph, cubeEvaluator } = prepareJsCompiler(model); + await compiler.compile(); + + return new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, { + measures: ['commission.dailyMrr'], + dimensions: ['commission.partner'], + filters: [{ + member: 'commission.currency', + operator: 'equals', + values: ['USD'], + }], + timeDimensions: [{ + dimension: 'commission.reconciliationDate', + granularity: 'month', + dateRange: ['2025-07-01', '2026-06-30'], + }], + timezone: 'UTC', + useNativeSqlPlanner, + ...options, + }); +} + +describe('FILTER_PARAMS callback column', () => { + describe.each([ + ['legacy planner', false], + ['native planner', true], + ])('%s', (_name, useNativeSqlPlanner) => { + it('renders the callback of a measure filter against the cube table', async () => { + // `pricingDuration` is outside the pre-aggregation, so the query reads the + // cube itself and the measure filter is rendered. + const query = await queryFor(useNativeSqlPlanner, { + dimensions: ['commission.partner', 'commission.pricingDuration'], + }); + const [sql] = query.buildSqlAndParams(); + + expect(sql).toMatch(PUSHED_DOWN_PREDICATE); + }); + + it('renders the callback of a measure filter in a pre-aggregation build query', async () => { + const query = await queryFor(useNativeSqlPlanner); + const [description]: any = query.preAggregations?.preAggregationsDescription(); + const [loadSql] = description.loadSql; + + expect(loadSql).toMatch(PUSHED_DOWN_PREDICATE); + }); + + // The column applies what its filter supplies, so an operator carrying no + // values leaves nothing to apply. + it('applies nothing when the filter on the column carries no values', async () => { + const query = await queryFor(useNativeSqlPlanner, { + dimensions: ['commission.partner', 'commission.pricingDuration'], + timeDimensions: [], + filters: [{ member: 'commission.reconciliationDate', operator: 'set' }], + }); + const [sql] = query.buildSqlAndParams(); + + expect(sql).not.toMatch(PUSHED_DOWN_PREDICATE); + expect(sql).toContain('1 = 1'); + }); + }); + + // A one-sided date operator carries one bound where this column takes both, so + // there is nothing to bind its second placeholder to and the column applies + // nothing. The filter still reaches the query on its own. + // + // The legacy planner instead fills the missing bound in with the current time, + // which for `beforeDate` lands the given value on the opposite side of the + // range and leaves the measure empty for every row the query keeps — so this + // is a divergence on purpose. + it('applies nothing for a filter carrying fewer values than the column takes', async () => { + const query = await queryFor(true, { + dimensions: ['commission.partner', 'commission.pricingDuration'], + timeDimensions: [], + filters: [{ + member: 'commission.reconciliationDate', + operator: 'beforeDate', + values: ['2025-07-01'], + }], + }); + const [sql] = query.buildSqlAndParams(); + + expect(sql).not.toMatch(PUSHED_DOWN_PREDICATE); + expect(sql).toContain('1 = 1'); + }); + + // A column renders only where its filter reaches the query, so the cube it + // reads is needed exactly there — and nowhere else. + describe.each([ + ['a member of another cube', '${users.city} IS NOT NULL AND ${CUBE.createdAt} >= ${from}'], + ['another cube directly', '${users}.city IS NOT NULL AND ${CUBE.createdAt} >= ${from}'], + ])('a column reading %s', (_name, callbackBody) => { + const schema = [ + 'cube(\'orders\', {', + ' sql: `SELECT * FROM orders`,', + ' joins: {', + ' users: {', + ' sql: `${CUBE}.user_id = ${users}.id`,', + ' relationship: `belongsTo`', + ' }', + ' },', + ' measures: {', + ' total: {', + ' sql: `${CUBE}.amount`,', + ' type: `sum`,', + ' filters: [', + ` { sql: \`\${FILTER_PARAMS.orders.createdAt.filter((from, to) => \`${callbackBody}\`)}\` }`, + ' ]', + ' }', + ' },', + ' dimensions: {', + ' id: {', + ' sql: `id`,', + ' type: `number`,', + ' primaryKey: true', + ' },', + ' createdAt: {', + ' sql: `created_at`,', + ' type: `time`', + ' }', + ' }', + '});', + 'cube(\'users\', {', + ' sql: `SELECT * FROM users`,', + ' dimensions: {', + ' id: {', + ' sql: `id`,', + ' type: `number`,', + ' primaryKey: true', + ' },', + ' city: {', + ' sql: `city`,', + ' type: `string`', + ' }', + ' }', + '});', + ].join('\n'); + + async function sqlFor(useNativeSqlPlanner: boolean, withFilter: boolean) { + const { compiler, joinGraph, cubeEvaluator } = prepareJsCompiler(schema); + await compiler.compile(); + const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, { + measures: ['orders.total'], + filters: withFilter + ? [{ member: 'orders.createdAt', operator: 'inDateRange', values: ['2025-07-01', '2026-06-30'] }] + : [], + timezone: 'UTC', + useNativeSqlPlanner, + }); + + return query.buildSqlAndParams()[0]; + } + + it.each([ + ['legacy planner', false], + ['native planner', true], + ])('joins that cube for %s when the filter reaches the query', async (_planner, useNativeSqlPlanner) => { + const sql = await sqlFor(useNativeSqlPlanner, true); + + expect(sql).toMatch(/join\s+users/i); + expect(sql).toContain('"users".city IS NOT NULL'); + }); + + it.each([ + ['legacy planner', false], + ['native planner', true], + ])('leaves that cube out for %s when the filter does not', async (_planner, useNativeSqlPlanner) => { + const sql = await sqlFor(useNativeSqlPlanner, false); + + expect(sql).not.toMatch(/join\s+users/i); + expect(sql).toContain('1 = 1'); + }); + }); +}); + +// A cube's `sql` builds the table the query reads from, so nothing a member +// reference could resolve against is in scope inside it — whichever way the +// reference is written. +describe('member references in a cube\'s sql', () => { + const cubeWith = (cubeSql: string, extraCube = '') => [ + extraCube, + 'cube(\'commission\', {', + ` sql: \`${cubeSql}\`,`, + ' measures: {', + ' total: {', + ' sql: `${CUBE}.total`,', + ' type: `sum`', + ' }', + ' },', + ' dimensions: {', + ' id: {', + ' sql: `id`,', + ' type: `number`,', + ' primaryKey: true', + ' },', + ' reconciliationDate: {', + ' sql: `reconciliation_date`,', + ' type: `time`', + ' }', + ' }', + '});', + ].join('\n'); + + async function sqlFor(schema: string, withFilter: boolean) { + const { compiler, joinGraph, cubeEvaluator } = prepareJsCompiler(schema); + await compiler.compile(); + + const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, { + measures: ['commission.total'], + timeDimensions: withFilter + ? [{ dimension: 'commission.reconciliationDate', dateRange: ['2025-07-01', '2026-06-30'] }] + : [], + timezone: 'UTC', + contextSymbols: { securityContext: { tenantId: 'acme' } }, + useNativeSqlPlanner: true, + }); + + return query.buildSqlAndParams()[0]; + } + + const REJECTED = /references member `commission\.reconciliationDate`/; + + // Each spelling reaches the same place, so each has to be reported the same + // way rather than resolved or left to render a qualifier for a table the query + // does not read. + it.each([ + ['a direct reference', 'SELECT * FROM commission WHERE ${CUBE.reconciliationDate} IS NOT NULL'], + ['a string filter param column', 'SELECT * FROM commission WHERE ${FILTER_PARAMS.commission.reconciliationDate.filter(`${CUBE.reconciliationDate}`)}'], + ['a filter param column callback', 'SELECT * FROM commission WHERE ${FILTER_PARAMS.commission.reconciliationDate.filter((from, to) => `${CUBE.reconciliationDate} >= ${from}`)}'], + ])('rejects %s', async (_name, cubeSql) => { + // Reported whether or not the query supplies the filter, since resolving the + // reference is what a cube's sql cannot do at all. + await expect(sqlFor(cubeWith(cubeSql), false)).rejects.toThrow(REJECTED); + await expect(sqlFor(cubeWith(cubeSql), true)).rejects.toThrow(REJECTED); + }); + + it('keeps a filter param column that reads plain columns', async () => { + const schema = cubeWith('SELECT * FROM commission WHERE ${FILTER_PARAMS.commission.reconciliationDate.filter((from, to) => `reconciliation_date >= ${from} AND reconciliation_date < ${to}`)}'); + + expect(await sqlFor(schema, false)).toContain('WHERE 1 = 1'); + expect(await sqlFor(schema, true)).toMatch(/reconciliation_date >= \$\d+ AND reconciliation_date < \$\d+/); + }); + + // A security context value needs no member in scope — it becomes a query + // param — so the row-level-security shape keeps working. + it('keeps a security context value inside a filter param column', async () => { + const schema = cubeWith('SELECT * FROM commission WHERE ${FILTER_PARAMS.commission.reconciliationDate.filter((from, to) => `reconciliation_date >= ${from} AND tenant = ${SECURITY_CONTEXT.tenantId.unsafeValue()}`)}'); + + expect(await sqlFor(schema, true)).toContain('tenant = acme'); + }); + + it('keeps a reference to another cube\'s sql', async () => { + const base = [ + 'cube(\'base\', {', + ' sql: `SELECT * FROM raw`,', + ' dimensions: {', + ' id: {', + ' sql: `id`,', + ' type: `number`,', + ' primaryKey: true', + ' }', + ' }', + '});', + ].join('\n'); + + expect(await sqlFor(cubeWith('SELECT * FROM ${base.sql()}', base), false)).toMatch(/FROM\s+raw\b/); + }); +}); + +// A column renders wherever the symbol carrying it does, which includes symbols a +// query reaches only through a filter, a segment or an order item. Its cube has +// to be joined in every one of those, or the qualifier it renders has no table +// behind it. +describe('a column reached through something other than a selected member', () => { + const COLUMN = '${FILTER_PARAMS.orders.createdAt.filter((from, to) => `${users.city} IS NOT NULL AND ${CUBE.createdAt} >= ${from}`)}'; + + const schema = [ + 'cube(\'orders\', {', + ' sql: `SELECT * FROM orders`,', + ' joins: {', + ' users: {', + ' sql: `${CUBE}.user_id = ${users}.id`,', + ' relationship: `belongsTo`', + ' }', + ' },', + ' measures: {', + ' count: {', + ' type: `count`', + ' },', + ` total: { sql: \`\${CUBE}.amount\`, type: \`sum\`, filters: [{ sql: \`${COLUMN}\` }] },`, + ' grouped: {', + ' sql: `${CUBE}.amount`,', + ' type: `sum`,', + ' filters: [{ sql: `${FILTER_GROUP(', + ' FILTER_PARAMS.orders.createdAt.filter((from, to) => `${users.city} IS NOT NULL AND ${CUBE.createdAt} >= ${from}`),', + ' FILTER_PARAMS.orders.status.filter((v) => `${CUBE.status} = ${v}`)', + ' )}` }]', + ' }', + ' },', + ' dimensions: {', + ' id: {', + ' sql: `id`,', + ' type: `number`,', + ' primaryKey: true', + ' },', + ' status: {', + ' sql: `status`,', + ' type: `string`', + ' },', + ' createdAt: {', + ' sql: `created_at`,', + ' type: `time`', + ' },', + ` flagged: { sql: \`CASE WHEN ${COLUMN} THEN 1 ELSE 0 END\`, type: \`number\` }`, + ' },', + ` segments: { recent: { sql: \`${COLUMN}\` } }`, + '});', + 'cube(\'users\', {', + ' sql: `SELECT * FROM users`,', + ' dimensions: {', + ' id: {', + ' sql: `id`,', + ' type: `number`,', + ' primaryKey: true', + ' },', + ' city: {', + ' sql: `city`,', + ' type: `string`', + ' }', + ' }', + '});', + ].join('\n'); + + const RANGE = { member: 'orders.createdAt', operator: 'inDateRange', values: ['2025-07-01', '2026-06-30'] }; + + async function sqlFor(query: any, useNativeSqlPlanner: boolean) { + const { compiler, joinGraph, cubeEvaluator } = prepareJsCompiler(schema); + await compiler.compile(); + + return new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, { + ...query, + timezone: 'UTC', + useNativeSqlPlanner, + }).buildSqlAndParams()[0]; + } + + it.each([ + ['a segment', { measures: ['orders.count'], segments: ['orders.recent'], filters: [RANGE] }], + ['a dimension only named in a filter', { measures: ['orders.count'], filters: [RANGE, { member: 'orders.flagged', operator: 'equals', values: ['1'] }] }], + ['a measure only named in a having filter', { measures: ['orders.count'], filters: [RANGE, { member: 'orders.total', operator: 'gt', values: ['1'] }] }], + // A FILTER_GROUP renders as one predicate, so its members share one verdict; + // an OR group survives only when every member of it matches the query. + ['a filter group under an or filter', { measures: ['orders.grouped'], filters: [{ or: [RANGE, { member: 'orders.status', operator: 'equals', values: ['x'] }] }] }], + ])('joins the cube read through %s', async (_name, query) => { + for (const useNativeSqlPlanner of [false, true]) { + const sql = await sqlFor(query, useNativeSqlPlanner); + + expect(sql).toContain('"users".city IS NOT NULL'); + expect(sql).toMatch(/join\s+users/i); + } + }); + + // The legacy planner cannot build this one at all: collecting its join hints + // recurses until the stack runs out. + it('joins the cube read through a filter group under an and filter', async () => { + const query = { + measures: ['orders.grouped'], + filters: [RANGE, { member: 'orders.status', operator: 'equals', values: ['x'] }], + }; + + const sql = await sqlFor(query, true); + + expect(sql).toContain('"users".city IS NOT NULL'); + expect(sql).toMatch(/join\s+users/i); + }); +}); diff --git a/packages/cubejs-schema-compiler/test/unit/member-sql-template-compiler.test.ts b/packages/cubejs-schema-compiler/test/unit/member-sql-template-compiler.test.ts index ebe37b975c398..e93a969b76a5d 100644 --- a/packages/cubejs-schema-compiler/test/unit/member-sql-template-compiler.test.ts +++ b/packages/cubejs-schema-compiler/test/unit/member-sql-template-compiler.test.ts @@ -87,7 +87,7 @@ describe('MemberSqlTemplateCompiler — FILTER_PARAMS / FILTER_GROUP', () => { expect(res.filterParams).toEqual([{ cube_name: 'orders', name: 'status', column: 't.status' }]); }); - it('keeps the column callback as a function (deferred) and records {fp:N}', () => { + it('compiles a column callback into a template of its own and records {fp:N}', () => { const res = compileMemberSql( (FILTER_PARAMS) => `${FILTER_PARAMS.orders.status.filter((c) => `${c} > 0`)}`, ['FILTER_PARAMS'] @@ -96,9 +96,97 @@ describe('MemberSqlTemplateCompiler — FILTER_PARAMS / FILTER_GROUP', () => { expect(res.filterParams).toHaveLength(1); expect(res.filterParams[0].cube_name).toBe('orders'); expect(res.filterParams[0].name).toBe('status'); + // The filter value it takes becomes a placeholder of its own. + expect(res.filterParams[0].column.template).toBe('{fpv:0} > 0'); + expect(res.filterParams[0].column.valueParamsCount).toBe(1); + expect(res.filterParams[0].column.symbolPaths).toEqual([]); + }); + + // What a callback references belongs to the callback, not to the member whose + // sql declared it: the placeholders it emits index its own dependency list. + it('records a callback reference into the callback, not the enclosing member', () => { + const res = compileMemberSql( + (CUBE, FILTER_PARAMS) => `${FILTER_PARAMS.orders.createdAt.filter((from, to) => `${CUBE.createdAt} >= ${from} AND ${CUBE.createdAt} < ${to}`)}`, + ['CUBE', 'FILTER_PARAMS'] + ); + expect(res.template).toBe('{fp:0}'); + expect(res.symbolPaths).toEqual([]); + expect(res.filterParams[0].column.template) + .toBe('{arg:0} >= {fpv:0} AND {arg:0} < {fpv:1}'); + expect(res.filterParams[0].column.symbolPaths).toEqual([['CUBE', 'createdAt']]); + }); + + it('numbers callback references separately from the enclosing template', () => { + const res = compileMemberSql( + (CUBE, FILTER_PARAMS) => `${CUBE.a} AND ${FILTER_PARAMS.orders.b.filter((v) => `${CUBE.b} = ${v}`)}`, + ['CUBE', 'FILTER_PARAMS'] + ); + expect(res.template).toBe('{arg:0} AND {fp:0}'); + expect(res.symbolPaths).toEqual([['CUBE', 'a']]); + expect(res.filterParams[0].column.template).toBe('{arg:0} = {fpv:0}'); + expect(res.filterParams[0].column.symbolPaths).toEqual([['CUBE', 'b']]); + }); + + it('counts a defaulted parameter as a filter value', () => { + const res = compileMemberSql( + (FILTER_PARAMS) => `${FILTER_PARAMS.orders.a.filter((from, to = 1) => `d BETWEEN ${from} AND ${to}`)}`, + ['FILTER_PARAMS'] + ); + expect(res.filterParams[0].column.template).toBe('d BETWEEN {fpv:0} AND {fpv:1}'); + }); + + // A rest parameter takes as many values as the query supplies, which a fixed + // set of placeholders cannot express. + it('leaves a rest-parameter column callback uncompiled', () => { + const res = compileMemberSql( + (CUBE, FILTER_PARAMS) => `${FILTER_PARAMS.orders.a.filter((...vals) => vals.map(v => `${CUBE.a} = ${v}`).join(' OR '))}`, + ['CUBE', 'FILTER_PARAMS'] + ); expect(typeof res.filterParams[0].column).toBe('function'); - const captured = res.filterParams[0].column; - expect(captured('X')).toBe('X > 0'); + }); + + // Too few placeholders would render the missing values as `undefined`, so a + // parameter list that cannot be read in full is left to render time. + it.each([ + // eslint-disable-next-line no-extra-bind + ['a bound callback', ((from, to) => `d >= ${from} AND d < ${to}`).bind(null)], + ['a comment closing the parameter list', (from /* ) */, to) => `d >= ${from} AND d < ${to}`], + // `Function.length` counts the parameters before the first default, so it + // cannot speak for the parse past that point — whichever parameter carries + // the string that breaks the scan. + ['a default containing a paren', (from, to = '(') => `d >= ${from} AND d < ${to}`], + ['a first parameter defaulted to a paren', (from = ')', to) => `d >= ${from} AND d < ${to}`], + ['a paren in a default with a parameter behind it', (from, to = ')', third) => `d >= ${from} AND d < ${to} AND x = ${third}`], + // A quote alone is enough to hold the callback back: whether it hides a paren + // is exactly what the scan cannot tell. + ['a default containing a quoted string', (from, to = 'x') => `d >= ${from} AND d < ${to}`], + ])('leaves %s uncompiled', (_name, column) => { + const res = compileMemberSql( + (FILTER_PARAMS) => `${FILTER_PARAMS.orders.a.filter(column)}`, + ['FILTER_PARAMS'] + ); + expect(typeof res.filterParams[0].column).toBe('function'); + }); + + // A defaulted parameter only costs `Function.length` its witness; a list with + // nothing for the scan to trip over is still read in full. + it('compiles a callback whose first parameter has a plain default', () => { + const res = compileMemberSql( + (FILTER_PARAMS) => `${FILTER_PARAMS.orders.a.filter((from = 1, to) => `d >= ${from} AND d < ${to}`)}`, + ['FILTER_PARAMS'] + ); + expect(res.filterParams[0].column.template).toBe('d >= {fpv:0} AND d < {fpv:1}'); + }); + + it('records a security context value referenced from a column callback into the callback', () => { + const res = compileMemberSql( + (SECURITY_CONTEXT, FILTER_PARAMS) => `${FILTER_PARAMS.orders.a.filter((v) => `t = ${SECURITY_CONTEXT.tenantId} AND a = ${v}`)}`, + ['SECURITY_CONTEXT', 'FILTER_PARAMS'], + { tenantId: 'acme' } + ); + expect(res.securityContextValues).toEqual([]); + expect(res.filterParams[0].column.template).toBe('t = {sv:0} AND a = {fpv:0}'); + expect(res.filterParams[0].column.securityContextValues).toEqual(['acme']); }); it('records a filter group from filter-param args and yields {fg:0}', () => { diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/cube_bridge/member_sql.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/cube_bridge/member_sql.rs index ac744baf5a063..d2a94f0e902a4 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/cube_bridge/member_sql.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/cube_bridge/member_sql.rs @@ -37,12 +37,68 @@ impl NativeDeserialize for SqlTemplate { } } +/// A column callback compiled into a template of its own. `{fpv:N}` +/// marks the Nth filter value the planner supplies at render time; +/// every other placeholder indexes `args`, the dependencies the +/// callback body touched. +#[derive(Clone, Debug)] +pub struct CompiledFilterParamsColumn { + pub template: SqlTemplate, + pub args: SqlTemplateArgs, + pub value_params_count: usize, +} + +impl CompiledFilterParamsColumn { + fn clone_to_context( + &self, + context_ref: &dyn NativeContextHolderRef, + ) -> Result { + Ok(Self { + template: self.template.clone(), + args: self.args.clone_to_context(context_ref)?, + value_params_count: self.value_params_count, + }) + } +} + +impl NativeDeserialize for CompiledFilterParamsColumn { + fn from_native(native_object: NativeObjectHandle) -> Result { + let object = native_object.to_struct()?; + let template = SqlTemplate::from_native(object.get_field("template")?)?; + let symbol_paths = Vec::>::from_native(object.get_field("symbolPaths")?)?; + let filter_params = deserialize_filter_params_vec(object.get_field("filterParams")?)?; + let filter_groups = object + .get_field("filterGroups")? + .to_array()? + .to_vec()? + .into_iter() + .map(FilterGroupItem::from_native) + .collect::, _>>()?; + let values = Vec::::from_native(object.get_field("securityContextValues")?)?; + let value_params_count = f64::from_native(object.get_field("valueParamsCount")?)? as usize; + Ok(Self { + template, + args: SqlTemplateArgs { + symbol_paths, + filter_params, + filter_groups, + security_context: SecutityContextProps { values }, + }, + value_params_count, + }) + } +} + /// Column argument passed to -/// `FILTER_PARAMS.cube.member.filter(...)`: either a plain column -/// name string, or a JS callback that produces the SQL snippet. +/// `FILTER_PARAMS.cube.member.filter(...)`: a plain column name +/// string, a callback already compiled into its own template, or — +/// when the callback takes its values as a rest parameter, so a fixed +/// set of value placeholders cannot express it — the raw JS callback +/// to invoke at render time. #[derive(Clone)] pub enum FilterParamsColumn { String(String), + Compiled(Rc), Callback(Rc), } @@ -53,6 +109,9 @@ impl FilterParamsColumn { ) -> Result { let res = match self { Self::String(s) => Self::String(s.clone()), + Self::Compiled(compiled) => { + Self::Compiled(Rc::new(compiled.clone_to_context(context_ref)?)) + } Self::Callback(callback) => Self::Callback(callback.clone_to_context(context_ref)?), }; Ok(res) @@ -66,6 +125,9 @@ impl NativeSerialize for FilterParamsColumn { ) -> Result, CubeError> { match self { FilterParamsColumn::String(s) => s.to_native(context.clone()), + FilterParamsColumn::Compiled(_) => Err(CubeError::internal( + "Compiled filter params column cannot be serialized back".to_string(), + )), FilterParamsColumn::Callback(cb) => { if let Ok(callback) = cb .clone() @@ -84,8 +146,13 @@ impl NativeSerialize for FilterParamsColumn { } impl NativeDeserialize for FilterParamsColumn { fn from_native(native_object: NativeObjectHandle) -> Result { + // A compiled column arrives as a struct carrying its template; a rest-param + // callback arrives as a bare function, which has no such field. let column = if let Ok(string_column) = String::from_native(native_object.clone()) { FilterParamsColumn::String(string_column) + } else if let Ok(compiled) = CompiledFilterParamsColumn::from_native(native_object.clone()) + { + FilterParamsColumn::Compiled(Rc::new(compiled)) } else { let callback = NativeFilterParamsCallback::from_native(native_object.clone())?; FilterParamsColumn::Callback(Rc::new(callback)) @@ -98,6 +165,7 @@ impl std::fmt::Debug for FilterParamsColumn { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::String(arg0) => f.debug_tuple("String").field(arg0).finish(), + Self::Compiled(compiled) => f.debug_tuple("Compiled").field(compiled).finish(), Self::Callback(_) => f .debug_tuple("Callback") .field(&"JsFunc".to_string()) diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/base_filter.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/base_filter.rs index 4cb5e629b3b17..c72348f2b5dd5 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/base_filter.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/filter/base_filter.rs @@ -21,7 +21,7 @@ impl ToSql for BaseFilter { if !filters_ctx.filter_params_columns.is_empty() { let symbol_to_match = resolve_base_symbol(self.raw_member_evaluator_ref()).resolve_reference_chain(); - if let Some(filter_params_column) = filters_ctx + if let Some(filter_params_item) = filters_ctx .filter_params_columns .get(&symbol_to_match.full_name()) { @@ -31,8 +31,10 @@ impl ToSql for BaseFilter { .get(&symbol_to_match.full_name()) .and_then(|shift| shift.interval.as_ref()); return self.typed_filter().to_sql_for_filter_params( - filter_params_column, + filter_params_item, time_shift, + visitor, + node_processor, &query_tools, templates, filters_ctx, 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 e9b7f3077115e..bb9acc21f293e 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 @@ -5,6 +5,7 @@ use crate::physical_plan::sql_nodes::SqlNode; use crate::physical_plan::SqlEvaluatorVisitor; use crate::planner::filter::typed_filter::{resolve_base_symbol, FilterOp, TypedFilter}; use crate::planner::query_tools::QueryTools; +use crate::planner::sql_call::SqlCallFilterParamsItem; use crate::planner::sql_templates::PlanSqlTemplates; use crate::planner::FiltersContext; use crate::planner::SqlInterval; @@ -48,15 +49,17 @@ impl ToSql for TypedFilter { impl TypedFilter { pub fn to_sql_for_filter_params( &self, - column: &FilterParamsColumn, + item: &SqlCallFilterParamsItem, time_shift: Option<&SqlInterval>, + visitor: &SqlEvaluatorVisitor, + node_processor: Rc, query_tools: &Rc, plan_templates: &PlanSqlTemplates, filters_context: &FiltersContext, ) -> Result { let use_db_time_zone = !filters_context.use_local_tz; - match column { + match &item.column { FilterParamsColumn::String(column_sql) => { // Inside a time-shifted CTE the FILTER_PARAMS column must carry the // same shift as the regular time-dimension filter, otherwise its @@ -82,47 +85,92 @@ impl TypedFilter { }; dispatch_to_sql(self.operation(), &ctx) } + FilterParamsColumn::Compiled(compiled) => { + if time_shift.is_some() { + return Err(CubeError::user(format!( + "FILTER_PARAMS column for `{}` is a callback, which cannot carry the time \ + shift the surrounding query applies; pass the column as a string instead", + item.filter_symbol_name + ))); + } + let values = + self.filter_param_values(query_tools, plan_templates, use_db_time_zone)?; + // A column applies what its filter supplies, and nothing when the + // filter cannot supply what the column takes — a `set` or `notSet` + // operator carries no values at all, and a one-sided date operator + // carries one where the column takes both bounds. The filter still + // reaches the query on its own; only its restatement inside this + // SQL is dropped, which is narrower than binding a bound the + // filter never gave. + if values.len() < compiled.value_params_count { + return plan_templates.always_true(); + } + let Some(call) = &item.compiled_call else { + return Err(CubeError::internal(format!( + "Compiled filter params column for `{}` has no call", + item.filter_symbol_name + ))); + }; + call.eval_with_filter_values( + visitor, + node_processor, + query_tools.clone(), + plan_templates, + &values, + ) + } FilterParamsColumn::Callback(callback) => { // A callback column is opaque SQL produced by user code, so a // time shift can't be wrapped around it; it is rendered as-is. - let args = match self.operation() { - // 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: "", - query_tools, - plan_templates, - use_db_time_zone, - use_raw_values: self.use_raw_values(), - }; - let from = self - .values() - .first() - .and_then(|v| v.to_param_string()) - .map(|v| ctx.format_and_allocate_from_date_no_cast(&v)) - .transpose()?; - let to = self - .values() - .get(1) - .and_then(|v| v.to_param_string()) - .map(|v| ctx.format_and_allocate_to_date_no_cast(&v)) - .transpose()?; - [from, to].into_iter().flatten().collect() - } - _ => self - .values() - .iter() - .filter_map(|v| v.to_param_string()) - .map(|v| query_tools.allocate_param(&v)) - .collect::>(), - }; + let args = + self.filter_param_values(query_tools, plan_templates, use_db_time_zone)?; callback.call(&args) } } } + + // The filter's values, formatted the way a `FILTER_PARAMS` column expects to + // receive them. + fn filter_param_values( + &self, + query_tools: &Rc, + plan_templates: &PlanSqlTemplates, + use_db_time_zone: bool, + ) -> Result, CubeError> { + let args = match self.operation() { + // 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: "", + query_tools, + plan_templates, + use_db_time_zone, + use_raw_values: self.use_raw_values(), + }; + let from = self + .values() + .first() + .and_then(|v| v.to_param_string()) + .map(|v| ctx.format_and_allocate_from_date_no_cast(&v)) + .transpose()?; + let to = self + .values() + .get(1) + .and_then(|v| v.to_param_string()) + .map(|v| ctx.format_and_allocate_to_date_no_cast(&v)) + .transpose()?; + [from, to].into_iter().flatten().collect() + } + _ => self + .values() + .iter() + .filter_map(|v| v.to_param_string()) + .map(|v| query_tools.allocate_param(&v)) + .collect::>(), + }; + Ok(args) + } } fn dispatch_to_sql(op: &FilterOp, ctx: &FilterSqlContext) -> Result { diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/visitor_context.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/visitor_context.rs index 47f195bdc2995..1a76613fa0cef 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/visitor_context.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/visitor_context.rs @@ -45,7 +45,7 @@ impl VisitorContext { pub fn new_for_filter_params( query_tools: Rc, nodes_factory: &SqlNodesFactory, - filter_params_columns: HashMap, + filter_params_columns: HashMap, time_shifts: TimeShiftState, ) -> Self { let filters_context = FiltersContext { diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/compiler.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/compiler.rs index 40fabd1420216..ac14f1142ad85 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/compiler.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/compiler.rs @@ -251,6 +251,25 @@ impl Compiler { &mut self, cube_name: &String, member_sql: Rc, + ) -> Result, CubeError> { + self.compile_sql_call_impl(cube_name, member_sql, false) + } + + /// Compiles a cube's own `sql`, where a member reference is rejected + /// rather than resolved. + pub fn compile_cube_sql_call( + &mut self, + cube_name: &String, + member_sql: Rc, + ) -> Result, CubeError> { + self.compile_sql_call_impl(cube_name, member_sql, true) + } + + fn compile_sql_call_impl( + &mut self, + cube_name: &String, + member_sql: Rc, + is_cube_sql: bool, ) -> Result, CubeError> { let call_builder = SqlCallBuilder::new( self, @@ -258,6 +277,11 @@ impl Compiler { self.base_tools.clone(), self.security_context.clone(), ); + let call_builder = if is_cube_sql { + call_builder.for_cube_sql() + } else { + call_builder + }; let sql_call = call_builder.build(&cube_name, member_sql.clone())?; Ok(Rc::new(sql_call)) } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/multi_stage_query_planner.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/multi_stage_query_planner.rs index a52869d71a828..5105653f32b43 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/multi_stage_query_planner.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/multi_stage_query_planner.rs @@ -473,6 +473,12 @@ impl MultiStageQueryPlanner { let member = member.resolve_reference_chain(); let member = transforms::apply_static_filter_to_symbol(&member, state.dimensions_filters())?; + // `filter: include` lets a stage filter a member the query around it does + // not, so activity is settled against this stage's own set. + let member = transforms::apply_filter_params_activity_to_symbol( + &member, + &transforms::filter_params_activity_filters(&state.all_filter_items()), + )?; let state = if member.is_dimension() { let mut new_state = state.as_ref().clone(); new_state.remove_multistage_dimensions(resolved_multi_stage_dimensions)?; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_properties.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_properties.rs index c69ad672ab318..6bedacca36d62 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_properties.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_properties.rs @@ -234,10 +234,51 @@ impl QueryProperties { } // Push every entry of `dimensions_filters` into matching `case` - // expressions on each member, filter and order item. Run once at - // construction; mutators do not re-apply it. + // expressions, and mark every FILTER_PARAMS binding by whether the query + // filters the members it renders from. Both cover each member, filter and + // order item. Run once at construction; mutators do not re-apply it. fn apply_static_filters(&mut self) -> Result<(), CubeError> { let dimensions_filters = self.dimensions_filters.clone(); + // A FILTER_PARAMS binding may name any filtered member, not only a + // dimension, so its activity is read from the whole set. + // + // A multi-stage stage may then filter more than the query around it. It + // builds its own `QueryProperties` from its state, so this runs again for + // it and settles activity against the set that stage renders with. + let all_filters = transforms::filter_params_activity_filters(&self.all_filter_items()); + for dim in self.dimensions.iter_mut() { + *dim = transforms::apply_filter_params_activity_to_symbol(dim, &all_filters)?; + } + for dim in self.time_dimensions.iter_mut() { + *dim = transforms::apply_filter_params_activity_to_symbol(dim, &all_filters)?; + } + for meas in self.measures.iter_mut() { + *meas = transforms::apply_filter_params_activity_to_symbol(meas, &all_filters)?; + } + // A column renders wherever its symbol does, which includes the symbols + // a query reaches only through a filter, a segment or an order item. + for filter_item in self.dimensions_filters.iter_mut() { + *filter_item = + transforms::apply_filter_params_activity_to_filter_item(filter_item, &all_filters)?; + } + for filter_item in self.measures_filters.iter_mut() { + *filter_item = + transforms::apply_filter_params_activity_to_filter_item(filter_item, &all_filters)?; + } + for filter_item in self.time_dimensions_filters.iter_mut() { + *filter_item = + transforms::apply_filter_params_activity_to_filter_item(filter_item, &all_filters)?; + } + for filter_item in self.segments.iter_mut() { + *filter_item = + transforms::apply_filter_params_activity_to_filter_item(filter_item, &all_filters)?; + } + for order_item in self.order_by.iter_mut().flatten() { + order_item.member_evaluator = transforms::apply_filter_params_activity_to_symbol( + &order_item.member_evaluator, + &all_filters, + )?; + } for dim in self.dimensions.iter_mut() { *dim = transforms::apply_static_filter_to_symbol(dim, &dimensions_filters)?; } @@ -395,14 +436,20 @@ impl QueryProperties { /// Concatenation of `time_dimensions_filters`, `dimensions_filters`, and /// `segments` into a single `Filter`. `measures_filters` are not included. - pub fn all_filters(&self) -> Option { - let items = self - .time_dimensions_filters + /// `time_dimensions_filters`, `dimensions_filters` and `segments` as a flat + /// list. `measures_filters` are HAVING-style and stay out. + pub fn all_filter_items(&self) -> Vec { + self.time_dimensions_filters .iter() .chain(self.dimensions_filters.iter()) .chain(self.segments.iter()) .cloned() - .collect_vec(); + .collect_vec() + } + + /// The same set as `all_filter_items`, as a single `Filter`. + pub fn all_filters(&self) -> Option { + let items = self.all_filter_items(); if items.is_empty() { None } else { diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_call.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_call.rs index 76115c9cc4437..b7aee3eb3417f 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_call.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_call.rs @@ -95,6 +95,8 @@ impl SqlDependency { /// - `fp:N` — Nth filter param. /// - `fg:N` — Nth filter group. /// - `sv:N` — Nth security-context value. +/// - `fpv:N` — Nth filter value, supplied when a compiled +/// `FILTER_PARAMS` column callback is rendered. pub struct SqlCallArg; impl SqlCallArg { @@ -102,6 +104,7 @@ impl SqlCallArg { const FILTER_PARAM_PREFIX: &'static str = "fp"; const FILTER_GROUP_PREFIX: &'static str = "fg"; const SECURITY_VALUE_PREFIX: &'static str = "sv"; + const FILTER_VALUE_PREFIX: &'static str = "fpv"; pub fn dependency(i: usize) -> String { format!("{{{}:{}}}", Self::ARG_PREFIX, i) @@ -124,6 +127,17 @@ impl SqlCallArg { pub struct SqlCallFilterParamsItem { pub filter_symbol_name: String, pub column: FilterParamsColumn, + /// The column callback compiled into a call of its own, when the + /// data model gave one. Its placeholders index its own + /// dependencies, so it is rendered on its own rather than spliced + /// into the enclosing template. + pub compiled_call: Option>, + /// Whether the query this call is being planned for filters the + /// member this binding names. Only then does the column render, so + /// only then do the members it reads belong to the enclosing + /// member's dependencies — and only then may they pull a cube into + /// the join. + pub active: bool, } /// `FILTER_GROUP` binding from the data-model SQL: several @@ -195,6 +209,20 @@ impl SqlCall { node_processor: Rc, query_tools: Rc, templates: &PlanSqlTemplates, + ) -> Result { + self.eval_with_filter_values(visitor, node_processor, query_tools, templates, &[]) + } + + /// Renders the call with the filter values a compiled + /// `FILTER_PARAMS` column takes through its `{fpv:N}` + /// placeholders. + pub fn eval_with_filter_values( + &self, + visitor: &SqlEvaluatorVisitor, + node_processor: Rc, + query_tools: Rc, + templates: &PlanSqlTemplates, + filter_values: &[String], ) -> Result { if let SqlTemplate::String(template) = &self.template { let (filter_params, filter_groups, deps, context_values) = @@ -206,6 +234,7 @@ impl SqlCall { &filter_params, &filter_groups, &context_values, + filter_values, ) } else { Err(CubeError::internal( @@ -237,6 +266,7 @@ impl SqlCall { &filter_params, &filter_groups, &context_values, + &[], )?] } SqlTemplate::StringVec(templates) => templates @@ -248,6 +278,7 @@ impl SqlCall { &filter_params, &filter_groups, &context_values, + &[], ) }) .collect::, _>>()?, @@ -349,13 +380,27 @@ impl SqlCall { let filter_params = filter_params .iter() .map(|s| { - Self::substitute_template(s, &deps, &filter_params, &filter_groups, &context_values) + Self::substitute_template( + s, + &deps, + &filter_params, + &filter_groups, + &context_values, + &[], + ) }) .collect::, _>>()?; let filter_groups = filter_groups .iter() .map(|s| { - Self::substitute_template(s, &deps, &filter_params, &filter_groups, &context_values) + Self::substitute_template( + s, + &deps, + &filter_params, + &filter_groups, + &context_values, + &[], + ) }) .collect::, _>>()?; @@ -363,13 +408,14 @@ impl SqlCall { } /// Substitute placeholders in template string with computed values in a single pass - /// Supports placeholders: {arg:N}, {fp:N}, {fg:N}, {sv:N} + /// Supports placeholders: {arg:N}, {fp:N}, {fg:N}, {sv:N}, {fpv:N} fn substitute_template( template: &str, deps: &[String], filter_params: &[String], filter_groups: &[String], security_values: &[String], + filter_values: &[String], ) -> Result { let mut result = String::with_capacity(template.len()); let mut chars = template.chars().peekable(); @@ -404,6 +450,7 @@ impl SqlCall { SqlCallArg::FILTER_PARAM_PREFIX => filter_params.get(idx), SqlCallArg::FILTER_GROUP_PREFIX => filter_groups.get(idx), SqlCallArg::SECURITY_VALUE_PREFIX => security_values.get(idx), + SqlCallArg::FILTER_VALUE_PREFIX => filter_values.get(idx), _ => { result.push('{'); result.push_str(&placeholder); @@ -456,7 +503,7 @@ impl SqlCall { let mut filter_params_columns = HashMap::new(); for itm in items { filter_params_columns - .insert(itm.filter_symbol_name.clone(), itm.column.clone()); + .insert(itm.filter_symbol_name.clone(), (*itm).clone()); } let context = VisitorContext::new_for_filter_params( @@ -515,6 +562,25 @@ impl SqlCall { .collect() } + fn active_filter_params(&self) -> impl Iterator { + self.filter_params + .iter() + .chain( + self.filter_groups + .iter() + .flat_map(|g| g.filter_params.iter()), + ) + .filter(|item| item.active) + } + + fn filter_params_mut(&mut self) -> impl Iterator { + self.filter_params.iter_mut().chain( + self.filter_groups + .iter_mut() + .flat_map(|g| g.filter_params.iter_mut()), + ) + } + pub fn struct_eq(&self, other: &Self) -> bool { self.template == other.template && self.deps.len() == other.deps.len() @@ -540,6 +606,12 @@ impl SymbolDeps for Rc { SqlDependency::CubeRef(cr) => visitor.cube_ref(cr)?, } } + // An active column renders, so what it reads is read by this call too. + for item in self.active_filter_params() { + if let Some(call) = &item.compiled_call { + call.visit_deps(visitor)?; + } + } std::ops::ControlFlow::Continue(()) } @@ -550,6 +622,19 @@ impl SymbolDeps for Rc { visitor.symbol(s)?; } } + // Reached whatever the activity, so a rewrite never leaves an inactive + // column holding a symbol every other reference to it has replaced. + for item in call.filter_params.iter_mut() { + visitor.filter_params_group(std::slice::from_mut(item))?; + } + for group in call.filter_groups.iter_mut() { + visitor.filter_params_group(&mut group.filter_params)?; + } + for item in call.filter_params_mut() { + if let Some(call) = &mut item.compiled_call { + call.visit_deps_mut(visitor)?; + } + } *self = Rc::new(call); Ok(()) } @@ -613,6 +698,7 @@ impl crate::utils::debug::DebugSql for SqlCall { &filter_params, &filter_groups, &context_values, + &[], ) .unwrap() } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_call_builder.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_call_builder.rs index 67fe0133df560..80028ec328784 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_call_builder.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_call_builder.rs @@ -19,6 +19,11 @@ pub struct SqlCallBuilder<'a> { cube_evaluator: Rc, base_tools: Rc, security_context: Rc, + /// Set while compiling a cube's own `sql`. That sql builds the table the + /// query reads from, so no member is in scope inside it — a member + /// reference there is rejected instead of resolved, wherever in the sql or + /// in one of its `FILTER_PARAMS` columns it appears. + is_cube_sql: bool, } impl<'a> SqlCallBuilder<'a> { @@ -33,9 +38,15 @@ impl<'a> SqlCallBuilder<'a> { cube_evaluator, base_tools, security_context, + is_cube_sql: false, } } + pub fn for_cube_sql(mut self) -> Self { + self.is_cube_sql = true; + self + } + pub fn build( mut self, cube_name: &String, @@ -46,54 +57,80 @@ impl<'a> SqlCallBuilder<'a> { self.security_context.clone(), member_sql.args_names().clone(), )?; - let (template, template_args) = (compiled.template, compiled.args); + self.build_from_template(cube_name, compiled.template, &compiled.args) + } - let deps = template_args + // Assembles a `SqlCall` from an already-compiled template and the + // dependencies it recorded. Recurses for a `FILTER_PARAMS` column that came + // back compiled, since such a column is a call of its own with its own + // dependency list. + fn build_from_template( + &mut self, + cube_name: &String, + template: SqlTemplate, + args: &SqlTemplateArgs, + ) -> Result { + let deps = args .symbol_paths .iter() .map(|path| self.build_dependency(cube_name, path)) .collect::, _>>()?; - let filter_params = template_args + let filter_params = args .filter_params .iter() - .map(|itm| self.build_filter_params_item(itm)) + .map(|itm| self.build_filter_params_item(cube_name, itm)) .collect::, _>>()?; - let filter_groups = template_args + let filter_groups = args .filter_groups .iter() - .map(|itm| self.build_filter_group_item(itm)) + .map(|itm| self.build_filter_group_item(cube_name, itm)) .collect::, _>>()?; - let result = SqlCall::new( - template.clone(), + Ok(SqlCall::new( + template, deps, filter_params, filter_groups, - template_args.security_context.clone(), - ); - Ok(result) + args.security_context.clone(), + )) } fn build_filter_params_item( &mut self, + cube_name: &String, item: &FilterParamsItem, ) -> Result { + let compiled_call = match &item.column { + FilterParamsColumn::Compiled(compiled) => Some(Rc::new(self.build_from_template( + cube_name, + compiled.template.clone(), + &compiled.args, + )?)), + _ => None, + }; + Ok(SqlCallFilterParamsItem { filter_symbol_name: format!("{}.{}", item.cube_name, item.name), column: item.column.clone(), + compiled_call, + // Turned on per query, and again per subquery, once the filters are + // known. Off until then, so a symbol compiled for comparison rather + // than for a query carries no dependency the query would not. + active: false, }) } fn build_filter_group_item( &mut self, + cube_name: &String, item: &FilterGroupItem, ) -> Result { let filter_params = item .filter_params .iter() - .map(|itm| self.build_filter_params_item(itm)) + .map(|itm| self.build_filter_params_item(cube_name, itm)) .collect::, _>>()?; Ok(SqlCallFilterGroupItem { filter_params }) } @@ -112,6 +149,20 @@ impl<'a> SqlCallBuilder<'a> { ) .map_err(|e| CubeError::user(format!("Error in `{}`: {}", dep_path.join("."), e)))?; + if self.is_cube_sql { + if let SymbolPathType::Dimension | SymbolPathType::Measure | SymbolPathType::Segment = + symbol_path.path_type() + { + return Err(CubeError::user(format!( + "`sql` of cube `{}` references member `{}`. A cube's sql builds the table the \ + query reads from, so no member is in scope there — reference the underlying \ + column instead", + current_cube_name, + symbol_path.full_name() + ))); + } + } + let path = symbol_path.path().clone(); match symbol_path.path_type() { diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/cube_symbol.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/cube_symbol.rs index a47a0926582aa..37ed3f9cee3a7 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/cube_symbol.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/cube_symbol.rs @@ -194,7 +194,7 @@ impl CubeTableSymbolFactory { is_table_sql, } = self; let sql = if let Some(sql) = sql { - Some(compiler.compile_sql_call(&cube_name, sql)?) + Some(compiler.compile_cube_sql_call(&cube_name, sql)?) } else { None }; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/deps.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/deps.rs index 747da036d8111..b35371a20491d 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/deps.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/deps.rs @@ -38,6 +38,17 @@ pub trait DepVisitor { /// member-symbol slot and may replace it. pub trait DepVisitorMut { fn symbol(&mut self, slot: &mut Rc) -> Result<(), CubeError>; + + /// The `FILTER_PARAMS` bindings reached on the way, grouped the way + /// they are rendered: a `FILTER_GROUP` arrives as one slice, a + /// standalone binding as a slice of one. Bindings that contribute no + /// dependencies are handed over all the same. + fn filter_params_group( + &mut self, + _items: &mut [crate::planner::SqlCallFilterParamsItem], + ) -> Result<(), CubeError> { + Ok(()) + } } /// A node whose dependency slots can be walked (read) or rebuilt diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/transforms/static_filter.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/transforms/static_filter.rs index 7cd890964e51b..662ef77e3eb33 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/transforms/static_filter.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/transforms/static_filter.rs @@ -2,6 +2,8 @@ use super::super::common::Case; use super::super::dimension_kinds::DimensionKind; use super::super::{DimensionSymbol, MeasureSymbol, MemberSymbol}; use crate::planner::filter::{Filter, FilterGroup, FilterGroupOperator, FilterItem}; +use crate::planner::symbols::deps::{DepVisitorMut, SymbolDeps}; +use crate::planner::SqlCallFilterParamsItem; use cubenativeutils::CubeError; use std::rc::Rc; @@ -93,3 +95,67 @@ fn replace_dimension_case(dimension: &DimensionSymbol, new_case: Case) -> Rc, + filters: &FilterItem, +) -> Result, CubeError> { + let mut visitor = ActivityVisitor { filters }; + let mut result = symbol.as_ref().clone(); + result.visit_deps_mut(&mut visitor)?; + Ok(Rc::new(result)) +} + +/// The filter set as the one item `apply_filter_params_activity_to_symbol` +/// matches against. +pub fn filter_params_activity_filters(filters: &[FilterItem]) -> FilterItem { + FilterItem::Group(Rc::new(FilterGroup { + operator: FilterGroupOperator::And, + items: filters.to_vec(), + })) +} + +struct ActivityVisitor<'a> { + filters: &'a FilterItem, +} + +impl DepVisitorMut for ActivityVisitor<'_> { + fn symbol(&mut self, slot: &mut Rc) -> Result<(), CubeError> { + let mut symbol = slot.as_ref().clone(); + symbol.visit_deps_mut(self)?; + *slot = Rc::new(symbol); + Ok(()) + } + + fn filter_params_group( + &mut self, + items: &mut [SqlCallFilterParamsItem], + ) -> Result<(), CubeError> { + // Matched against the whole group, since that is the predicate the group + // renders: an OR group survives only when every member of it matches. + let members = items + .iter() + .map(|item| &item.filter_symbol_name) + .collect::>(); + let active = self.filters.find_subtree_for_members(&members).is_some(); + for item in items.iter_mut() { + item.active = active; + } + Ok(()) + } +} + +/// `apply_filter_params_activity_to_symbol` over every symbol a filter item +/// carries. +pub fn apply_filter_params_activity_to_filter_item( + filter_item: &FilterItem, + filters: &FilterItem, +) -> Result { + super::map_filter_item_symbols(filter_item, &|symbol| { + apply_filter_params_activity_to_symbol(symbol, filters) + }) +} diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/visitor_context.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/visitor_context.rs index d5718c2295ff5..71c3c97dbd93c 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/visitor_context.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/visitor_context.rs @@ -1,10 +1,10 @@ -use crate::cube_bridge::member_sql::FilterParamsColumn; +use crate::planner::sql_call::SqlCallFilterParamsItem; use std::collections::HashMap; #[derive(Default)] pub struct FiltersContext { pub use_local_tz: bool, - pub filter_params_columns: HashMap, + pub filter_params_columns: HashMap, /// True when members resolve to pre-aggregation columns (a rollup read). A /// segment is then a stored boolean column, which some dialects can't use /// as a bare predicate (e.g. MSSQL `BIT` needs `= 1`). diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_filter_params_callback.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_filter_params_callback.rs new file mode 100644 index 0000000000000..82d4c36380a4a --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mock_filter_params_callback.rs @@ -0,0 +1,62 @@ +use crate::cube_bridge::filter_params_callback::FilterParamsCallback; +use cubenativeutils::wrappers::NativeContextHolderRef; +use cubenativeutils::CubeError; +use std::any::Any; +use std::rc::Rc; + +/// Mock `FILTER_PARAMS.….filter(callback)` column: holds the SQL the data +/// model's callback would produce, with `%N` marking the Nth filter value the +/// planner passes in at render time. Member references are already rendered as +/// `{arg:N}` placeholders by `MockMemberSql`, indexing the same dependency list +/// as the surrounding template. +pub struct MockFilterParamsCallback { + template: String, +} + +impl MockFilterParamsCallback { + pub fn new(template: impl Into) -> Self { + Self { + template: template.into(), + } + } +} + +impl FilterParamsCallback for MockFilterParamsCallback { + fn call(&self, filter_params: &Vec) -> Result { + let mut result = self.template.clone(); + // Highest index first: `%1` is a prefix of `%10`, so substituting it + // first would corrupt the two-digit slot. + for (i, param) in filter_params.iter().enumerate().rev() { + result = result.replace(&format!("%{}", i), param); + } + Ok(result) + } + + fn as_any(self: Rc) -> Rc { + self + } + + fn clone_to_context( + &self, + _context_ref: &dyn NativeContextHolderRef, + ) -> Result, CubeError> { + Ok(Rc::new(Self { + template: self.template.clone(), + })) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn substitutes_filter_values_by_index() { + let cb = MockFilterParamsCallback::new("{arg:0} >= %0 AND {arg:0} < %1"); + + assert_eq!( + cb.call(&vec!["$1".to_string(), "$2".to_string()]).unwrap(), + "{arg:0} >= $1 AND {arg:0} < $2" + ); + } +} 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 8afd5dd575806..6aa0d1836aae5 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 @@ -1,6 +1,8 @@ use crate::cube_bridge::member_sql::{ - CompiledMemberTemplate, MemberSql, SqlTemplate, SqlTemplateArgs, + CompiledMemberTemplate, FilterParamsColumn, FilterParamsItem, MemberSql, SqlTemplate, + SqlTemplateArgs, }; +use crate::test_fixtures::cube_bridge::MockFilterParamsCallback; use cubenativeutils::CubeError; use std::any::Any; use std::rc::Rc; @@ -148,6 +150,47 @@ impl MockMemberSql { continue; } + // `{FILTER_PARAMS:.:}` records a FILTER_PARAMS + // binding with a callback column and yields `{fp:N}`. Inside + // ``, `[path]` is a member reference — recorded into this + // template's own args, so the callback's output indexes the same + // dependency list — and `%N` stands for the Nth filter value the + // planner passes at render time. + if let Some(body) = path.strip_prefix("FILTER_PARAMS:") { + let (member, column) = body.split_once(':').ok_or_else(|| { + CubeError::user(format!( + "FILTER_PARAMS needs a `.:` body: {}", + body + )) + })?; + // The scanner above stops at the first `}`, so a column carrying + // one would have been cut short here. + if column.is_empty() || column.contains('{') { + return Err(CubeError::user(format!( + "FILTER_PARAMS column must be non-empty and reference members as `[path]`: {}", + column + ))); + } + let member_parts = member.split('.').collect::>(); + if member_parts.len() != 2 || member_parts.iter().any(|p| p.is_empty()) { + return Err(CubeError::user(format!( + "FILTER_PARAMS member must be `.`: {}", + member + ))); + } + let (cube_name, name) = (member_parts[0], member_parts[1]); + let column = Self::parse_column_references(column, &mut args, &mut args_names)?; + let index = args.insert_filter_params(FilterParamsItem { + cube_name: cube_name.to_string(), + name: name.to_string(), + column: FilterParamsColumn::Callback(Rc::new( + MockFilterParamsCallback::new(column), + )), + }); + result.push_str(&format!("{{fp:{}}}", index)); + continue; + } + // Parse the path and add to symbol_paths let path_parts: Vec = path.split('.').map(|s| s.to_string()).collect(); @@ -181,6 +224,46 @@ impl MockMemberSql { Ok((result, args, args_names)) } + + // Replaces every `[path.to.member]` in a callback column with the `{arg:N}` + // placeholder of its recorded path. + fn parse_column_references( + column: &str, + args: &mut SqlTemplateArgs, + args_names: &mut Vec, + ) -> Result { + let mut result = String::new(); + let mut rest = column; + + while let Some(open) = rest.find('[') { + result.push_str(&rest[..open]); + let after = &rest[open + 1..]; + let close = after.find(']').ok_or_else(|| { + CubeError::user(format!("Unclosed member reference in column: {}", column)) + })?; + + let path_parts: Vec = + after[..close].split('.').map(|s| s.to_string()).collect(); + if path_parts.iter().any(|p| p.is_empty()) { + return Err(CubeError::user(format!( + "Invalid member reference in column: {}", + column + ))); + } + + let arg_name = path_parts[0].clone(); + if !args_names.contains(&arg_name) { + args_names.push(arg_name); + } + let index = args.insert_symbol_path(path_parts); + result.push_str(&format!("{{arg:{}}}", index)); + + rest = &after[close + 1..]; + } + result.push_str(rest); + + Ok(result) + } } impl MockMemberSql { diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mod.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mod.rs index fae44573b99ef..de1fc6018a932 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mod.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/cube_bridge/mod.rs @@ -16,6 +16,7 @@ mod mock_dimension_definition; mod mock_driver_tools; mod mock_evaluator; mod mock_expression_struct; +mod mock_filter_params_callback; mod mock_geo_item; mod mock_granularity_definition; mod mock_join_definition; @@ -54,6 +55,7 @@ pub use mock_dimension_definition::MockDimensionDefinition; pub use mock_driver_tools::MockDriverTools; pub use mock_evaluator::MockCubeEvaluator; pub use mock_expression_struct::MockExpressionStruct; +pub use mock_filter_params_callback::MockFilterParamsCallback; pub use mock_geo_item::MockGeoItem; pub use mock_granularity_definition::MockGranularityDefinition; pub use mock_join_definition::MockJoinDefinition; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter_params_callback_column.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter_params_callback_column.rs new file mode 100644 index 0000000000000..1e92c37d5ddea --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/filter_params_callback_column.rs @@ -0,0 +1,113 @@ +use crate::test_fixtures::cube_bridge::MockSchema; +use crate::test_fixtures::test_utils::TestContext; +use indoc::indoc; + +// The measure filter's only content is a `FILTER_PARAMS` binding whose column is +// a callback, and the time dimension is referenced from inside that callback +// alone — nothing else in the filter's SQL mentions it. +fn schema() -> MockSchema { + MockSchema::from_yaml(indoc! {" + cubes: + - name: commission + sql: \"SELECT * FROM commission\" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: partner + type: string + sql: partner + - name: pricing_duration + type: string + sql: pricing_duration + - name: reconciliation_date + type: time + sql: reconciliation_date + measures: + - name: daily_mrr + type: sum + sql: \"{CUBE}.total\" + filters: + - sql: \"{CUBE.pricing_duration} = 'DAILY'\" + - sql: \"{FILTER_PARAMS:commission.reconciliation_date:[CUBE.reconciliation_date] >= %0 AND [CUBE.reconciliation_date] < %1}\" + "}) + .unwrap() +} + +const QUERY: &str = indoc! {" + measures: + - commission.daily_mrr + dimensions: + - commission.partner + time_dimensions: + - dimension: commission.reconciliation_date + granularity: month + dateRange: + - \"2025-07-01\" + - \"2026-06-30\" +"}; + +#[test] +fn measure_filter_callback_column_renders_the_referenced_member() { + let ctx = TestContext::new(schema()).unwrap(); + + let (sql, _) = ctx.build_sql_and_params(QUERY).unwrap(); + + // Both halves of the callback's own predicate, with its member reference + // resolved to the dimension's SQL. The query's own time filter renders `<=` + // against the same column, so the strict `<` is what pins this to the + // callback. + assert!( + sql.contains( + "\"commission\".reconciliation_date >= $1 AND \"commission\".reconciliation_date < $2" + ), + "the filter param predicate must carry the referenced column\nsql: {}", + sql + ); + assert!( + !sql.contains("{arg:"), + "no dependency placeholder may survive into the SQL\nsql: {}", + sql + ); +} + +// A cube's `sql` builds the table the query reads from, so no member is in scope +// inside it. The reference is reported without resolving it, which is also what +// keeps the cube table and the dimension from resolving each other in a loop. +#[test] +fn cube_sql_referencing_a_member_is_rejected() { + let schema = MockSchema::from_yaml(indoc! {" + cubes: + - name: commission + sql: \"SELECT * FROM commission WHERE {CUBE.reconciliation_date} IS NOT NULL\" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: reconciliation_date + type: time + sql: reconciliation_date + measures: + - name: count + type: count + "}) + .unwrap(); + + let err = TestContext::new(schema) + .and_then(|ctx| { + ctx.build_sql_and_params(indoc! {" + measures: + - commission.count + "}) + }) + .expect_err("a member reference in a cube's sql must be reported"); + + assert!( + err.message + .contains("references member `commission.reconciliation_date`"), + "unexpected error: {}", + err.message + ); +} diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/mod.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/mod.rs index 5cb0406fda012..fef9974441c84 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/mod.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/mod.rs @@ -6,6 +6,7 @@ mod cube_names_collector; mod date_filters; mod dimension_symbol; mod filter; +mod filter_params_callback_column; mod join_hints_collector; mod measure_symbol; mod member_expressions_on_views; From d47943417ef1f93bed376c406271e7e7ca11b465 Mon Sep 17 00:00:00 2001 From: waralexrom <108349432+waralexrom@users.noreply.github.com> Date: Fri, 7 Aug 2026 11:43:28 +0200 Subject: [PATCH 2/3] fix(tesseract): keep the query row set when a multi-stage filter directive drops a filter (#11483) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(tesseract): keep the query row set when a multi-stage filter directive drops a filter A `filter:` directive frees the inner aggregation stage from a query filter, which is its purpose. On the window path it freed the reported rows along with it, because a window has a single row set serving both roles: the rows it aggregates over and the rows it reports. The measure's CTE then contributed dimension keys the query had filtered out, and the FullKeyAggregate brought them back as result rows carrying NULL for every other measure. Revoke the window path for a member whose directive drops a filter the query restricts the grid by, so the JOIN-model assembles it instead: there the keys side carries the query's rows while the value side spans the widened set. Detection is anchored to the query's own filters — a filter an upstream multi-stage member introduced through its own `filter: include` narrows that member's view, not the result grid, so a descendant dropping it cannot widen anything. The comparison is member-aware because `FilterItem` equality ignores the member a filter targets, and a dropped filter would otherwise be cancelled out by an unrelated look-alike that survived. Also make the explicit-keys join null-safe. A NULL dimension key kept its place in the grid but lost the measure, since `NULL = NULL` is unknown; the shape that derives its keys from the measure refs was already null-safe. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): build the keys side when a multi-stage filter directive drops a query filter Revoking the window path restores the reported row set only for inodes whose grain reshape already demands a keys side. An inode that drops a query filter while keeping the parent grain has nothing to shrink, so it got no keys side and still reported the widened rows: the dimension values the filter excluded came back with NULL for every other measure. Gate the keys side on a dropped query filter as well. The condition it joins is the same one the grain case already satisfies — the measure side no longer enumerates the rows the inode has to report — the difference being that here the grid keeps every dimension and only the row count within it grows. A rollup that does not store the dropped filter's dimension can no longer serve such a query. That is the price of asking the right question: before, the filter was absent from the plan entirely, so the rollup answered a query the user had not made. Exempt a parent state with no dimensions. Its grid is a single row that no filter change can widen, and the keys assembly does not render a keys side without key dimensions. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): exempt Dimension and Rank inodes from the query-grid keys side Gating the keys side on a dropped query filter reached two inode kinds the grain-shrink gate never could, and neither can use it. `plan_for_cte_dimension_query` builds its FullKeyAggregate without a keys input at all, so a multi-stage dimension carrying a `filter:` directive planned a duplicate child subtree whose CTEs nothing then referenced. A Rank inode ranks within whatever its source carries. Feeding it the keys side shrinks the ranked population to the query grid, which leaves one row per partition and collapses every rank to 1 — trading a widened row set for wrong values. Rank needs its rows restricted after the window, which this assembly cannot express, so it keeps reporting the wider set. Move the member-aware filter-tree comparison to `planner/filter/tree_ops`, next to the other tree walkers and to the equality it compensates for, rather than keeping a private copy in the planner. Order the null-key test by every selected dimension: all of its rows tie on the one key it had, so the snapshot pinned whatever order the plan happened to emit. Cover the value side of a `keep_only` segment drop. With the dropped filter's dimension inside the query grid, the measure agrees with the plain one whichever way the directive resolves, so the existing snapshot cannot detect the directive regressing; grouping by status instead puts the two apart at 600 and 1400. Co-Authored-By: Claude Opus 5 (1M context) * test(tesseract): cover a rolling window over a dropped date filter A rolling window rewrites the date range it hands to its base state, extending it backwards by the trailing interval. The filter an inner multi-stage measure drops therefore no longer carries the query's own bounds, which raised the question of whether the row set could widen the way it does elsewhere. It cannot, and the reason is structural rather than incidental: a rolling window draws its rows from the time series built out of the query's `dateRange`, and its values through the frame condition derived from that same range. Neither depends on the leaf keeping a date filter, so dropping it leaves both the reported rows and the values bounded. Two snapshots pin that — one at the grain of the time dimension alone, one with a dimension and a range narrow enough that the trailing window reaches months the leaf can see and the query did not ask for. Record the reasoning behind the unconditional null-safe keys join as well: the data model carries no nullability information, and a dimension read through an outer join in the join graph arrives as NULL even from a column declared NOT NULL, so the comparison cannot be narrowed to the cases that need it. Co-Authored-By: Claude Opus 5 (1M context) * fix(tesseract): make the multi-stage CTE dedup key member-aware `eq_as_state` decides whether an already-planned CTE can serve a member, and it compared filter vectors with `FilterItem`'s own equality — which looks at a filter's operator and values but not at the member it restricts. Two states filtering different dimensions to the same value therefore counted as one. That let the keys side dedup onto the very CTE it exists to bound. A directive combining `exclude` with an `include` whose operator and values coincide on another member plans the widened measure child first; the keys child is then matched against it, compares equal, and comes back as the same CTE — so the keys grid is the widened grid and the row set is not held after all. Compare filters with the member-aware equality instead. Two tests come with it: the shape above, which returns the query's six rows rather than nine, and a rank measure with a filter directive, which pins the row set Rank keeps reporting and would fail loudly if Rank were dropped from the keys-side exemption. State the two remaining couplings where they are decided rather than only in tests: the exemptions now sit next to the flag revocation they belong with, and the query-membership check records that a filter whose values were rewritten on the way down reads as one the query never asked for — a rewriting path has to bound the row set by other means, as the rolling-window one does. Drop the `assertion_line` header from the snapshots this branch touches; most of the suite does without it, and it churns them on unrelated line shifts. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../postgres/multi-stage-grain.test.ts | 34 ++ .../keys_aggregate_strategy.rs | 22 +- .../src/planner/filter/tree_ops.rs | 27 ++ .../multi_stage/multi_stage_query_planner.rs | 135 ++++++-- .../src/planner/query_properties.rs | 24 +- .../common/integration_multi_stage.yaml | 91 ++++++ .../multi_stage/filter_directive.rs | 291 ++++++++++++++++++ ...keeps_row_set_when_member_not_grouped.snap | 12 + ...lookalike_include_keeps_query_row_set.snap | 12 + ...nd_filter_exclude_keeps_query_row_set.snap | 9 + ...rain_exclude_keeps_null_dimension_key.snap | 9 + ...keep_only_segment_drops_other_filters.snap | 4 +- ..._segment_value_ignores_dropped_filter.snap | 7 + ...ter_exclude_ranks_over_whole_universe.snap | 15 + ...lling_window_over_dropped_date_filter.snap | 7 + ...over_dropped_date_filter_by_dimension.snap | 9 + ...re_of_grand_total_keeps_query_row_set.snap | 9 + ...views__view_keep_only_segment_at_leaf.snap | 4 +- 18 files changed, 686 insertions(+), 35 deletions(-) create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__filter_exclude_keeps_row_set_when_member_not_grouped.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__filter_exclude_with_lookalike_include_keeps_query_row_set.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__grain_and_filter_exclude_keeps_query_row_set.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__grain_exclude_keeps_null_dimension_key.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__keep_only_segment_value_ignores_dropped_filter.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rank_with_filter_exclude_ranks_over_whole_universe.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rolling_window_over_dropped_date_filter.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rolling_window_over_dropped_date_filter_by_dimension.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__share_of_grand_total_keeps_query_row_set.snap diff --git a/packages/cubejs-schema-compiler/test/integration/postgres/multi-stage-grain.test.ts b/packages/cubejs-schema-compiler/test/integration/postgres/multi-stage-grain.test.ts index d7a44751903c4..80e5087c2c323 100644 --- a/packages/cubejs-schema-compiler/test/integration/postgres/multi-stage-grain.test.ts +++ b/packages/cubejs-schema-compiler/test/integration/postgres/multi-stage-grain.test.ts @@ -133,6 +133,22 @@ cubes: - orders.status include: - orders.id + + # ── grain.exclude + filter.exclude on the same dim ──────────── + # "Share of the whole universe" denominator: the value ignores both + # the query filter on status and the status partition. The result row + # set must stay the query's — dropping the filter may not resurrect + # rows the query filtered out. + - name: amount_grain_and_filter_exclude_status + sql: "{CUBE.total_amount}" + type: sum + multi_stage: true + grain: + exclude: + - orders.status + filter: + exclude: + - orders.status `); if (getEnv('nativeSqlPlanner')) { @@ -257,6 +273,23 @@ cubes: { orders__status: 'pending', orders__category: 'books', orders__total_amount: '50', orders__amount_grain_keep_status_include_id: '125' }, { orders__status: 'pending', orders__category: 'electronics', orders__total_amount: '75', orders__amount_grain_keep_status_include_id: '125' }, ], { joinGraph, cubeEvaluator, compiler })); + + // ── grain.exclude + filter.exclude on the same dim ──────────── + it('grain.exclude + filter.exclude: keeps the query row set', async () => dbRunner.runQueryTest({ + measures: ['orders.total_amount', 'orders.amount_grain_and_filter_exclude_status'], + dimensions: ['orders.status', 'orders.category'], + filters: [ + { member: 'orders.status', operator: 'equals', values: ['completed'] }, + ], + order: [{ id: 'orders.status' }, { id: 'orders.category' }], + timezone: 'UTC', + }, [ + // The denominator is the per-category total over every status (books=250, + // electronics=275), while the rows stay the filtered ones — no pending or + // cancelled row may come back through the join. + { orders__status: 'completed', orders__category: 'books', orders__total_amount: '170', orders__amount_grain_and_filter_exclude_status: '250' }, + { orders__status: 'completed', orders__category: 'electronics', orders__total_amount: '200', orders__amount_grain_and_filter_exclude_status: '275' }, + ], { joinGraph, cubeEvaluator, compiler })); } else { // These tests rely on Tesseract; v1 planner does not implement the directive. test.skip('exclude: drops a dim from the partition', () => { expect(1).toBe(1); }); @@ -267,5 +300,6 @@ cubes: test.skip('keep_only: two-element array narrows to the (status, category) cell', () => { expect(1).toBe(1); }); test.skip('include: two-element array extends the leaf grain', () => { expect(1).toBe(1); }); test.skip('keep_only + include: keep narrows, include extends', () => { expect(1).toBe(1); }); + test.skip('grain.exclude + filter.exclude: keeps the query row set', () => { expect(1).toBe(1); }); } }); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan_builder/processors/full_key_aggregate/keys_aggregate_strategy.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan_builder/processors/full_key_aggregate/keys_aggregate_strategy.rs index 4374f24f1a13b..15314e04caa1e 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan_builder/processors/full_key_aggregate/keys_aggregate_strategy.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan_builder/processors/full_key_aggregate/keys_aggregate_strategy.rs @@ -175,14 +175,26 @@ impl FullKeyAggregateStrategy for KeysFullKeyAggregateStrategy<'_> { }) .collect::, _>>()?; - // Null-safe dimension join when keys are derived from the - // measure refs (FULL JOIN-equivalent shape). For the JOIN-model - // path (explicit keys) it's a one-to-one match — null-safety is - // unnecessary but stays correct. + // Both shapes join on the null-safe comparison, unconditionally. + // A plain `=` never matches NULL to NULL, so a key row whose + // dimension is NULL would keep its place in the grid and lose the + // measure. + // + // It has to be unconditional because nothing here can rule NULL + // out. The data model carries no nullability information, and even + // a column declared NOT NULL reaches this join as NULL once the + // dimension is read through an outer join in the cube's join + // graph — so the grid can hold a NULL key whatever the source + // table says. + // + // The cost is the planner's ability to hash- or merge-join these + // keys on engines that treat the null-safe operator as unhashable. + // Correctness is not tradeable against it: the alternative drops a + // measure value silently, on a row that is still returned. join_builder.left_join_subselect( query, query_alias, - JoinCondition::new_dimension_join(conditions, !has_explicit_keys), + JoinCondition::new_dimension_join(conditions, true), ); } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/filter/tree_ops.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/filter/tree_ops.rs index 84638d735c939..6933a2327c47a 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/filter/tree_ops.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/filter/tree_ops.rs @@ -102,3 +102,30 @@ pub fn has_filter_for_member(member_name: &String, filters: &[FilterItem]) -> bo } false } + +/// Structural equality that also compares the member each leaf filter targets. +/// `FilterItem`'s own `PartialEq` compares a filter's type, operator and values +/// but not its member, so two filters differing only in the dimension they +/// restrict count as equal there. Groups are compared element-wise in order. +/// Segments carry their member in `full_name` and compare as-is. +pub fn eq_with_member(a: &FilterItem, b: &FilterItem) -> bool { + match (a, b) { + (FilterItem::Item(a), FilterItem::Item(b)) => a.member_name() == b.member_name() && a == b, + (FilterItem::Group(a), FilterItem::Group(b)) => { + a.operator == b.operator + && a.items.len() == b.items.len() + && a.items + .iter() + .zip(b.items.iter()) + .all(|(a, b)| eq_with_member(a, b)) + } + _ => a == b, + } +} + +/// True when `items` holds a filter equal to `item` under [`eq_with_member`]. +pub fn contains_with_member(items: &[FilterItem], item: &FilterItem) -> bool { + items + .iter() + .any(|candidate| eq_with_member(item, candidate)) +} diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/multi_stage_query_planner.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/multi_stage_query_planner.rs index 5105653f32b43..a5befaa8c1136 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/multi_stage_query_planner.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/multi_stage_query_planner.rs @@ -9,6 +9,7 @@ use crate::logical_plan::*; use crate::planner::collectors::has_multi_stage_members; use crate::planner::collectors::member_childs; use crate::planner::filter::base_filter::FilterType; +use crate::planner::filter::tree_ops; use crate::planner::filter::BaseFilter; use crate::planner::filter::FilterItem; use crate::planner::filter::FilterOperator; @@ -177,12 +178,16 @@ impl MultiStageQueryPlanner { .multi_stage() .map(|ms| ms.grain.clone()) .unwrap_or_default(); - // Window-path eligibility intentionally checks only `include`: - // `exclude` and `keep_only` are realised through the window's - // PARTITION BY at render time, so they don't disqualify the - // path. `include` extends the leaf grain, which the JOIN-model - // is required for. Revisit if window-path expands to cases - // where exclude/keep_only affect render correctness. + // Of the `grain` lists only `include` disqualifies the window path + // here: `exclude` and `keep_only` are realised through the window's + // PARTITION BY at render time, while `include` extends the leaf + // grain, which the JOIN-model is required for. + // + // Grain is not the only disqualifier. A `filter` directive that + // drops a filter the query restricts the grid by also rules the + // window path out. Detecting that needs the inherited state, which + // only exists further down in `make_queries_descriptions` — the + // flag is revoked there. let has_include = grain.include.as_ref().is_some_and(|v| !v.is_empty()); let use_window_path = matches!(member_type, MultiStageInodeMemberType::Aggregate) && !has_include @@ -525,7 +530,7 @@ impl MultiStageQueryPlanner { alias.clone(), ) } else { - let (multi_stage_member, is_ungrupped) = self + let (mut multi_stage_member, is_ungrupped) = self .create_multi_stage_inode_member(member.clone(), resolved_multi_stage_dimensions)?; let mut dimensions_to_add = multi_stage_member @@ -557,16 +562,55 @@ impl MultiStageQueryPlanner { // The window-path Aggregate inode skips step 2: the leaf stays // at the parent state plus any `include` extension, and the // window function does the `exclude` collapse at outer level. - let use_window_path = multi_stage_member.use_window_path(); - let new_state = { - let mut new_state = match directive_filter.as_ref().map(|f| &f.mode) { + let filtered_state = { + let mut filtered_state = match directive_filter.as_ref().map(|f| &f.mode) { Some(MultiStageFilterMode::Fixed) => self.root_state().as_ref().clone(), Some(MultiStageFilterMode::Relative) | None => state.as_ref().clone(), }; if let Some(filter) = &directive_filter { - apply_filter_directive_to_state(filter, &mut new_state); + apply_filter_directive_to_state(filter, &mut filtered_state); } + filtered_state + }; + + // Step 1 can drop a filter the query restricts the grid by. That is + // the point of the directive — the aggregation input widens — but + // the rows the inode *reports* must stay the query's, and only the + // JOIN-model can hold the two apart: its keys side enumerates the + // grid while its measure side spans the widened set. A window + // expression has one row set serving both roles, so it reports the + // widened rows too and values the query filtered out come back as + // result rows. Hand such an inode to the JOIN-model. + let query_filter_dropped = + query_filters_dropped(self.root_state(), &state, &filtered_state); + if query_filter_dropped { + multi_stage_member = multi_stage_member.with_use_window_path(false); + } + + // Whether this inode can actually act on that, decided here so both + // halves of the decision stay together — the keys side that carries + // the query's rows is requested further down. + // + // A parent state with no dimensions is a single-row grid that no + // filter change can widen, and an empty key-dimension list + // degenerates into a cross join rather than being rejected. A + // Dimension inode never reads `keys_input`, so building one leaves + // unreferenced CTEs behind. And a Rank inode ranks within whatever + // its source carries: handing it the keys side would shrink the + // ranked population to the grid and collapse the ranks, trading a + // widened row set for wrong values. Rank needs its rows restricted + // *after* the window, which this assembly cannot express. + let needs_query_grid = query_filter_dropped + && (!state.dimensions().is_empty() || !state.time_dimensions().is_empty()) + && !matches!( + multi_stage_member.inode_type(), + MultiStageInodeMemberType::Dimension | MultiStageInodeMemberType::Rank + ); + + let use_window_path = multi_stage_member.use_window_path(); + let new_state = { + let mut new_state = filtered_state; if !use_window_path && matches!( @@ -602,14 +646,15 @@ impl MultiStageQueryPlanner { scope, )?; - // JOIN-model: when new_state misses any dim that was on the - // parent's `state`, this inode shrinks the parent grain. We - // build keys-side descriptions per child on the parent state - // so the FullKeyAggregate can broadcast measure values back - // to the full query grain. Window-path Aggregate inodes - // (sum-of-sum / sum-of-count with no leaf-extending `include`) - // handle broadcast via the window expression instead and don't - // need keys_input. + // JOIN-model: when the measure side no longer enumerates the rows + // this inode has to report — because new_state misses a dim that + // was on the parent's `state`, or because a dropped query filter + // widened it — we build keys-side descriptions per child on the + // parent state, so the FullKeyAggregate broadcasts measure values + // onto the query grain and only onto it. Window-path Aggregate + // inodes (sum-of-sum / sum-of-count with no leaf-extending + // `include`) handle broadcast via the window expression instead and + // don't need keys_input. let mut keys_input: Vec> = vec![]; if !use_window_path { let new_state_has = |sym: &Rc| { @@ -625,7 +670,11 @@ impl MultiStageQueryPlanner { .iter() .chain(state.time_dimensions().iter()) .any(|d| !new_state_has(d)); - if any_missing { + // A dropped query filter needs the keys side for the same + // reason a shrunk grain does — the measure side no longer + // enumerates the query's rows — except here the grid keeps + // every dimension and only the row count within it grows. + if any_missing || needs_query_grid { self.make_childs( member.clone(), state.clone(), @@ -1097,6 +1146,52 @@ fn filter_directive_match_names(symbol: &Rc) -> Vec { } } +// True when `narrowed` lost a *query-level* filter that `base` restricts the +// grid by. Three conditions per filter: `base` has it, the query asked for it, +// and `narrowed` doesn't have it. +// +// The query membership check is what keeps the notion anchored to the result +// grid. A filter a parent multi-stage member introduced through its own +// `filter: include` narrows that parent's view, not the grid the query asked +// for; a child dropping it (`mode: fixed`) therefore cannot widen the grid +// past the query, and needs no keys side. +// +// Measure filters are absent from the comparison because they never reach a +// CTE state to begin with — `build_root_state` drops them — so there is no +// query-level measure filter for a directive to lose. Filters added on top of +// `base` don't count either: they shrink the grid, which is safe. +// +// The query-membership check compares whole filters, so a filter whose values +// were rewritten between the root state and `base` reads as one the query never +// asked for, and the drop goes undetected. That is deliberately out of scope: a +// path that rewrites filter values on the way down has to bound the row set by +// other means. The rolling-window date-range rewrite is the one such path, and +// it does — its rows come from the time series and its values through the frame +// condition, both built from the query's own range. A new rewriting path has to +// establish the same, or anchor this check on the member instead of the value. +fn query_filters_dropped( + root: &QueryProperties, + base: &QueryProperties, + narrowed: &QueryProperties, +) -> bool { + fn any_dropped(root: &[FilterItem], base: &[FilterItem], narrowed: &[FilterItem]) -> bool { + base.iter().any(|item| { + tree_ops::contains_with_member(root, item) + && !tree_ops::contains_with_member(narrowed, item) + }) + } + + any_dropped( + root.dimensions_filters(), + base.dimensions_filters(), + narrowed.dimensions_filters(), + ) || any_dropped( + root.time_dimensions_filters(), + base.time_dimensions_filters(), + narrowed.time_dimensions_filters(), + ) || any_dropped(root.segments(), base.segments(), narrowed.segments()) +} + fn apply_filter_directive_to_state(filter: &MultiStageFilter, state: &mut QueryProperties) { if let Some(exclude) = &filter.exclude { let names: Vec = exclude diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_properties.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_properties.rs index 6bedacca36d62..2633890570c8e 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_properties.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/query_properties.rs @@ -1152,15 +1152,31 @@ impl QueryProperties { /// Equality over members (chain-resolved), the three filter slots, /// segments and time-shifts. Excludes ordering, limits, planner flags /// and join hints; for those fields use the full [`PartialEq`]. + /// + /// Filters are compared with [`tree_ops::eq_with_member`] rather than with + /// `FilterItem`'s own equality, which looks at a filter's operator and + /// values but not at the member it restricts. Two states filtering + /// different dimensions to the same value are different states, and + /// conflating them makes a CTE serve a filter it was never built for. pub fn eq_as_state(&self, other: &Self) -> bool { Self::members_equivalent(&self.dimensions, &other.dimensions) + && Self::filters_equivalent(&self.dimensions_filters, &other.dimensions_filters) && Self::members_equivalent(&self.time_dimensions, &other.time_dimensions) - && self.dimensions_filters == other.dimensions_filters - && self.time_dimensions_filters == other.time_dimensions_filters - && self.measures_filters == other.measures_filters - && self.segments == other.segments + && Self::filters_equivalent( + &self.time_dimensions_filters, + &other.time_dimensions_filters, + ) + && Self::filters_equivalent(&self.measures_filters, &other.measures_filters) + && Self::filters_equivalent(&self.segments, &other.segments) && self.time_shifts == other.time_shifts } + + fn filters_equivalent(a: &[FilterItem], b: &[FilterItem]) -> bool { + a.len() == b.len() + && a.iter() + .zip(b.iter()) + .all(|(a, b)| tree_ops::eq_with_member(a, b)) + } } /// Equality over every semantic field. Members are compared by reference- diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/integration_multi_stage.yaml b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/integration_multi_stage.yaml index 0dad048dda9c7..cfae8627de248 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/integration_multi_stage.yaml +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/integration_multi_stage.yaml @@ -125,6 +125,12 @@ cubes: else: sql: "{CUBE.status}" + # Every books row is NULL here, so a query grouping by it carries a + # NULL dimension key through the multi-stage assembly. + - name: category_nullable + type: string + sql: "NULLIF(category, 'books')" + - name: customer_name type: string sql: "{customers.name}" @@ -636,6 +642,91 @@ cubes: keep_only: - orders.status + # Both directives name `status`: the value ignores the query filter + # on it (`filter`) and collapses its partition (`grain`). The result + # row set must still be the query's. + - name: amount_grain_and_filter_exclude_status + type: sum + sql: "{CUBE.total_amount}" + multi_stage: true + grain: + exclude: + - orders.status + filter: + exclude: + - orders.status + + # Share-of-total shape: a grand-total denominator (grain drops every + # dimension) that also ignores the query filter on `status`, consumed + # by a ratio that coalesces a missing denominator to 0 — so a widened + # row set surfaces as extra rows reading 0 rather than NULL. + - name: amount_grand_total_unfiltered + type: sum + sql: "{CUBE.total_amount}" + multi_stage: true + grain: + exclude: + - orders.status + - orders.category + filter: + exclude: + - orders.status + + - name: amount_share_percent + type: number + multi_stage: true + sql: "coalesce(100 * {CUBE.total_amount} / nullif({CUBE.amount_grand_total_unfiltered}, 0), 0)" + + # A rolling window rewrites the query date range for its base state + # (extending it backwards by the trailing interval). The inner measure + # below drops that date filter, so detecting the drop must not depend + # on the filter still carrying the query's own bounds. + - name: amount_no_date_bound + type: sum + sql: "{CUBE.total_amount}" + multi_stage: true + filter: + exclude: + - orders.created_at + + - name: rolling_amount_no_date_bound + type: sum + sql: "{CUBE.amount_no_date_bound}" + multi_stage: true + rolling_window: + trailing: 3 month + + # The `include` predicate matches the dropped query filter on + # operator and values while naming a different member, so the CTE + # dedup key has to compare members to keep the two states apart. + - name: amount_exclude_status_include_lookalike + type: sum + sql: "{CUBE.total_amount}" + multi_stage: true + filter: + exclude: + - orders.status + include: + - member: orders.category + operator: notEquals + values: [pending] + + # A rank whose ordering ignores the query filter on `status` and whose + # partition drops it. Rank is deliberately left out of the keys side, + # so the ranks span the whole universe and the rows do too. + - name: category_rank_all_statuses + type: rank + multi_stage: true + grain: + exclude: + - orders.status + filter: + exclude: + - orders.status + order_by: + - sql: "{CUBE.total_amount}" + dir: desc + - name: amount_only_completed type: sum sql: "{CUBE.total_amount}" diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs index 1ea67ebdec057..2e5859b51b444 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs @@ -245,3 +245,294 @@ async fn test_mode_fixed_in_chain_diverges_from_relative() { insta::assert_snapshot!(result); } } + +// `grain.exclude` and `filter.exclude` naming the same dimension, which the +// query both groups by and filters on. The value ignores the filter and the +// partition; the row set stays the query's — the statuses the query filtered +// out may not reappear. +#[tokio::test(flavor = "multi_thread")] +async fn test_grain_and_filter_exclude_keeps_query_row_set() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.total_amount + - orders.amount_grain_and_filter_exclude_status + dimensions: + - orders.status + - orders.category + filters: + - dimension: orders.status + operator: equals + values: + - completed + order: + - id: orders.status + - id: orders.category + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// Share-of-total over a grand-total denominator that ignores the query filter. +// The denominator's own aggregation must span every status while the ratio is +// reported only for the rows the query asked for; a widened row set would show +// up as extra rows whose share coalesces to 0. +#[tokio::test(flavor = "multi_thread")] +async fn test_share_of_grand_total_keeps_query_row_set() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.amount_share_percent + dimensions: + - orders.status + - orders.category + filters: + - dimension: orders.status + operator: equals + values: + - completed + order: + - id: orders.status + - id: orders.category + "#}; + + let sql = ctx.build_sql(query).unwrap(); + // The window text comes from a `format!` in the physical plan, not from a + // dialect template, so `OVER (` is stable; whitespace is stripped only so + // the check does not depend on how the expression is laid out. + let dense: String = sql.chars().filter(|c| !c.is_whitespace()).collect(); + assert!( + !dense.contains("OVER("), + "the denominator has to hold the query's rows through a keys side, \ + which a window expression cannot do:\n{}", + sql + ); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// A NULL dimension value is a grid key like any other: the keys side and the +// measure side must still meet on it. +#[tokio::test(flavor = "multi_thread")] +async fn test_grain_exclude_keeps_null_dimension_key() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.total_amount + - orders.amount_grain_and_filter_exclude_status + dimensions: + - orders.status + - orders.category_nullable + filters: + - dimension: orders.status + operator: equals + values: + - completed + order: + - id: orders.status + - id: orders.category_nullable + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// The dropped filter's dimension is not one the query groups by, so the grain +// reshape has nothing to remove and the measure side keeps the full grid — yet +// the rows within that grid still widen. Grouping by the primary key makes it +// visible: only the six completed orders may appear. +#[tokio::test(flavor = "multi_thread")] +async fn test_filter_exclude_keeps_row_set_when_member_not_grouped() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.total_amount + - orders.amount_exclude_status + - orders.amount_grain_and_filter_exclude_status + dimensions: + - orders.id + filters: + - dimension: orders.status + operator: equals + values: + - completed + order: + - id: orders.id + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// `keep_only` on a segment, with the dropped filter's dimension outside the +// query grid so the value differs from the plain measure: the segment still +// restricts to completed orders while the category filter is gone, so the +// measure spans every category and the plain measure only books. +#[tokio::test(flavor = "multi_thread")] +async fn test_keep_only_segment_value_ignores_dropped_filter() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.total_amount + - orders.amount_keep_only_segment + dimensions: + - orders.status + segments: + - orders.completed_orders + filters: + - dimension: orders.category + operator: equals + values: + - books + order: + - id: orders.status + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// A rolling window rewrites the date range it hands to its base state, so the +// filter the inner measure drops no longer carries the query's own bounds. The +// row set survives that regardless: a rolling window takes its rows from the +// time series built out of the query's `dateRange`, and its values through the +// frame condition derived from the same range — neither depends on the leaf +// keeping its date filter. +#[tokio::test(flavor = "multi_thread")] +async fn test_rolling_window_over_dropped_date_filter() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.total_amount + - orders.rolling_amount_no_date_bound + time_dimensions: + - dimension: orders.created_at + granularity: month + dateRange: + - "2024-03-01" + - "2024-03-31" + order: + - id: orders.created_at + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// The same shape with a dimension and a range narrow enough that the trailing +// window reaches months the leaf can see but the query did not ask for. +#[tokio::test(flavor = "multi_thread")] +async fn test_rolling_window_over_dropped_date_filter_by_dimension() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.total_amount + - orders.rolling_amount_no_date_bound + dimensions: + - orders.category + time_dimensions: + - dimension: orders.created_at + granularity: month + dateRange: + - "2024-01-01" + - "2024-01-31" + order: + - id: orders.created_at + - id: orders.category + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// The directive's `include` predicate shares operator and values with the query +// filter it drops but names a different member. The keys side is planned on the +// inherited state, so a member-blind CTE dedup key would hand back the widened +// measure CTE and leave the row set widened. +#[tokio::test(flavor = "multi_thread")] +async fn test_filter_exclude_with_lookalike_include_keeps_query_row_set() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.total_amount + - orders.amount_exclude_status_include_lookalike + dimensions: + - orders.status + - orders.category + filters: + - dimension: orders.status + operator: notEquals + values: + - pending + order: + - id: orders.status + - id: orders.category + "#}; + + ctx.build_sql(query).unwrap(); + + if let Some(result) = ctx.try_execute_pg(query, SEED).await { + insta::assert_snapshot!(result); + } +} + +// Rank is out of the keys side on purpose: ranking within the query grid would +// leave one row per partition and collapse every rank to 1. The cost is that the +// statuses the query filtered out stay in the result, carrying a NULL for every +// measure that does honour the filter. Pins that trade rather than endorsing it. +#[tokio::test(flavor = "multi_thread")] +async fn test_rank_with_filter_exclude_ranks_over_whole_universe() { + let ctx = create_context(); + + let query = indoc! {r#" + measures: + - orders.total_amount + - orders.category_rank_all_statuses + dimensions: + - orders.status + - orders.category + filters: + - dimension: orders.status + operator: equals + values: + - completed + order: + - id: orders.status + - id: orders.category + "#}; + + 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/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__filter_exclude_keeps_row_set_when_member_not_grouped.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__filter_exclude_keeps_row_set_when_member_not_grouped.snap new file mode 100644 index 0000000000000..81ffe52b7ec36 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__filter_exclude_keeps_row_set_when_member_not_grouped.snap @@ -0,0 +1,12 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +expression: result +--- +orders__id | orders__total_amount | orders__amount_exclude_status | orders__amount_grain_and_filter_exclude_status +-----------+----------------------+-------------------------------+----------------------------------------------- +1 | 100.00 | 100.00 | 100.00 +2 | 200.00 | 200.00 | 200.00 +6 | 300.00 | 300.00 | 300.00 +7 | 200.00 | 200.00 | 200.00 +11 | 400.00 | 400.00 | 400.00 +12 | 200.00 | 200.00 | 200.00 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__filter_exclude_with_lookalike_include_keeps_query_row_set.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__filter_exclude_with_lookalike_include_keeps_query_row_set.snap new file mode 100644 index 0000000000000..455c6f13cd472 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__filter_exclude_with_lookalike_include_keeps_query_row_set.snap @@ -0,0 +1,12 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +expression: result +--- +orders__status | orders__category | orders__total_amount | orders__amount_exclude_status_include_lookalike +---------------+------------------+----------------------+------------------------------------------------ +cancelled | books | 100.00 | 100.00 +cancelled | clothing | 50.00 | 50.00 +cancelled | electronics | 50.00 | 50.00 +completed | books | 600.00 | 600.00 +completed | clothing | 300.00 | 300.00 +completed | electronics | 500.00 | 500.00 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__grain_and_filter_exclude_keeps_query_row_set.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__grain_and_filter_exclude_keeps_query_row_set.snap new file mode 100644 index 0000000000000..047115d46ed44 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__grain_and_filter_exclude_keeps_query_row_set.snap @@ -0,0 +1,9 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +expression: result +--- +orders__status | orders__category | orders__total_amount | orders__amount_grain_and_filter_exclude_status +---------------+------------------+----------------------+----------------------------------------------- +completed | books | 600.00 | 880.00 +completed | clothing | 300.00 | 720.00 +completed | electronics | 500.00 | 650.00 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__grain_exclude_keeps_null_dimension_key.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__grain_exclude_keeps_null_dimension_key.snap new file mode 100644 index 0000000000000..0f4ea3599ec4a --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__grain_exclude_keeps_null_dimension_key.snap @@ -0,0 +1,9 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +expression: result +--- +orders__status | orders__category_nullable | orders__total_amount | orders__amount_grain_and_filter_exclude_status +---------------+---------------------------+----------------------+----------------------------------------------- +completed | clothing | 300.00 | 720.00 +completed | electronics | 500.00 | 650.00 +completed | NULL | 600.00 | 880.00 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__keep_only_segment_drops_other_filters.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__keep_only_segment_drops_other_filters.snap index c2aa42fb27592..c334effc16da8 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__keep_only_segment_drops_other_filters.snap +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__keep_only_segment_drops_other_filters.snap @@ -5,6 +5,4 @@ expression: result --- orders__category | orders__total_amount | orders__amount_keep_only_segment -----------------+----------------------+--------------------------------- -books | 600.00 | 600.00 -clothing | NULL | 300.00 -electronics | NULL | 500.00 +books | 600.00 | 600.00 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__keep_only_segment_value_ignores_dropped_filter.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__keep_only_segment_value_ignores_dropped_filter.snap new file mode 100644 index 0000000000000..73b0d4be6ca1f --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__keep_only_segment_value_ignores_dropped_filter.snap @@ -0,0 +1,7 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +expression: result +--- +orders__status | orders__total_amount | orders__amount_keep_only_segment +---------------+----------------------+--------------------------------- +completed | 600.00 | 1400.00 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rank_with_filter_exclude_ranks_over_whole_universe.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rank_with_filter_exclude_ranks_over_whole_universe.snap new file mode 100644 index 0000000000000..70a302bae503f --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rank_with_filter_exclude_ranks_over_whole_universe.snap @@ -0,0 +1,15 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +expression: result +--- +orders__status | orders__category | orders__total_amount | orders__category_rank_all_statuses +---------------+------------------+----------------------+----------------------------------- +cancelled | books | NULL | 3 +cancelled | clothing | NULL | 3 +cancelled | electronics | NULL | 3 +completed | books | 600.00 | 1 +completed | clothing | 300.00 | 2 +completed | electronics | 500.00 | 1 +pending | books | NULL | 2 +pending | clothing | NULL | 1 +pending | electronics | NULL | 2 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rolling_window_over_dropped_date_filter.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rolling_window_over_dropped_date_filter.snap new file mode 100644 index 0000000000000..fe0993a8777e6 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rolling_window_over_dropped_date_filter.snap @@ -0,0 +1,7 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +expression: result +--- +orders__created_at_month | orders__total_amount | orders__rolling_amount_no_date_bound +-------------------------+----------------------+------------------------------------- +2024-03-01 00:00:00 | 1000.00 | 2250.00 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rolling_window_over_dropped_date_filter_by_dimension.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rolling_window_over_dropped_date_filter_by_dimension.snap new file mode 100644 index 0000000000000..2eaacac3e13d8 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__rolling_window_over_dropped_date_filter_by_dimension.snap @@ -0,0 +1,9 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +expression: result +--- +orders__category | orders__created_at_month | orders__total_amount | orders__rolling_amount_no_date_bound +-----------------+--------------------------+----------------------+------------------------------------- +books | 2024-01-01 00:00:00 | 230.00 | 230.00 +clothing | 2024-01-01 00:00:00 | 120.00 | 120.00 +electronics | 2024-01-01 00:00:00 | 150.00 | 150.00 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__share_of_grand_total_keeps_query_row_set.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__share_of_grand_total_keeps_query_row_set.snap new file mode 100644 index 0000000000000..aa86ff0523e33 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__filter_directive__share_of_grand_total_keeps_query_row_set.snap @@ -0,0 +1,9 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/filter_directive.rs +expression: result +--- +orders__status | orders__category | orders__amount_share_percent +---------------+------------------+----------------------------- +completed | books | 26.6666666666666667 +completed | clothing | 13.3333333333333333 +completed | electronics | 22.2222222222222222 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__views__view_keep_only_segment_at_leaf.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__views__view_keep_only_segment_at_leaf.snap index 890c172a1ec93..27996a5cdb088 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__views__view_keep_only_segment_at_leaf.snap +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/snapshots/cubesqlplanner__tests__integration__multi_stage__views__view_keep_only_segment_at_leaf.snap @@ -4,6 +4,4 @@ expression: result --- orders_ms_view__category | orders_ms_view__total_amount | orders_ms_view__amount_keep_only_segment -------------------------+------------------------------+----------------------------------------- -books | 600.00 | 600.00 -clothing | NULL | 300.00 -electronics | NULL | 500.00 +books | 600.00 | 600.00 From 259188d9cd511c9ccf5f4cf18c92719f5d9e5e07 Mon Sep 17 00:00:00 2001 From: waralexrom <108349432+waralexrom@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:29:57 +0200 Subject: [PATCH 3/3] fix(tesseract): don't serve ungrouped queries from collapsing rollups (#11485) --- ...-aggregations-ungrouped-cross-cube.test.ts | 233 +++++++ .../cubesqlplanner/src/logical_plan/join.rs | 11 + .../optimizers/pre_aggregation/optimizer.rs | 202 ++++++- .../cubesqlplanner/src/planner/join_tree.rs | 19 +- .../src/planner/planners/join_planner.rs | 1 + .../src/planner/planners/join_tree_builder.rs | 47 ++ .../src/planner/symbols/measure_kinds/mod.rs | 8 + .../common/pre_aggregation_matching_test.yaml | 11 + .../common/pre_aggregations_test.yaml | 29 + .../common/ungrouped_multiplied_dup.yaml | 38 ++ .../common/ungrouped_pre_agg_gate.yaml | 493 +++++++++++++++ .../seeds/ungrouped_multiplied_dup_tables.sql | 23 + .../src/tests/integration/modifiers.rs | 106 ++++ .../tests/integration/pre_aggregations/mod.rs | 1 + ...w_joined_keys_rollup_cubestore_result.snap | 19 + ..._reads_rollup_column_cubestore_result.snap | 21 +- ..._view_query_for_join_cubestore_result.snap | 16 + .../pre_aggregations/sql_generation.rs | 180 +++++- .../pre_aggregations/ungrouped_gate.rs | 567 ++++++++++++++++++ ...ps_group_by_for_aggregate_sql_measure.snap | 7 + 20 files changed, 2009 insertions(+), 23 deletions(-) create mode 100644 packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-ungrouped-cross-cube.test.ts create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/ungrouped_multiplied_dup.yaml create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/ungrouped_pre_agg_gate.yaml create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/seeds/ungrouped_multiplied_dup_tables.sql create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_cross_cube_view_joined_keys_rollup_cubestore_result.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_view_query_for_join_cubestore_result.snap create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/ungrouped_gate.rs create mode 100644 rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__modifiers__ungrouped_multiplied_keeps_group_by_for_aggregate_sql_measure.snap diff --git a/packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-ungrouped-cross-cube.test.ts b/packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-ungrouped-cross-cube.test.ts new file mode 100644 index 0000000000000..fa2de31cfe3a2 --- /dev/null +++ b/packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-ungrouped-cross-cube.test.ts @@ -0,0 +1,233 @@ +import { PostgresQuery } from '../../../src/adapter/PostgresQuery'; +import { prepareYamlCompiler } from '../../unit/PrepareCompiler'; + +const CUBES = ` +cubes: + - name: visitors + sql: "SELECT * FROM visitors" + joins: + - name: visitor_checkins + relationship: one_to_many + sql: "{CUBE.id} = {visitor_checkins.visitor_id}" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: source + type: string + sql: source + + - name: visitor_checkins + sql: "SELECT * FROM visitor_checkins" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: visitor_id + type: number + sql: visitor_id + - name: created_at + type: time + sql: created_at + measures: + - name: count + type: count + pre_aggregations: +`; + +// Renames every member, so the query spells members no pre-aggregation mentions. +const VIEW = ` +views: + - name: visitors_view + cubes: + - join_path: visitors + includes: + - id + - source + prefix: true + - join_path: visitors.visitor_checkins + includes: + - visitor_id + - count + prefix: true +`; + +// Grouped by a non-key dimension plus a cross-cube one, so its rows are already +// collapsed and reading them flat would drop the join. +const COLLAPSED_ROLLUP = ` + - name: joined_rollup + type: rollup + measures: + - count + dimensions: + - visitor_id + - visitors.source + time_dimension: created_at + granularity: day +`; + +// The same collapsed rollup without a time dimension, so the query's member set +// matches it exactly. Nothing but the primary-key rule stands between this query +// and a join-less read of collapsed rows. +const COLLAPSED_ROLLUP_NO_TIME_DIMENSION = ` + - name: no_keys_rollup + type: rollup + measures: + - count + dimensions: + - visitor_id + - visitors.source +`; + +// Same cross-cube shape, but grouped by the primary keys of both cubes, so each +// stored row is one raw joined row. +const KEYED_ROLLUP = ` + - name: joined_keys_rollup + type: rollup + measures: + - count + dimensions: + - id + - visitor_id + - visitors.id + - visitors.source +`; + +const BASE_QUERY = { + ungrouped: true, + // Without this a joined ungrouped query is rejected by `initUngrouped` unless + // it selects the primary keys of every joined cube. It is a server-level + // option (`CUBEJS_ALLOW_UNGROUPED_WITHOUT_PRIMARY_KEY`) that inherits + // `CUBESQL_SQL_PUSH_DOWN`'s default, so it is normally on. + allowUngroupedWithoutPrimaryKey: true, + timezone: 'America/Los_Angeles', + preAggregationsSchema: '', +}; + +const PLANNERS: [string, boolean][] = [ + ['legacy planner', false], + ['native planner', true], +]; + +describe('PreAggregations ungrouped cross-cube query', () => { + jest.setTimeout(200000); + + describe('rollup whose grouping collapses raw rows', () => { + const { compiler, joinGraph, cubeEvaluator } = prepareYamlCompiler( + CUBES + COLLAPSED_ROLLUP + VIEW + ); + + it.each(PLANNERS)('is not matched, keeping the join (%s)', async (_label, useNativeSqlPlanner) => { + await compiler.compile(); + + const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, { + ...BASE_QUERY, + dimensions: ['visitor_checkins.visitor_id', 'visitors.source'], + filters: [{ member: 'visitors.source', operator: 'equals', values: ['google'] }], + useNativeSqlPlanner, + } as any); + + const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription(); + const [sql] = query.buildSqlAndParams(); + + expect(preAggregationsDescription).toEqual([]); + expect(sql).not.toContain('joined_rollup'); + expect(sql.toLowerCase()).toContain('join'); + }); + + it.each(PLANNERS)('is not matched through a view either (%s)', async (_label, useNativeSqlPlanner) => { + await compiler.compile(); + + const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, { + ...BASE_QUERY, + dimensions: ['visitors_view.visitor_checkins_visitor_id', 'visitors_view.visitors_source'], + filters: [{ member: 'visitors_view.visitors_source', operator: 'equals', values: ['google'] }], + useNativeSqlPlanner, + } as any); + + const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription(); + const [sql] = query.buildSqlAndParams(); + + expect(preAggregationsDescription).toEqual([]); + expect(sql).not.toContain('joined_rollup'); + expect(sql.toLowerCase()).toContain('join'); + }); + }); + + // Native planner only: legacy matches this rollup and reads it without the + // join, so asserting legacy here would pin that bug. + describe('collapsed rollup whose member set matches the query exactly', () => { + const { compiler, joinGraph, cubeEvaluator } = prepareYamlCompiler( + CUBES + COLLAPSED_ROLLUP_NO_TIME_DIMENSION + VIEW + ); + + it('is not matched, keeping the join (native planner)', async () => { + await compiler.compile(); + + const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, { + ...BASE_QUERY, + dimensions: ['visitor_checkins.visitor_id', 'visitors.source'], + useNativeSqlPlanner: true, + } as any); + + const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription(); + const [sql] = query.buildSqlAndParams(); + + expect(preAggregationsDescription).toEqual([]); + expect(sql).not.toContain('no_keys_rollup'); + expect(sql.toLowerCase()).toContain('join'); + }); + }); + + describe('rollup grouped by the primary keys of both cubes', () => { + const { compiler, joinGraph, cubeEvaluator } = prepareYamlCompiler( + CUBES + KEYED_ROLLUP + VIEW + ); + + it.each(PLANNERS)('is matched when the query carries the primary keys (%s)', async (_label, useNativeSqlPlanner) => { + await compiler.compile(); + + const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, { + ...BASE_QUERY, + dimensions: [ + 'visitors.id', + 'visitor_checkins.id', + 'visitors.source', + 'visitor_checkins.visitor_id', + ], + useNativeSqlPlanner, + } as any); + + const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription(); + const [sql] = query.buildSqlAndParams(); + + expect(preAggregationsDescription.map((d: any) => d.tableName)).toEqual([ + 'visitor_checkins_joined_keys_rollup', + ]); + expect(sql).toContain('visitor_checkins_joined_keys_rollup'); + }); + + // Native planner only: legacy rejects this rollup even though one stored row + // is one raw joined row. Going through the view also pins that the view is + // not mistaken for a third cube. + it('is matched through a view (native planner)', async () => { + await compiler.compile(); + + const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, { + ...BASE_QUERY, + dimensions: ['visitors_view.visitors_source', 'visitors_view.visitor_checkins_visitor_id'], + useNativeSqlPlanner: true, + } as any); + + const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription(); + const [sql] = query.buildSqlAndParams(); + + expect(preAggregationsDescription.map((d: any) => d.tableName)).toEqual([ + 'visitor_checkins_joined_keys_rollup', + ]); + expect(sql).toContain('visitor_checkins_joined_keys_rollup'); + }); + }); +}); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/join.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/join.rs index 884eac91c6507..c1ffb306b2c72 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/join.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/join.rs @@ -11,6 +11,10 @@ use typed_builder::TypedBuilder; pub struct LogicalJoinItem { cube: Rc, on_sql: Rc, + /// Required, with no default: `false` is the answer that lets a rollup + /// collapsing this edge's rows serve a raw-row query, so a construction site + /// must not be able to omit it. + splits_rows: bool, } impl LogicalJoinItem { @@ -21,6 +25,12 @@ impl LogicalJoinItem { pub fn on_sql(&self) -> &Rc { &self.on_sql } + + /// Whether joining this cube in splits a row of its parent into + /// several, i.e. it sits on the `one_to_many` side of its edge. + pub fn splits_rows(&self) -> bool { + self.splits_rows + } } impl PrettyPrint for LogicalJoinItem { @@ -129,6 +139,7 @@ impl LogicalNode for LogicalJoin { Ok(LogicalJoinItem::builder() .cube(item.clone().into_logical_node()?) .on_sql(self_item.on_sql().clone()) + .splits_rows(self_item.splits_rows()) .build()) }) .collect::, _>>()?; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/optimizer.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/optimizer.rs index 0d6ee57f17ef6..25754b62f082a 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/optimizer.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/optimizer.rs @@ -2,17 +2,19 @@ use super::PreAggregationsCompiler; use super::*; use crate::logical_plan::visitor::{LogicalPlanRewriter, NodeRewriteResult}; use crate::logical_plan::*; -use crate::planner::collectors::has_multi_stage_members; +use crate::planner::collectors::{collect_cube_names_from_symbols, has_multi_stage_members}; use crate::planner::filter::FilterItem; use crate::planner::filter::FilterOp; use crate::planner::join_hints::JoinHints; use crate::planner::multi_fact_join_groups::{MeasuresJoinHints, MultiFactJoinGroups}; use crate::planner::planners::multi_stage::TimeShiftState; +use crate::planner::planners::CommonUtils; use crate::planner::state::State; use crate::planner::time_dimension::QueryDateTime; use crate::planner::MemberSymbol; use cubenativeutils::CubeError; -use std::collections::HashSet; +use std::cell::RefCell; +use std::collections::{HashMap, HashSet}; use std::rc::Rc; pub struct PreAggregationUsage { @@ -35,11 +37,24 @@ impl PreAggregationUsage { } } +/// What a query does with the rows underneath it, which decides whether a +/// pre-aggregation's grain has to match those rows one for one. +enum RowGrain { + /// The query aggregates, so a coarser stored grain is still usable. + Aggregated, + /// The query returns raw rows over the given join. `None` when the node is + /// not a plain cube join, leaving nothing to establish row identity against. + RawRows(Option>), +} + pub struct PreAggregationOptimizer { query_tools: Rc, allow_multi_stage: bool, usages: Vec, usage_counter: usize, + /// Resolved primary-key names per cube. Every candidate pre-aggregation asks + /// for the same cubes, and resolving them crosses the JS bridge. + primary_keys_cache: RefCell>>, } impl PreAggregationOptimizer { @@ -49,6 +64,7 @@ impl PreAggregationOptimizer { allow_multi_stage, usages: Vec::new(), usage_counter: 0, + primary_keys_cache: RefCell::new(HashMap::new()), } } @@ -88,6 +104,7 @@ impl PreAggregationOptimizer { root.query(), compiled_pre_aggregations, &TimeShiftState::default(), + true, )? { return Ok(Some(Rc::new( RootQuery::builder().ctes(vec![]).query(rewritten).build(), @@ -109,18 +126,22 @@ impl PreAggregationOptimizer { std::mem::take(&mut self.usages) } + // `is_user_query` marks the query the user actually asked for, as opposed to + // an internal multi-stage leaf. Only the former's `ungrouped` flag means + // "return raw rows"; a leaf may carry it purely for how its stage renders. fn try_rewrite_query( &mut self, query: &Rc, compiled_pre_aggregations: &[Rc], time_shifts: &TimeShiftState, + is_user_query: bool, ) -> Result>, CubeError> { for pre_aggregation in compiled_pre_aggregations.iter() { let external = pre_aggregation.external.unwrap_or(false); let date_range = Self::extract_date_range(&query.filter(), &self.query_tools, time_shifts, external); if let Some(rewritten) = - self.try_rewrite_simple_query(query, pre_aggregation, date_range)? + self.try_rewrite_simple_query(query, pre_aggregation, date_range, is_user_query)? { return Ok(Some(rewritten)); } @@ -134,10 +155,25 @@ impl PreAggregationOptimizer { query: &Rc, pre_aggregation: &Rc, date_range: Option<(String, String)>, + is_user_query: bool, ) -> Result>, CubeError> { - if let Some(matched_measures) = - self.is_schema_and_filters_match(&query.schema(), &query.filter(), pre_aggregation)? - { + // Row identity for an ungrouped read is judged against the join this + // very node will render, taken from the node itself rather than + // re-resolved, so the two can never disagree. + let row_grain = if is_user_query && query.modifers().ungrouped { + RowGrain::RawRows(match query.source() { + QuerySource::LogicalJoin(join) => Some(join.clone()), + _ => None, + }) + } else { + RowGrain::Aggregated + }; + if let Some(matched_measures) = self.is_schema_and_filters_match( + &query.schema(), + &query.filter(), + pre_aggregation, + row_grain, + )? { let source = self.make_pre_aggregation_source(pre_aggregation, &matched_measures, date_range)?; let new_query = Query::builder() @@ -169,9 +205,16 @@ impl PreAggregationOptimizer { &TimeShiftState::default(), external, ); - if let Some(matched_measures) = - self.is_schema_and_filters_match(schema, filter, pre_aggregation)? - { + // This node holds no `Query` of its own, so its `ungrouped` flag is + // not reachable here and no join is available to judge row identity + // against. An ungrouped request routed through here can still be + // served by a pre-aggregation that collapses its rows. + if let Some(matched_measures) = self.is_schema_and_filters_match( + schema, + filter, + pre_aggregation, + RowGrain::Aggregated, + )? { let source = self.make_pre_aggregation_source( pre_aggregation, &matched_measures, @@ -238,6 +281,7 @@ impl PreAggregationOptimizer { &multi_stage_leaf_measure.query, compiled_pre_aggregations, &multi_stage_leaf_measure.evaluation_context.time_shifts, + false, )? { let new_leaf = Rc::new(MultiStageLeafMeasure { measures: multi_stage_leaf_measure.measures.clone(), @@ -481,6 +525,7 @@ impl PreAggregationOptimizer { schema: &Rc, filters: &Rc, pre_aggregation: &CompiledPreAggregation, + row_grain: RowGrain, ) -> Result>, CubeError> { let helper = OptimizerHelper::new(); @@ -498,6 +543,12 @@ impl PreAggregationOptimizer { return Ok(None); } + if let RowGrain::RawRows(node_join) = &row_grain { + if !self.is_raw_rows_match(node_join.as_ref(), pre_aggregation)? { + return Ok(None); + } + } + // The query's join groups answer both the multiplicativity gate // and the join-path comparison below, so build them once. let query_groups = self.query_join_groups(schema, &all_measures)?; @@ -517,6 +568,20 @@ impl PreAggregationOptimizer { return Ok(None); }; + // An ungrouped read projects stored columns as they are, with no + // aggregate around them, so a measure kept as a mergeable sketch would + // reach the client as the sketch instead of a number. + if matches!(row_grain, RowGrain::RawRows(_)) { + for symbol in pre_aggregation.measures.iter() { + if !matched_measures.contains(symbol.full_name().as_str()) { + continue; + } + if symbol.as_measure()?.kind().is_stored_as_state() { + return Ok(None); + } + } + } + // Even when the query itself has no multiplied measures, a measure that // is multiplied in the pre-aggregation (because the pre-agg groups by a // multiplier dimension) stores a different value than the query expects, @@ -566,6 +631,125 @@ impl PreAggregationOptimizer { Ok(Some(matched_measures)) } + // An ungrouped query returns raw rows, so a pre-aggregation may serve it + // only when each of its stored rows is exactly one row of the raw join. + // + // What identifies such a row is the key of the join root plus the key of + // every cube joined in on an edge that splits a row of its parent into + // several. A cube reached only over non-splitting edges cannot make the + // output finer, so its key is not needed, while a cube that merely transits + // the tree still splits rows and counts even when the query names no member + // of it. The root is always needed: it is what tells apart the rows an outer + // join leaves unmatched. A cube without a primary key has no row identity at + // all, so nothing can be read raw from it. + // + // The join comes from the node being rewritten, so it is by construction the + // one that node renders — filters, join hints and order-by members included. + fn is_raw_rows_match( + &self, + node_join: Option<&Rc>, + pre_aggregation: &CompiledPreAggregation, + ) -> Result { + // Only a plain rollup describes the grain its rows were stored at. A + // join or union source is described by declared members that need not + // reflect what the underlying rollups actually store, so its grain + // cannot be established here. + if !matches!( + pre_aggregation.source.as_ref(), + PreAggregationSource::Single(_) + ) { + return Ok(false); + } + + // Anything but a plain cube join — a full-key aggregate, or a source + // already rewritten to a pre-aggregation — has no single join to judge + // row identity against. + let Some(node_join) = node_join else { + return Ok(false); + }; + let Some(root) = node_join.root() else { + return Ok(false); + }; + + let stored_dimensions: HashSet = pre_aggregation + .dimensions + .iter() + .map(|d| d.clone().resolve_reference_chain().full_name()) + .collect(); + + let joined_cubes: HashSet = std::iter::once(root.name().clone()) + .chain( + node_join + .joins() + .iter() + .map(|item| item.cube().name().clone()), + ) + .collect(); + + // The pre-aggregation must not be stored at a finer grain than the node + // reads either: a cube it groups by but the node never joins splits its + // rows further, so the same row would come back more than once. + for cube_name in Self::pre_aggregation_grain_cubes(pre_aggregation)? { + if !joined_cubes.contains(&cube_name) { + return Ok(false); + } + } + + let identifying_cubes = std::iter::once(root.name().clone()).chain( + node_join + .joins() + .iter() + .filter(|item| item.splits_rows()) + .map(|item| item.cube().name().clone()), + ); + + for cube_name in identifying_cubes { + let keys = self.resolved_primary_keys(&cube_name)?; + if keys.is_empty() { + return Ok(false); + } + if !keys.iter().all(|key| stored_dimensions.contains(key)) { + return Ok(false); + } + } + + Ok(true) + } + + /// Cubes that set the grain the pre-aggregation stores its rows at, which is + /// what it groups by: its dimensions and time dimensions, plus its segments, + /// which are appended to the dimension list when the table is materialized + /// and so group the stored rows too. Measures are excluded — a measure joins + /// whatever its own SQL references, but that join is aggregated away inside + /// the rollup and leaves its row count untouched. + fn pre_aggregation_grain_cubes( + pre_aggregation: &CompiledPreAggregation, + ) -> Result, CubeError> { + let members = pre_aggregation + .dimensions + .iter() + .chain(pre_aggregation.time_dimensions.iter()) + .chain(pre_aggregation.segments.iter()) + .cloned() + .collect::>(); + collect_cube_names_from_symbols(&members) + } + + fn resolved_primary_keys(&self, cube_name: &String) -> Result, CubeError> { + if let Some(cached) = self.primary_keys_cache.borrow().get(cube_name) { + return Ok(cached.clone()); + } + let keys = CommonUtils::new(self.query_tools.clone()) + .primary_keys_dimensions(cube_name)? + .into_iter() + .map(|key| key.resolve_reference_chain().full_name()) + .collect::>(); + self.primary_keys_cache + .borrow_mut() + .insert(cube_name.clone(), keys.clone()); + Ok(keys) + } + fn query_join_groups( &self, schema: &Rc, diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/join_tree.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/join_tree.rs index ebf0fb78edc72..0903d9984a732 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/join_tree.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/join_tree.rs @@ -10,14 +10,21 @@ pub struct JoinTreeItem { cube: Rc, original_from: String, on_sql: Rc, + splits_rows: bool, } impl JoinTreeItem { - pub fn new(cube: Rc, original_from: String, on_sql: Rc) -> Self { + pub fn new( + cube: Rc, + original_from: String, + on_sql: Rc, + splits_rows: bool, + ) -> Self { Self { cube, original_from, on_sql, + splits_rows, } } @@ -32,6 +39,16 @@ impl JoinTreeItem { pub fn on_sql(&self) -> &Rc { &self.on_sql } + + /// Whether joining this cube in splits a row of its parent into + /// several — true exactly for the `one_to_many` side of an edge. + /// Unlike `JoinTree::is_multiplied`, which answers whether a cube's + /// own rows repeat and is only populated for the cubes the query + /// asked for, this is a property of the edge itself and so also + /// holds for cubes that merely transit the tree. + pub fn splits_rows(&self) -> bool { + self.splits_rows + } } /// A resolved join tree: the root cube plus its joined cubes with the diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/join_planner.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/join_planner.rs index 6b70c32812a6c..99edf84fbdaa8 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/join_planner.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/join_planner.rs @@ -87,6 +87,7 @@ impl JoinPlanner { LogicalJoinItem::builder() .cube(Cube::new(item.cube().clone())) .on_sql(item.on_sql().clone()) + .splits_rows(item.splits_rows()) .build() }) .collect(); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/join_tree_builder.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/join_tree_builder.rs index c5a360ce78a3a..343face2164b1 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/join_tree_builder.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/join_tree_builder.rs @@ -26,10 +26,12 @@ impl JoinTreeBuilder { let static_data = join_definition.static_data(); let cube = self.utils.cube_from_path(static_data.original_to.clone())?; let on_sql = self.utils.compile_join_condition(join_definition.clone())?; + let relationship = join_definition.join()?.static_data().relationship.clone(); joins.push(JoinTreeItem::new( cube, static_data.original_from.clone(), on_sql, + relationship_splits_rows(&relationship), )); } Ok(JoinTree::new( @@ -39,3 +41,48 @@ impl JoinTreeBuilder { )) } } + +/// Whether joining the `to` side of an edge with this relationship splits one row +/// of the `from` side into several. Only many-to-one and one-to-one keep the row +/// count, and a relationship arrives either normalized or in one of its model +/// spellings, so every spelling of those two is listed. Anything unrecognized +/// counts as splitting: requiring a primary key that is not needed only refuses a +/// usable pre-aggregation, while omitting a needed one serves collapsed rows. +fn relationship_splits_rows(relationship: &str) -> bool { + !matches!( + relationship, + "belongsTo" + | "belongs_to" + | "many_to_one" + | "manyToOne" + | "hasOne" + | "has_one" + | "one_to_one" + | "oneToOne" + ) +} + +#[cfg(test)] +mod tests { + use super::relationship_splits_rows; + + #[test] + fn splits_rows_covers_every_relationship_spelling() { + for keeps_row_count in ["belongsTo", "many_to_one", "hasOne", "one_to_one"] { + assert!( + !relationship_splits_rows(keeps_row_count), + "`{keeps_row_count}` joins at most one row" + ); + } + for splits in ["hasMany", "one_to_many"] { + assert!( + relationship_splits_rows(splits), + "`{splits}` joins many rows" + ); + } + assert!( + relationship_splits_rows("something_else"), + "an unrecognized relationship has to be treated as splitting" + ); + } +} diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/measure_kinds/mod.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/measure_kinds/mod.rs index 8781d1f336db5..0f4278e1fea0a 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/measure_kinds/mod.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/measure_kinds/mod.rs @@ -120,6 +120,14 @@ impl MeasureKind { } } + /// True if a rollup stores this kind as a mergeable sketch rather than the + /// final value — either it already is that state form, or it is the kind + /// that has one. Derived from [`Self::as_state`] so both answers cannot + /// drift apart. + pub fn is_stored_as_state(&self) -> bool { + matches!(self, Self::AggregatedState(_)) || self.as_state().is_some() + } + pub fn measure_type_str(&self) -> &str { match self { Self::Count(_) | Self::MultipliedCount(_) => "count", diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/pre_aggregation_matching_test.yaml b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/pre_aggregation_matching_test.yaml index 6d6ecdf05fd5f..a5f9ea15659f9 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/pre_aggregation_matching_test.yaml +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/pre_aggregation_matching_test.yaml @@ -170,3 +170,14 @@ cubes: - high_priority time_dimension: created_at granularity: day + + # Groups by the primary key, so every stored row is a single order — + # the only shape a rollup may serve an ungrouped query from. + - name: primary_key_rollup + type: rollup + measures: + - total_amount + dimensions: + - id + - status + - city diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/pre_aggregations_test.yaml b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/pre_aggregations_test.yaml index 9834011ad24c9..578e2299cc9a9 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/pre_aggregations_test.yaml +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/pre_aggregations_test.yaml @@ -126,6 +126,18 @@ cubes: - visitor_checkins.for_lambda - visitor_checkins2.for_lambda + # Same cross-cube shape as joined_rollup, but grouped by the primary keys + # of both cubes, so each stored row is one raw joined row + - name: joined_keys_rollup + type: rollup + measures: + - count + dimensions: + - id + - visitor_id + - visitors.id + - visitors.source + # Second cube for rollupLambda example - name: visitor_checkins2 sql: "SELECT * FROM visitor_checkins" @@ -154,3 +166,20 @@ cubes: time_dimension: created_at granularity: day partition_granularity: day + +views: + # Renames every member (prefix), so view members are references whose names + # differ from the cube members the pre-aggregations are declared over. + - name: visitors_view + cubes: + - join_path: visitors + includes: + - id + - source + prefix: true + - join_path: visitors.visitor_checkins + includes: + - id + - visitor_id + - count + prefix: true diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/ungrouped_multiplied_dup.yaml b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/ungrouped_multiplied_dup.yaml new file mode 100644 index 0000000000000..771cdb7d2647d --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/ungrouped_multiplied_dup.yaml @@ -0,0 +1,38 @@ +# Two customers share the same name, each with one order of the same status, so +# grouping by the requested dimensions would collapse two distinct primary keys +# into one row. +cubes: + - name: dup_customers + sql: "SELECT * FROM dup_customers" + joins: + - name: dup_orders + relationship: one_to_many + sql: "{CUBE.id} = {dup_orders.customer_id}" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: name + type: string + sql: name + measures: + # The aggregate lives inside the measure's own sql, so nothing may wrap it + # and the select it lands in has to group. + - name: ltv_agg_number + type: number + sql: "sum(ltv)" + + - name: dup_orders + sql: "SELECT * FROM dup_orders" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: customer_id + type: number + sql: customer_id + - name: status + type: string + sql: status diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/ungrouped_pre_agg_gate.yaml b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/ungrouped_pre_agg_gate.yaml new file mode 100644 index 0000000000000..1051003c62a71 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/ungrouped_pre_agg_gate.yaml @@ -0,0 +1,493 @@ +# Models for the ungrouped pre-aggregation eligibility rule. Each cube group is +# self-contained so a query only ever pulls in the cubes it names. +cubes: + # No primary key at all. `collapsing` groups by non-key columns only. + - name: no_pk_orders + sql: "SELECT * FROM orders" + dimensions: + - name: status + type: string + sql: status + - name: city + type: string + sql: city + measures: + - name: total + type: sum + sql: amount + pre_aggregations: + - name: collapsing + type: rollup + measures: + - total + dimensions: + - status + - city + + # Single cube that does declare a key, with a rollup that omits it. + - name: sc_orders + sql: "SELECT * FROM orders" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: status + type: string + sql: status + - name: city + type: string + sql: city + measures: + - name: total + type: sum + sql: amount + pre_aggregations: + - name: sc_coarse + type: rollup + measures: + - total + dimensions: + - status + - city + + # visitors -> checkins -> cities. `checkins` fans out but is only a transit + # hop, so a query naming just visitors and cities members still multiplies. + - name: chain_visitors + sql: "SELECT * FROM visitors" + joins: + - name: chain_checkins + relationship: one_to_many + sql: "{CUBE.id} = {chain_checkins.visitor_id}" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: source + type: string + sql: source + pre_aggregations: + - name: chain_rollup + type: rollup + dimensions: + - id + - source + - chain_cities.id + - chain_cities.name + + # Keyed on the root only, with a segment reaching across the fan-out. + - name: root_key_seg_rollup + type: rollup + dimensions: + - id + - source + segments: + - chain_cities.named_x + + - name: chain_checkins + sql: "SELECT * FROM visitor_checkins" + joins: + - name: chain_cities + relationship: many_to_one + sql: "{CUBE.city_id} = {chain_cities.id}" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: visitor_id + type: number + sql: visitor_id + - name: city_id + type: number + sql: city_id + + - name: chain_cities + sql: "SELECT * FROM cities" + segments: + - name: named_x + sql: "{CUBE}.name = 'X'" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: name + type: string + sql: name + + # Canonical star: fact many_to_one dimension cube, so the join cannot + # multiply rows and the fact key alone identifies every stored row. + - name: star_checkins + sql: "SELECT * FROM visitor_checkins" + joins: + - name: star_visitors + relationship: many_to_one + sql: "{CUBE.visitor_id} = {star_visitors.id}" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: visitor_id + type: number + sql: visitor_id + # Proxy for this cube's own primary key, which row identity does require. + - name: own_ref + type: number + sql: "{id}" + - name: created_at + type: time + sql: created_at + measures: + - name: cnt + type: count + pre_aggregations: + # Keyed on the primary key, so 1:1 with raw rows, but it stores the time + # dimension truncated to the day. + - name: star_day_rollup + type: rollup + measures: + - cnt + dimensions: + - id + time_dimension: created_at + granularity: day + + - name: star_rollup + type: rollup + measures: + - cnt + dimensions: + - id + - star_visitors.source + + # Stores the fact key only through a proxy dimension. + - name: proxy_rollup + type: rollup + measures: + - cnt + dimensions: + - own_ref + - star_visitors.source + + # 1:1 with raw rows: the segment groups the stored rows, but it reaches + # star_visitors over a many_to_one edge that cannot split a fact row. + - name: seg_rollup + type: rollup + measures: + - cnt + dimensions: + - id + - visitor_id + segments: + - star_visitors.source_google + + - name: star_visitors + sql: "SELECT * FROM visitors" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: source + type: string + sql: source + segments: + - name: source_google + sql: "{CUBE}.source = 'google'" + + # Pure many_to_one chain: the middle cube only bridges, so it cannot split a + # row and its key is not part of row identity. + - name: bridge_orders + sql: "SELECT * FROM orders" + joins: + - name: bridge_stores + relationship: many_to_one + sql: "{CUBE.store_id} = {bridge_stores.id}" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: store_id + type: number + sql: store_id + measures: + - name: cnt + type: count + pre_aggregations: + - name: bridge_rollup + type: rollup + measures: + - cnt + dimensions: + - id + - bridge_regions.name + + - name: bridge_stores + sql: "SELECT * FROM stores" + joins: + - name: bridge_regions + relationship: many_to_one + sql: "{CUBE.region_id} = {bridge_regions.id}" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: region_id + type: number + sql: region_id + + - name: bridge_regions + sql: "SELECT * FROM regions" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: name + type: string + sql: name + + # Non-additive stored state: a rollup row holds an HLL sketch, not a number. + - name: hll_orders + sql: "SELECT * FROM orders" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: status + type: string + sql: status + - name: user_id + type: number + sql: user_id + measures: + - name: uniq + type: count_distinct_approx + sql: user_id + pre_aggregations: + - name: hll_pk_rollup + type: rollup + measures: + - uniq + dimensions: + - id + - status + + # Fan-out measure: forces the multiplied-subquery planning path. + - name: mult_customers + sql: "SELECT * FROM customers" + joins: + - name: mult_orders + relationship: one_to_many + sql: "{CUBE.id} = {mult_orders.customer_id}" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: name + type: string + sql: name + measures: + - name: count + type: count + # Joins mult_orders inside its own sql; the rollup aggregates that away. + - name: orders_total + type: sum + sql: "{mult_orders.cnt}" + pre_aggregations: + # Grouped by the primary key, and carrying a measure whose sql reaches + # another cube without that cube touching the stored grain. + - name: mult_keyed + type: rollup + measures: + - orders_total + dimensions: + - id + - name + + - name: mult_collapsing + type: rollup + measures: + - count + dimensions: + - name + - mult_orders.status + + - name: mult_orders + sql: "SELECT * FROM orders" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: customer_id + type: number + sql: customer_id + - name: status + type: string + sql: status + measures: + - name: cnt + type: count + + # Non-additive rolling measure: its leaf stage is planned ungrouped for + # rendering reasons even though the user query is grouped. + - name: roll_orders + sql: "SELECT * FROM orders" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: category + type: string + sql: category + - name: created_at + type: time + sql: created_at + measures: + - name: rolling_avg_7d + type: avg + sql: amount + rolling_window: + trailing: 7 day + pre_aggregations: + - name: rolling_avg_rollup + type: rollup + measures: + - rolling_avg_7d + dimensions: + - category + time_dimension: created_at + granularity: day + + # rollupLambda union where the second branch groups by a non-unique column + # that merely shares the short name `id` with the first branch's key. + - name: lam_a + sql: "SELECT * FROM visitor_checkins" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: visitor_id + type: number + sql: visitor_id + - name: created_at + type: time + sql: created_at + measures: + - name: count + type: count + pre_aggregations: + - name: lam_base + type: rollup + measures: + - count + dimensions: + - id + - visitor_id + time_dimension: created_at + granularity: day + partition_granularity: day + + - name: lam_union + type: rollupLambda + rollups: + - lam_a.lam_base + - lam_b.lam_base + + - name: lam_b + sql: "SELECT * FROM visitor_checkins" + dimensions: + - name: row_id + type: number + sql: row_id + primary_key: true + # Shares the short name with lam_a's key but is not unique. + - name: id + type: number + sql: visitor_id + - name: visitor_id + type: number + sql: visitor_id + - name: created_at + type: time + sql: created_at + measures: + - name: count + type: count + pre_aggregations: + - name: lam_base + type: rollup + measures: + - count + dimensions: + - id + - visitor_id + time_dimension: created_at + granularity: day + partition_granularity: day + + # rollupJoin advertising a primary key that neither member rollup stores. + - name: rj_checkins + sql: "SELECT * FROM visitor_checkins" + joins: + - name: rj_visitors + relationship: many_to_one + sql: "{CUBE.visitor_id} = {rj_visitors.id}" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: visitor_id + type: number + sql: visitor_id + measures: + - name: count + type: count + pre_aggregations: + - name: rj_base + type: rollup + measures: + - count + dimensions: + - visitor_id + + - name: rj_keys_join + type: rollupJoin + measures: + - count + dimensions: + - id + - visitor_id + - rj_visitors.id + - rj_visitors.source + rollups: + - rj_checkins.rj_base + - rj_visitors.rj_base + + - name: rj_visitors + sql: "SELECT * FROM visitors" + dimensions: + - name: id + type: number + sql: id + primary_key: true + - name: source + type: string + sql: source + pre_aggregations: + - name: rj_base + type: rollup + dimensions: + - id + - source diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/seeds/ungrouped_multiplied_dup_tables.sql b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/seeds/ungrouped_multiplied_dup_tables.sql new file mode 100644 index 0000000000000..14761d0e97ac0 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/seeds/ungrouped_multiplied_dup_tables.sql @@ -0,0 +1,23 @@ +DROP TABLE IF EXISTS dup_orders CASCADE; +DROP TABLE IF EXISTS dup_customers CASCADE; + +CREATE TABLE dup_customers ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + ltv NUMERIC(10, 2) NOT NULL +); + +-- Same name, different keys and different ltv. +INSERT INTO dup_customers (id, name, ltv) VALUES + (1, 'dup', 100), + (2, 'dup', 50); + +CREATE TABLE dup_orders ( + id INTEGER PRIMARY KEY, + customer_id INTEGER NOT NULL, + status TEXT NOT NULL +); + +INSERT INTO dup_orders (id, customer_id, status) VALUES + (10, 1, 'new'), + (11, 2, 'new'); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/modifiers.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/modifiers.rs index a790679d4c1f1..78c582e486a90 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/modifiers.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/modifiers.rs @@ -15,6 +15,39 @@ fn create_multi_fact_context() -> TestContext { const BASIC_SEED: &str = "integration_basic_tables.sql"; const MULTI_FACT_SEED: &str = "integration_multi_fact_tables.sql"; +/// Body of the first ` AS ( ... )` block, in text order, whose text +/// contains `marker`, so an assertion can be aimed at one CTE instead of the +/// whole statement. `marker` must be lower-case. +fn cte_body_containing(sql: &str, marker: &str) -> Option { + let lower = sql.to_lowercase(); + let mut from = 0; + while let Some(rel) = lower[from..].find(" as (") { + let open = from + rel + " as (".len() - 1; + let mut depth = 0; + let mut close = None; + for (offset, ch) in sql[open..].char_indices() { + match ch { + '(' => depth += 1, + ')' => { + depth -= 1; + if depth == 0 { + close = Some(open + offset + 1); + break; + } + } + _ => {} + } + } + let close = close?; + let body = &sql[open..close]; + if body.to_lowercase().contains(marker) { + return Some(body.to_string()); + } + from = open + 1; + } + None +} + // 9.1: ORDER BY dimension ASC, measure DESC #[tokio::test(flavor = "multi_thread")] async fn test_order_by_dimension_and_measure() { @@ -279,3 +312,76 @@ async fn test_ungrouped_with_time_dimension() { insta::assert_snapshot!(result); } } + +const UNGROUPED_DUP_SEED: &str = "ungrouped_multiplied_dup_tables.sql"; + +// A measure whose own sql already aggregates cannot be projected row-level: the +// select it lands in must group, or the SQL is not valid at all. This holds for +// an ungrouped request too, so the multiplied CTE keeps its GROUP BY here. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_multiplied_keeps_group_by_for_aggregate_sql_measure() { + let schema = MockSchema::from_yaml_file("common/ungrouped_multiplied_dup.yaml"); + let ctx = TestContext::new(schema).unwrap(); + + let query = indoc! {" + measures: + - dup_customers.ltv_agg_number + dimensions: + - dup_customers.name + - dup_orders.status + ungrouped: true + "}; + + let sql = ctx.build_sql(query).unwrap(); + let lower = sql.to_lowercase(); + + assert!( + lower.contains("select distinct"), + "expected the keys subquery of a multiplied measure, got:\n{sql}" + ); + assert!( + lower.contains("group by"), + "a measure that aggregates in its own sql needs the enclosing select to \ + group, got:\n{sql}" + ); + + // Executing is the real assertion: without the grouping the engine rejects + // the statement outright. + if let Some(result) = ctx.try_execute_pg(query, UNGROUPED_DUP_SEED).await { + insta::assert_snapshot!(result); + } +} + +// A CTE hoisted out of a multi-stage leaf is consumed by the stage above it, +// which re-aggregates its rows. It projects row-level values for that reason, so +// it has to keep grouping to the leaf's own grain no matter what the request +// asked for — an ungrouped request does not make this CTE a raw-row scan. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_request_keeps_group_by_in_hoisted_multi_stage_leaf() { + let schema = + MockSchema::from_yaml_file("common/integration_multi_stage_multiplied_pre_agg.yaml") + .only_pre_aggregations(&[]); + let ctx = TestContext::new(schema).unwrap(); + + let query = indoc! {" + measures: + - customers.total_lifetime_value_prev_month_by_returns + time_dimensions: + - dimension: returns.created_at + granularity: month + ungrouped: true + "}; + + let sql = ctx.build_sql(query).unwrap(); + + // The hoisted leaf is the CTE holding the multiplied-measure keys subquery. + let leaf = cte_body_containing(&sql, "as \"keys\"").unwrap_or_else(|| { + panic!("expected a hoisted keys subquery, got:\n{sql}"); + }); + + assert!( + leaf.to_lowercase().contains("group by"), + "the hoisted leaf feeds a stage that re-aggregates it, so it must stay \ + grouped, got:\n{leaf}" + ); +} diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/mod.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/mod.rs index ae99a03383f86..e27e269f61412 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/mod.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/mod.rs @@ -1,3 +1,4 @@ mod multi_fact; mod multi_stage; mod sql_generation; +mod ungrouped_gate; diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_cross_cube_view_joined_keys_rollup_cubestore_result.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_cross_cube_view_joined_keys_rollup_cubestore_result.snap new file mode 100644 index 0000000000000..8e0d97a48422d --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_cross_cube_view_joined_keys_rollup_cubestore_result.snap @@ -0,0 +1,19 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs +expression: result +--- +visitors_view__visitors_source | visitors_view__visitor_checkins_visitor_id +-------------------------------+------------------------------------------- +google | NULL +google | NULL +google | NULL +google | 1 +google | 1 +google | 1 +google | 8 +organic | NULL +organic | 6 +twitter | 4 +twitter | 4 +twitter | 5 +NULL | 7 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_pre_agg_measure_reads_rollup_column_cubestore_result.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_pre_agg_measure_reads_rollup_column_cubestore_result.snap index 559dcf5521533..4716ffabb09f2 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_pre_agg_measure_reads_rollup_column_cubestore_result.snap +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_pre_agg_measure_reads_rollup_column_cubestore_result.snap @@ -2,12 +2,15 @@ source: cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs expression: result --- -orders__status | orders__city | orders__total_amount ----------------+--------------+--------------------- -cancelled | Chicago | 25 -completed | Boston | 375 -completed | Chicago | 400 -completed | New York | 150 -pending | Boston | 150 -pending | Chicago | 175 -pending | New York | 260 +orders__id | orders__status | orders__city | orders__total_amount +-----------+----------------+--------------+--------------------- +1 | completed | New York | 100 +2 | pending | New York | 200 +3 | completed | New York | 50 +4 | completed | Boston | 300 +5 | pending | Boston | 150 +6 | completed | Boston | 75 +7 | cancelled | Chicago | 25 +8 | completed | Chicago | 400 +9 | pending | Chicago | 175 +10 | pending | New York | 60 diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_view_query_for_join_cubestore_result.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_view_query_for_join_cubestore_result.snap new file mode 100644 index 0000000000000..a04059d459e0b --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/snapshots/cubesqlplanner__tests__integration__pre_aggregations__sql_generation__ungrouped_view_query_for_join_cubestore_result.snap @@ -0,0 +1,16 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs +expression: result +--- +visitors_view__visitors_id | visitors_view__visitors_source +---------------------------+------------------------------- +1 | google +2 | google +3 | google +4 | twitter +5 | twitter +6 | organic +7 | NULL +8 | google +9 | google +10 | organic diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs index a75cd162fcf87..8994412eda9a1 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs @@ -1438,25 +1438,25 @@ async fn test_order_by_only_measure_dropped_from_pre_agg() -> Result<(), CubeErr #[tokio::test(flavor = "multi_thread")] async fn test_ungrouped_pre_agg_measure_reads_rollup_column() -> Result<(), CubeError> { let schema = MockSchema::from_yaml_file("common/pre_aggregation_matching_test.yaml") - .only_pre_aggregations(&["main_rollup"]); + .only_pre_aggregations(&["primary_key_rollup"]); let ctx = TestContext::new(schema)?; let query_yaml = indoc! {" measures: - orders.total_amount dimensions: + - orders.id - orders.status - orders.city ungrouped: true order: - - id: orders.status - - id: orders.city + - id: orders.id "}; let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; assert_eq!(pre_aggrs.len(), 1); - assert_eq!(pre_aggrs[0].name(), "main_rollup"); + assert_eq!(pre_aggrs[0].name(), "primary_key_rollup"); assert!( sql.contains("\"orders__total_amount\" \"orders__total_amount\""), "ungrouped measure should project the rollup column, got:\n{sql}" @@ -1520,3 +1520,175 @@ async fn test_rollup_lambda_cross_cube_union_aliases() -> Result<(), CubeError> Ok(()) } + +// `joined_rollup` is a rollup on the fact cube `visitor_checkins` that includes +// a cross-cube dimension from the many_to_one joined `visitors` cube, plus a +// `time_dimension`. An ungrouped query with no measures and no time dimension +// requests raw rows, so it may only use a pre-aggregation whose dimensions +// cover the primary keys of every cube in the query. Here they don't, so +// reading flat rollup columns would silently drop the join between the two +// cubes: the query must fall back to base SQL. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_no_measures_cross_cube_query_should_not_match_rollup( +) -> Result<(), CubeError> { + let schema = MockSchema::from_yaml_file("common/pre_aggregations_test.yaml") + .only_pre_aggregations(&["joined_rollup"]); + let ctx = TestContext::new(schema)?; + + let query_yaml = indoc! {" + dimensions: + - visitor_checkins.visitor_id + - visitors.source + filters: + - dimension: visitors.source + operator: equals + values: + - google + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "expected no pre-aggregation, got `{}`. Generated SQL:\n{sql}", + pre_aggrs + .iter() + .map(|p| p.name().clone()) + .collect::>() + .join(", ") + ); + + assert!( + sql.to_lowercase().contains("join"), + "expected base SQL with a JOIN between visitor_checkins and visitors, got:\n{sql}" + ); + + Ok(()) +} + +// Same rejection as above, reached through a view. The view renames every +// member, so the ungrouped gate must compare the cubes and primary keys the +// view members resolve to rather than the names the query spells them with. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_cross_cube_view_query_should_not_match_rollup() -> Result<(), CubeError> { + let schema = MockSchema::from_yaml_file("common/pre_aggregations_test.yaml") + .only_pre_aggregations(&["joined_rollup"]); + let ctx = TestContext::new(schema)?; + + let query_yaml = indoc! {" + dimensions: + - visitors_view.visitor_checkins_visitor_id + - visitors_view.visitors_source + filters: + - dimension: visitors_view.visitors_source + operator: equals + values: + - google + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "expected no pre-aggregation, got `{}`. Generated SQL:\n{sql}", + pre_aggrs + .iter() + .map(|p| p.name().clone()) + .collect::>() + .join(", ") + ); + + assert!( + sql.to_lowercase().contains("join"), + "expected base SQL with a JOIN between visitor_checkins and visitors, got:\n{sql}" + ); + + Ok(()) +} + +// The mirror case: `for_join` covers the primary key of the only cube the query +// resolves to, so an ungrouped view query must still match it. Guards the gate +// against reading the view itself as a separate cube, which would make the cube +// sets differ and reject every ungrouped query issued through a view. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_view_query_matches_rollup_covering_primary_key() -> Result<(), CubeError> { + let schema = MockSchema::from_yaml_file("common/pre_aggregations_test.yaml") + .only_pre_aggregations(&["for_join"]); + let ctx = TestContext::new(schema)?; + + let query_yaml = indoc! {" + dimensions: + - visitors_view.visitors_id + - visitors_view.visitors_source + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert_eq!( + pre_aggrs.len(), + 1, + "expected `visitors.for_join` to be used, got SQL:\n{sql}" + ); + assert_eq!(pre_aggrs[0].name(), "for_join"); + assert_eq!(pre_aggrs[0].cube_name(), "visitors"); + + // The rows are the claim: one per raw visitor, not per (id, source) group. + if let Some(result) = ctx + .try_execute(query_yaml, "pre_aggregation_tables.sql") + .await + { + insta::assert_snapshot!("ungrouped_view_query_for_join_cubestore_result", result); + } + + Ok(()) +} + +// `joined_keys_rollup` has the same cross-cube shape as `joined_rollup` but is +// grouped by the primary keys of both cubes, so it does hold one row per raw +// joined row and an ungrouped view query spanning both cubes may use it. The +// contrast with the rejected `joined_rollup` is what the gate is discriminating. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_cross_cube_view_query_matches_rollup_covering_both_primary_keys( +) -> Result<(), CubeError> { + let schema = MockSchema::from_yaml_file("common/pre_aggregations_test.yaml") + .only_pre_aggregations(&["joined_keys_rollup"]); + let ctx = TestContext::new(schema)?; + + let query_yaml = indoc! {" + dimensions: + - visitors_view.visitors_source + - visitors_view.visitor_checkins_visitor_id + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert_eq!( + pre_aggrs.len(), + 1, + "expected `joined_keys_rollup` to be used, got SQL:\n{sql}" + ); + assert_eq!(pre_aggrs[0].name(), "joined_keys_rollup"); + // Tokenized so the check is immune to line breaks and to `join` appearing + // inside the rollup's own table name. + assert!( + !sql.to_lowercase().split_whitespace().any(|t| t == "join"), + "reading the rollup should not re-join the cubes, got:\n{sql}" + ); + + // The rows are the claim the gate rests on: one per raw joined row. + if let Some(result) = ctx + .try_execute(query_yaml, "pre_aggregation_tables.sql") + .await + { + insta::assert_snapshot!( + "ungrouped_cross_cube_view_joined_keys_rollup_cubestore_result", + result + ); + } + + Ok(()) +} diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/ungrouped_gate.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/ungrouped_gate.rs new file mode 100644 index 0000000000000..0cdf81ed88e52 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/ungrouped_gate.rs @@ -0,0 +1,567 @@ +//! Eligibility of pre-aggregations for ungrouped queries. +//! +//! An ungrouped query returns raw rows, so a pre-aggregation may serve it only +//! when each stored row maps to exactly one row of the query result. These tests +//! pin both directions of that rule: rollups whose grouping collapses rows must +//! be refused, and rollups that are provably 1:1 with raw rows must be kept. + +use crate::test_fixtures::cube_bridge::MockSchema; +use crate::test_fixtures::test_utils::TestContext; +use cubenativeutils::CubeError; +use indoc::indoc; + +fn ctx_with(pre_aggregations: &[&str]) -> Result { + let schema = MockSchema::from_yaml_file("common/ungrouped_pre_agg_gate.yaml") + .only_pre_aggregations(pre_aggregations); + TestContext::new(schema) +} + +fn used_names(pre_aggrs: &[crate::logical_plan::PreAggregationUsage]) -> String { + pre_aggrs + .iter() + .map(|p| p.name().clone()) + .collect::>() + .join(", ") +} + +// A cube may legally declare no primary key. Nothing then identifies a raw row, +// so no rollup can be read as raw rows and the query must fall back to the +// source table. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_rollup_when_cube_has_no_primary_key() -> Result<(), CubeError> { + let ctx = ctx_with(&["collapsing"])?; + + let query_yaml = indoc! {" + measures: + - no_pk_orders.total + dimensions: + - no_pk_orders.status + - no_pk_orders.city + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "a cube without a primary key has no raw-row identity, so `{}` must not \ + serve an ungrouped query. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// `chain_checkins` is not named by the query, but it sits between the two cubes +// that are, and its one_to_many edge multiplies rows. The rollup is grouped by +// the visitor and city keys only, so it stores one row per (visitor, city) pair +// while the raw join yields one row per checkin. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_rollup_missing_transit_cube_key() -> Result<(), CubeError> { + let ctx = ctx_with(&["chain_rollup"])?; + + let query_yaml = indoc! {" + dimensions: + - chain_visitors.source + - chain_cities.name + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "the fan-out transit cube `chain_checkins` is unaccounted for, so `{}` \ + must not serve an ungrouped query. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// A many_to_one join cannot multiply rows, so the fact primary key alone +// identifies every stored row and the dimension cube's key is not needed. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_accepts_star_rollup_without_dimension_cube_key() -> Result<(), CubeError> { + let ctx = ctx_with(&["star_rollup"])?; + + let query_yaml = indoc! {" + measures: + - star_checkins.cnt + dimensions: + - star_checkins.id + - star_visitors.source + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert_eq!( + pre_aggrs.len(), + 1, + "the fact key identifies each row across a many_to_one join, so \ + `star_rollup` should be used. Generated SQL:\n{sql}" + ); + assert_eq!(pre_aggrs[0].name(), "star_rollup"); + + Ok(()) +} + +// `own_ref` is a proxy for the fact cube's own primary key, which row identity +// does require, so the key is stored even though it is spelled under another +// name and only a resolved comparison can see it. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_accepts_rollup_storing_primary_key_by_reference() -> Result<(), CubeError> { + let ctx = ctx_with(&["proxy_rollup"])?; + + let query_yaml = indoc! {" + measures: + - star_checkins.cnt + dimensions: + - star_checkins.own_ref + - star_visitors.source + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert_eq!( + pre_aggrs.len(), + 1, + "`own_ref` resolves to `star_checkins.id`, so `proxy_rollup` does store \ + the identifying key and should be used. Generated SQL:\n{sql}" + ); + assert_eq!(pre_aggrs[0].name(), "proxy_rollup"); + + Ok(()) +} + +// A cross-cube segment only filters rows; it neither adds columns nor changes +// the stored grain, so the segment's cube must not be required to contribute a +// primary key. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_accepts_rollup_whose_extra_cube_comes_from_a_segment( +) -> Result<(), CubeError> { + let ctx = ctx_with(&["seg_rollup"])?; + + let query_yaml = indoc! {" + measures: + - star_checkins.cnt + dimensions: + - star_checkins.id + - star_checkins.visitor_id + segments: + - star_visitors.source_google + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert_eq!( + pre_aggrs.len(), + 1, + "a segment only filters, so `seg_rollup` stays 1:1 with raw rows and \ + should be used. Generated SQL:\n{sql}" + ); + assert_eq!(pre_aggrs[0].name(), "seg_rollup"); + + Ok(()) +} + +// The rollup is 1:1 with raw rows, but `uniq` is stored as an HLL sketch that +// only means anything after a merge. An ungrouped read projects the column +// as-is, so it would hand the client a binary sketch instead of a count. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_rollup_storing_non_additive_state() -> Result<(), CubeError> { + let ctx = ctx_with(&["hll_pk_rollup"])?; + + let query_yaml = indoc! {" + measures: + - hll_orders.uniq + dimensions: + - hll_orders.id + - hll_orders.status + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "an ungrouped read cannot finalize the stored HLL state, so `{}` must \ + not serve this query. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// A fan-out measure under `ungrouped` is rendered inline rather than through a +// multiplied subquery, so this stays a simple query. The rollup groups by +// non-key columns and must be refused. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_collapsing_rollup_with_fan_out_measure() -> Result<(), CubeError> { + let ctx = ctx_with(&["mult_collapsing"])?; + + let query_yaml = indoc! {" + measures: + - mult_customers.count + dimensions: + - mult_customers.name + - mult_orders.status + ungrouped: true + cubestoreSupportMultistage: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "`{}` groups by non-key columns, so it must not serve an ungrouped \ + query. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// A non-additive rolling measure makes the planner render its leaf stage +// ungrouped, but the user query is grouped and asks for aggregates, so the +// raw-row rule must not be applied to that leaf. +#[tokio::test(flavor = "multi_thread")] +async fn test_grouped_non_additive_rolling_query_still_uses_rollup() -> Result<(), CubeError> { + let ctx = ctx_with(&["rolling_avg_rollup"])?; + + let query_yaml = indoc! {r#" + measures: + - roll_orders.rolling_avg_7d + dimensions: + - roll_orders.category + time_dimensions: + - dimension: roll_orders.created_at + granularity: day + dateRange: + - "2024-01-10" + - "2024-01-25" + cubestoreSupportMultistage: true + "#}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert_eq!( + pre_aggrs.len(), + 1, + "the user query is grouped, so the internal ungrouped leaf must not cost \ + it `rolling_avg_rollup`. Generated SQL:\n{sql}" + ); + assert_eq!(pre_aggrs[0].name(), "rolling_avg_rollup"); + + Ok(()) +} + +// `lam_union` exposes only its first member rollup's symbols. The second branch +// groups by a non-unique column that merely shares the short name `id`, so the +// union as a whole is not 1:1 with raw rows. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_lambda_whose_other_branch_collapses() -> Result<(), CubeError> { + let ctx = ctx_with(&["lam_base", "lam_union"])?; + + let query_yaml = indoc! {" + measures: + - lam_a.count + dimensions: + - lam_a.id + - lam_a.visitor_id + time_dimensions: + - dimension: lam_a.created_at + granularity: day + ungrouped: true + pre_aggregation_id: lam_a.lam_union + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "the union's second branch is grouped on a non-key column, so `{}` must \ + not serve an ungrouped query. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// `rj_keys_join` declares both primary keys, but the rollups it actually reads +// store neither `rj_checkins.id` nor a per-checkin grain, so its rows are one +// per visitor. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_rollup_join_advertising_unstored_keys() -> Result<(), CubeError> { + let ctx = ctx_with(&["rj_base", "rj_keys_join"])?; + + let query_yaml = indoc! {" + measures: + - rj_checkins.count + dimensions: + - rj_checkins.visitor_id + - rj_visitors.source + ungrouped: true + pre_aggregation_id: rj_checkins.rj_keys_join + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "the member rollups do not store `rj_checkins.id`, so `{}` must not serve \ + an ungrouped query. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// The single-cube shape: the cube has a key, the rollup omits it, so its rows +// are collapsed and an ungrouped query must read the source table instead. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_single_cube_rejects_rollup_missing_primary_key() -> Result<(), CubeError> { + let ctx = ctx_with(&["sc_coarse"])?; + + let query_yaml = indoc! {" + measures: + - sc_orders.total + dimensions: + - sc_orders.status + - sc_orders.city + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "`{}` groups by non-key columns, so it must not serve an ungrouped \ + single-cube query. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// A filter is enough to pull a fan-out cube into the join even though the query +// selects none of its members, so row identity has to account for it. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_rollup_when_a_filter_widens_the_join() -> Result<(), CubeError> { + let ctx = ctx_with(&["chain_rollup"])?; + + let query_yaml = indoc! {" + dimensions: + - chain_visitors.id + - chain_visitors.source + filters: + - dimension: chain_cities.name + operator: equals + values: + - X + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "the filter joins in the fan-out cube `chain_checkins`, so `{}` must not \ + serve an ungrouped query. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// The mirror direction of the rule: the rollup is grouped across a fan-out cube +// the query never joins, so it holds more rows than the query asks for and would +// return each visitor once per city. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_rollup_stored_at_a_finer_grain() -> Result<(), CubeError> { + let ctx = ctx_with(&["chain_rollup"])?; + + let query_yaml = indoc! {" + dimensions: + - chain_visitors.id + - chain_visitors.source + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "`{}` is stored per (visitor, city) while the query reads one row per \ + visitor, so it must not serve it. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// A segment only filters, but reaching it joins in the fan-out cube, so a rollup +// keyed on the root alone no longer holds one row per output row. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_rollup_whose_segment_crosses_a_fan_out() -> Result<(), CubeError> { + let ctx = ctx_with(&["root_key_seg_rollup"])?; + + let query_yaml = indoc! {" + dimensions: + - chain_visitors.id + - chain_visitors.source + segments: + - chain_cities.named_x + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "the segment joins in the fan-out cube `chain_checkins`, so `{}` must not \ + serve an ungrouped query. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// `bridge_stores` only bridges two many_to_one edges, so it cannot split a row +// and its key is no part of row identity — the fact key alone identifies the +// output row even though the bridge sits in the middle of the join. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_accepts_rollup_across_a_many_to_one_bridge() -> Result<(), CubeError> { + let ctx = ctx_with(&["bridge_rollup"])?; + + let query_yaml = indoc! {" + measures: + - bridge_orders.cnt + dimensions: + - bridge_orders.id + - bridge_regions.name + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert_eq!( + pre_aggrs.len(), + 1, + "a many_to_one bridge cannot split a row, so `bridge_rollup` should be \ + used. Generated SQL:\n{sql}" + ); + assert_eq!(pre_aggrs[0].name(), "bridge_rollup"); + + Ok(()) +} + +// A measure joins whatever its own sql references, but the rollup aggregates +// that join away, so the referenced cube is no part of the stored grain and must +// not make the rollup ineligible. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_accepts_rollup_with_a_measure_reaching_another_cube( +) -> Result<(), CubeError> { + let ctx = ctx_with(&["mult_keyed"])?; + + let query_yaml = indoc! {" + dimensions: + - mult_customers.id + - mult_customers.name + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert_eq!( + pre_aggrs.len(), + 1, + "`mult_keyed` is grouped by the primary key, so it should be used. \ + Generated SQL:\n{sql}" + ); + assert_eq!(pre_aggrs[0].name(), "mult_keyed"); + + Ok(()) +} + +// A rollup's segments are appended to its dimension list when the table is +// materialized, so a segment groups the stored rows just like a dimension. One +// reaching across a row-splitting join therefore stores a row per (entity, +// segment value) even though the query never asks for the segment. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_rejects_rollup_whose_unrequested_segment_splits_rows( +) -> Result<(), CubeError> { + let ctx = ctx_with(&["root_key_seg_rollup"])?; + + let query_yaml = indoc! {" + dimensions: + - chain_visitors.id + - chain_visitors.source + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert!( + pre_aggrs.is_empty(), + "`{}` groups by a segment of `chain_cities`, reached over the fan-out to \ + `chain_checkins`, so it holds more than one row per visitor. Generated \ + SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} + +// A rollup stores its time dimension truncated to `granularity`, so that column +// must never reach a raw-row result. It cannot: a time dimension requested +// without a granularity is a filter, not an output member, and one requested +// with a granularity only matches a rollup storing that same granularity. +#[tokio::test(flavor = "multi_thread")] +async fn test_ungrouped_never_reads_a_truncated_time_dimension() -> Result<(), CubeError> { + let ctx = ctx_with(&["star_day_rollup"])?; + + let query_yaml = indoc! {" + measures: + - star_checkins.cnt + dimensions: + - star_checkins.id + time_dimensions: + - dimension: star_checkins.created_at + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(query_yaml)?; + + assert_eq!( + pre_aggrs.len(), + 1, + "`star_day_rollup` is grouped by the primary key, so it holds one row per checkin and may serve raw rows. Generated SQL:\n{sql}" + ); + assert!( + !sql.contains("created_at"), + "the stored time dimension is truncated to the day, so no raw-row result may read it. Generated SQL:\n{sql}" + ); + + // Asking for the raw value as a plain dimension finds no match at all: the + // rollup groups by `id` alone and stores no untruncated timestamp. + let raw_value_query = indoc! {" + dimensions: + - star_checkins.id + - star_checkins.created_at + ungrouped: true + "}; + + let (sql, pre_aggrs) = ctx.build_sql_with_used_pre_aggregations(raw_value_query)?; + + assert!( + pre_aggrs.is_empty(), + "`{}` stores no untruncated timestamp. Generated SQL:\n{sql}", + used_names(&pre_aggrs) + ); + + Ok(()) +} diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__modifiers__ungrouped_multiplied_keeps_group_by_for_aggregate_sql_measure.snap b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__modifiers__ungrouped_multiplied_keeps_group_by_for_aggregate_sql_measure.snap new file mode 100644 index 0000000000000..8900c105826d9 --- /dev/null +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/snapshots/cubesqlplanner__tests__integration__modifiers__ungrouped_multiplied_keeps_group_by_for_aggregate_sql_measure.snap @@ -0,0 +1,7 @@ +--- +source: cubesqlplanner/cubesqlplanner/src/tests/integration/modifiers.rs +expression: result +--- +dup_customers__name | dup_orders__status | dup_customers__ltv_agg_number +--------------------+--------------------+------------------------------ +dup | new | 150.00