fix(tesseract): resolve FILTER_PARAMS column callback references - #11460
fix(tesseract): resolve FILTER_PARAMS column callback references#11460waralexrom wants to merge 9 commits into
Conversation
github-actions
Bot
added
rust
…bers
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:<cube>.<member>:<column>}`, 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) <noreply@anthropic.com>
…f 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…t 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) <noreply@anthropic.com>
…pendencies 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) <noreply@anthropic.com>
…enders 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) <noreply@anthropic.com>
c1ea9ec to
038bc9a
Compare
|
Claude finished @waralexrom's task in 5m 3s —— View job Review complete — no blockers; 1 medium (stale activity window), 3 low, 4 inline commentsFull review — findings, rationale, and what I checked
OverallThis is a careful, well-reasoned change. Compiling the column callback into a The comment quality throughout is genuinely above average — Test coverage is strong: both planners run the same assertions, the reported model is covered on both the cube read and the pre-aggregation build, and the parameter-list shapes that can't be parsed are pinned as staying on the render-time path. Findings
Two more I looked at and am satisfied by, noted here rather than inline:
Not verifiedI could not run the suites: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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. | ||
| let all_filters = self.all_filter_items(); | ||
| for dim in self.dimensions.iter_mut() { | ||
| *dim = transforms::apply_filter_params_activity_to_symbol(dim, &all_filters)?; |
There was a problem hiding this comment.
Question on the staleness window. Activity is settled here, at construction, against all_filter_items() — but rendering (SqlCall::eval_filter_group → visitor.all_filters(), base_filter.rs) reads the live filter set at plan time. Several mutators change that set afterwards and none re-settle:
add_dimension_filter/add_dimension_filters/add_time_dimension_filters— can turn a binding that renders1 = 1at settle time into one that renders its column. Itscompiled_calldeps were never contributed, so the cube it reads may not be in the join and the column renders a qualifier for a table nothing joins.remove_filters_for_members/keep_only_filters_for_members— the benign direction (a staleactive: truecontributes a join that goes unused).
The multi-stage path is covered: make_queries_descriptions re-settles the member against the stage's own state after apply_filter_directive_to_state. Two gaps I couldn't rule out by reading:
- Inside a stage, only the member is re-settled — the state's own filter-item symbols (and order items) keep the outer query's activity, so a symbol reached only through a stage-local
filter: includepredicate could render an unsettled column. keys_sub_query.rs:46renders withkeys_subquery.filter().all_filters(), a set derived independently of the one settled here.
If both are provably subsets (render ⊆ settled), a comment saying so on apply_static_filters would save the next reader the same walk. If not, re-settling in add_*_filters (or asserting in eval_filter_group that a rendering binding is active) would turn a silent bad-SQL case into a loud one.
| const { count, rest } = declaredValueParams(column); | ||
| if (rest || !valueParamsAreCertain(column, count)) { | ||
| return column; | ||
| } | ||
|
|
||
| const values = []; | ||
| for (let i = 0; i < count; i++) { | ||
| values.push(placeholder(FILTER_VALUE_PREFIX, i)); |
There was a problem hiding this comment.
Worth noting in the changelog: this moves the user callback's invocation from render time (once per query that actually uses the filter) to compile time (once, unconditionally, for every member whose sql declares the column). Two consequences that don't show up in the tests:
- A callback that throws — or that inspects its arguments as anything but opaque strings — now fails model compilation rather than the one query that uses it. The blast radius goes from a query to the whole schema.
- A callback with side effects, or one that isn't referentially transparent, is now called exactly once and its output cached in the template.
Both are almost certainly fine for the documented shape (string interpolation of param placeholders), but they're user-visible enough to belong in the release note alongside the two new hard errors (value_params_count mismatch, and member references in a cube's sql).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11460 +/- ##
==========================================
+ Coverage 79.45% 79.47% +0.01%
==========================================
Files 480 480
Lines 98810 98861 +51
Branches 3636 3636
==========================================
+ Hits 78511 78567 +56
+ Misses 19778 19773 -5
Partials 521 521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…omething 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) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 2m 28s —— View job Re-review of
|
| # | Prior finding | Status |
|---|---|---|
| 2 | valueParamsAreCertain leans on Function.length, which is 0 once param #1 is defaulted |
Partly fixed — the reported case is closed and pinned by a test; the same class survives one parameter to the right (new inline comment) |
| 4 | AND FilterGroup rebuilt per symbol |
Fixed — filter_params_activity_filters builds it once per pass; apply_filter_params_activity_to_symbol and …_to_filter_item now take &FilterItem |
| 1 | Activity settles at construction, render reads the live filter set | Documented, not closed — thread still open |
| 3 | Callback invocation moved to compile time; release-note material | Open — no changelog/docs change in this PR |
New finding — the guard still under-counts
fn.length > 0 short-circuits before the text check, but Function.length is a lower bound: it stops at the first default and can't vouch for anything after it either.
(from, to = ')', third) => `d >= ${from} AND d < ${to} AND x = ${third}`fn.length is 1; the scan closes at the ) inside the string default → count: 2; 2 >= 1 passes and fn.length > 0 skips the inner test. Compiled with two placeholders, third renders as the literal undefined — the exact failure the guard exists to prevent. The two shapes the suite does catch are caught for other reasons: (from = ')', to) has fn.length === 0 and so reaches the text check, and (from, /* ) */ to, third) is rejected by count < fn.length because comments don't reduce Function.length.
Fix is to drop the short-circuit — return !/['"\]|/*|///.test(inner)` — which costs a quote-in-default callback its compiled form and sends it to the render-time path, where master has it today. Suggestion posted inline. Fix this →
Still open from before
- Staleness window (
query_properties.rs:240). The new comment is a real improvement — it states plainly that this runs once at construction and that mutators do not re-apply it, and it records why a multi-stage stage settles correctly (it builds its ownQueryProperties). What it doesn't answer is the widening direction:add_dimension_filter(s)/add_time_dimension_filterscan add a predicate after settling, turning a binding that settled inactive into one that renders its column with dependencies never contributed to the join. Thread left unresolved. Not a blocker if those callers are provably confined to members already in the join — worth one sentence saying so if they are. - Release note (finding ResultSet pivot methods documentation #3). Compile-time invocation of the user callback, plus the two new hard errors (
value_params_countmismatch, member references in a cube'ssql), are user-visible and undocumented in this PR.
Verification
The Function.length analysis is by reading declaredValueParams and the guard directly — the sandbox here blocks node, so I traced the scan by hand rather than executing it. I could not run the Jest or cargo test suites (node_modules not installed in this checkout). The Rust changes in this commit are mechanical (a &[FilterItem] → &FilterItem signature change with the group hoisted to the caller) and both call sites are updated.
Resolved
Two of my earlier threads — the Function.length hole as originally reported, and the per-symbol group rebuild — are addressed and resolved.
`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) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 1m 45s —— View job Re-review of
|
| Shape | fn.length |
scan count |
Verdict |
|---|---|---|---|
(from, to = ')', third) |
1 | 2 | rejected by the text check ✓ (was silently compiled with 2 placeholders) |
(from = ')', to) |
0 | 1 | rejected ✓ |
(from, /* ) */ to, third) |
3 | 2 | rejected by count < fn.length ✓ |
(from, to) |
2 | 2 | compiled ✓ |
(from, to = 1) |
1 | 2 | compiled ✓ (numeric default, no quote) |
The test-suite bookkeeping is consistent with the tightening: (from, to = 'x') moved from the compiled case to the uncompiled list, and the "counts a defaulted parameter as a filter value" test switched its default to 1 so it still exercises the compiled path. Both new cases — (from, to = ')', third) and the bare (from, to = 'x') — are pinned. I checked filter-params-callback-column.test.ts for callbacks with quoted defaults that would now silently move to the render-time path: there are none, so the integration assertions are unaffected.
The comment rewrite is an improvement on an already-good comment — it now states why Function.length can't speak past the first default, in both directions.
One contrived edge left (low, optional)
The text check screens quotes and comment openers, but not a regex literal:
FILTER_PARAMS.orders.a.filter((from, to = /)/, third) => `…`)The depth scan closes on the ) inside the regex → inner = "from, to = /" → count: 2, fn.length: 1, and /['"]|/*|///doesn't match a lone/. So thirdrenders as the literalundefined— the same failure class, one more character. Adding/ to the class closes it (/['"\/]/ subsumes both comment alternatives too); the cost is that (from, to = x/2) also falls back, which is the safe direction. Genuinely contrived — a regex default in a filter-param callback — so this is a nit, not a request. I'm not posting it inline; the previous two rounds on this line are enough.
Still open from before
- Staleness window (
query_properties.rs:250) — unchanged in this commit. Activity settles at construction againstall_filter_items();add_dimension_filter(s)/add_time_dimension_filterscan widen the set afterwards without re-settling, turning a binding that settled inactive into one that renders its column with dependencies never contributed to the join. Not a blocker if those callers are provably confined to members already in the join — one sentence saying so would close it. Thread left unresolved. - Release note (prior finding ResultSet pivot methods documentation #3) — unchanged. Compile-time invocation of the user callback (a throwing callback now fails model compilation, not one query) plus the two new hard errors (
value_params_countmismatch, member references in a cube'ssql) are user-visible and undocumented in this PR.
Verification
node is blocked in this sandbox and node_modules is not installed, so I could not execute the Jest suite or cargo test — the guard analysis above is by reading declaredValueParams and valueParamsAreCertain and tracing the scan by hand. The diff since 503d0e4 is two files, 17/-11, all in the JS compiler and its unit test; no Rust changed, so my prior Rust review stands as-is.
Resolved
My thread on the fn.length > 0 short-circuit is addressed and resolved. Two of my threads stay open (staleness, release note) — both are judgement calls for the author, neither blocks.
…r 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) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 3m 45s —— View job Re-review of
|
| expect(sql).not.toMatch(PUSHED_DOWN_PREDICATE); | ||
| expect(sql).toContain('1 = 1'); |
There was a problem hiding this comment.
The claim this test rests on — "the filter still reaches the query on its own", which is what makes dropping the restatement safe rather than result-changing — is the one thing it doesn't assert. Both assertions here are negative-or-weak: the restatement is absent, and 1 = 1 appears somewhere (satisfiable by any other always-true in the plan).
Worth pinning the positive half, so a future change that drops the outer predicate too fails here instead of silently widening the result set:
| expect(sql).not.toMatch(PUSHED_DOWN_PREDICATE); | |
| expect(sql).toContain('1 = 1'); | |
| expect(sql).not.toMatch(PUSHED_DOWN_PREDICATE); | |
| expect(sql).toContain('1 = 1'); | |
| // The filter itself still reaches the query — that is what makes dropping | |
| // only the restatement narrower than binding a bound the filter never gave. | |
| expect(sql).toMatch(/"commission"\.reconciliation_date <=? \$\d+/); |
(Adjust the operator to whatever beforeDate renders in this dialect.)
1 participant
Summary
A
FILTER_PARAMS.….filter(cb)column callback failed to render under the native planner withPlaceholder {arg:0} out of boundswhenever the callback referenced a member. The callback is a SQL function in its own right, but its placeholders were resolved against the enclosing member's dependency list — a list already read by the time the callback recorded anything into it. It is now compiled into aSqlCallof its own, and the dependencies it reads reach the join the same way any other reference does.Reported for a
summeasure whosefilters:entry was only a FILTER_PARAMS column; both the query and the pre-aggregation build SQL now match the legacy planner for that model.Changes
{fpv:N}value placeholders and its member references record into its own lists, so the placeholders it emits index its own dependencies. That fixes the reported error for every spelling of a reference —${CUBE.x},${cube.x}, another cube — and gives the column its own parenthesisation contexts, so a compound member inside it is parenthesised correctly (the legacy planner still getstotal - fee * 100wrong here).filter: includelets a subquery filter a member the query around it does not. An active column's dependencies flow into join hints and pull the cube it reads into the join; an inactive one contributes nothing, so a query that never uses that filter plans as though the column were absent. Where the join cannot be built, the join graph says so itself, in the same terms as for any other reference.set,notSet) or fewer than the column declares (a one-sided date operator against a column taking both bounds). The filter still reaches the query on its own; only its restatement inside the member's SQL is dropped. The legacy planner instead fills the missing bound in with the current time, which forbeforeDatelands the given value on the opposite side of the range — leaving the measure empty for every row the query keeps, and making the SQL for one query change from day to day.sqlis rejected. That sql builds the table the query reads from, so nothing a reference could resolve against is in scope. All three spellings — direct, string column, callback column — used to either recurse until the stack ran out or render a qualifier for a table nothing joins. Callbacks themselves stay allowed there, along with everything in them that needs no member in scope: the filter values and any security context value, so the row-level-security shape keeps working.Left deliberately untouched, all outside the reported shape and unchanged from master: a callback taking its values through a rest parameter, which no fixed set of placeholders can express; a time shift on a compiled column, now refused explicitly instead of silently emptying a shifted CTE; and a nested
FILTER_PARAMSinside a callback.Behaviour worth a release note
The column callback moves from render time to compile time. It used to be invoked
once per query that actually used the filter; it is now compiled once per query
for every member whose sql declares it, whatever the query filters. For the
documented shape — interpolating the placeholders it is handed into a SQL string
— that is invisible. A callback that throws on an argument it did not expect, or
that is not referentially transparent, behaves differently.
A reference inside a callback is resolved at that point too, so a reference to a
member that no longer exists now fails every query touching that member rather
than only the ones supplying its filter. A model carrying a dead FILTER_PARAMS
branch will surface it on upgrade without any query changing.
Two new user-facing errors, both replacing something worse:
sqlis reported instead of recursinguntil the stack runs out, or rendering a qualifier for a table nothing joins;
which used to restrict a shifted CTE to the current period and empty it.
Testing
test/unit/filter-params-callback-column.test.tsis the regression guard, written red before the fix and run against both planners: the reported model on the cube read and on the pre-aggregation build query; a column reading another cube, joined when the filter reaches the query and left out when it does not; a column reached through a segment, through a dimension named only in a filter, through a measure named only in a having filter, and through aFILTER_GROUPunder anorfilter; the value-count cases; and the cube-sqlrejections next to the shapes that stay allowed.test/unit/member-sql-template-compiler.test.tscovers the compiler contract, including the parameter-list shapes that cannot be read in full and so stay on the render-time path.Verified against the legacy planner as the oracle throughout — including that a
hasManyjoin and a missing join path produce the same errors as legacy, and that pre-aggregation matching is unchanged for own-cube and cross-cube columns.1118 Rust unit tests; the schema-compiler unit suite (only the pre-existing
error-reporterANSI snapshot failures remain); andcube-views,sql-generation,pre-aggregations,multi-stage,pre-aggregations-multi-stage,multi-stage-time-shift-filter-paramsPostgres integration suites underCUBEJS_TESSERACT_SQL_PLANNER=true.