fix(tesseract): resolve FILTER_PARAMS column callback references - #11460
fix(tesseract): resolve FILTER_PARAMS column callback references#11460waralexrom wants to merge 7 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% 83.95% +4.49%
==========================================
Files 480 257 -223
Lines 98810 80919 -17891
Branches 3636 0 -3636
==========================================
- Hits 78511 67936 -10575
+ Misses 19778 12983 -6795
+ Partials 521 0 -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.
| if (count < fn.length) { | ||
| return false; | ||
| } | ||
| return fn.length > 0 || !/['"`]|\/\*|\/\//.test(inner); |
There was a problem hiding this comment.
The fn.length > 0 short-circuit still leaves the hole open one parameter to the right. Function.length is a lower bound, so it rescues the parse only up to the first default — a ) inside a later default breaks the scan just as badly, and the fn.length > 0 branch skips the text check that would have caught it.
FILTER_PARAMS.orders.a.filter((from, to = ')', third) => `d >= ${from} AND d < ${to} AND x = ${third}`)fn.lengthis1(params before the first default).- The scan closes at the
)inside the string default →inner = "from, to = '"→count: 2. - Guard: not native ✓,
2 >= 1✓,fn.length > 0→ returnstruewithout ever testinginner. - Compiled with two placeholders;
thirdrenders as the literalundefined, which is the failure this guard exists to prevent.
The new (from = ')', to) test passes because fn.length is 0 there, so it reaches the text check. The (from, /* ) */ to, third) shape is caught for a different reason — comments don't reduce Function.length, so count(2) < fn.length(3) rejects it.
The check is already written; it just needs to apply unconditionally:
| return fn.length > 0 || !/['"`]|\/\*|\/\//.test(inner); | |
| // `Function.length` is a lower bound — it stops at the first defaulted | |
| // parameter and can vouch for nothing after that, in either direction. So the | |
| // text must also hold nothing for the scan to trip over. | |
| return !/['"`]|\/\*|\/\//.test(inner); |
That costs a callback whose default happens to contain a quote but no paren (e.g. (from, to = 'x')) its compiled form — it falls back to the render-time path, which is where master has it today and where every other unreadable list already goes. Safe direction; silently emitting undefined into generated SQL is not.
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/notSetsemantics. Fewer values than the column declares is reported rather than rendered: the legacy planner fills the missing bound in with the current time, quietly widening the predicate to a range the filter never asked for.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.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.