feat(tesseract): push a segment named in FILTER_PARAMS into the cube's sql - #11517
Conversation
…s sql `FILTER_PARAMS.<cube>.<segment>.filter(...)` compiled without error but rendered `1 = 1`, leaving the segment predicate in the outer WHERE after the joins. On a cube wrapping a large union scan that predicate is what makes the scan viable, so the query never completes. A segment is compared to no value, so the `filter()` argument is the whole predicate rather than the left side of one. It is stated there rather than taken from the segment's own `sql`, which prefixes its columns with the cube — not in scope inside the sql that builds that cube, the same reason a dimension binding restates its column. `find_subtree_for_members` now matches a `Segment` node when a target names it, which also activates the binding through the existing activity visitor, and `BaseSegment` renders the binding's column when the filters context carries one. A segment absent from the query still renders `1 = 1`. The name is matched along the reference chain, so a view re-exporting a segment activates the underlying cube's binding. The walk stops at the first symbol that is not a member expression: a segment whose sql is a bare reference resolves on to that dimension, whose binding states a column and not a predicate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
github-actions
Bot
added
rust
|
Claude finished @waralexrom's task in 7m 51s —— View job Review complete — approach is sound; 2 medium, 4 low. 6 inline comments posted.Full review
OverallThe mechanism is a good fit for the existing design: reusing Rendering safety is fine: the raw string column is returned unparenthesised, but every I could not run Findings
Things done well
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if let Some(item) = filters_ctx | ||
| .filter_params_columns | ||
| .iter() | ||
| .filter(|(name, _)| self.matches_member_name(name)) | ||
| .min_by_key(|(name, _)| *name) | ||
| .map(|(_, item)| item) | ||
| { | ||
| return self.filter_params_column_sql( | ||
| item, | ||
| visitor, | ||
| node_processor, | ||
| query_tools, | ||
| templates, | ||
| ); | ||
| } | ||
|
|
||
| let sql = visitor.apply(&self.member_evaluator(), node_processor, templates)?; | ||
| if filters_ctx.reading_pre_aggregation { |
There was a problem hiding this comment.
Two smaller points on this lookup:
-
Alphabetical order is a weaker tiebreak than specificity. When both a cube's own path and a view's re-export path have bindings in the same group,
min_by_keypicks the lexicographically smaller name, which happens to favourorders.completedoverorders_view.completedbut is arbitrary in general — the binding whose name is exactly what the query asked for (self.full_name) is the one the user meant. Tryingfilter_params_columns.get(&self.full_name)first and only then falling back to the chain scan also mirrors the dimension path (base_filter.rs:24uses a plainget) and skips a full-map scan per segment render. -
This early return precedes the
reading_pre_aggregationbranch below. Unreachable today, since the only context carrying a non-emptyfilter_params_columnsisnew_for_filter_params, whicheval_filter_groupalways builds with a freshSqlNodesFactory::new()(soreading_pre_aggregationis false). But if a pre-agg-reading factory ever reaches this context, a segment would render the pushdown predicate against raw columns instead ofwrap_segment_filterover the stored pre-aggregation column — invalid SQL rather than a planning error. Cheap to make explicit by gating the new block on!filters_ctx.reading_pre_aggregation.
| let mut current = Some(self.member_evaluator.clone()); | ||
| while let Some(symbol) = current { | ||
| if symbol.as_member_expression().is_err() { | ||
| return false; | ||
| } | ||
| // A segment symbol lives in the `expr:` namespace, so the path is | ||
| // reassembled from the cube and member names it was compiled under. | ||
| if format!("{}.{}", symbol.cube_name(), symbol.name()) == member { | ||
| return true; | ||
| } | ||
| current = symbol.reference_member(); | ||
| } |
There was a problem hiding this comment.
Nit: the first iteration of the loop re-checks what line 72 already covered — for a cube-level segment member_evaluator's cube_name/name reassemble to the same path as self.full_name. Harmless, just slightly obscures that the loop exists purely for the view re-export hop.
The chain walk itself reads correctly to me, and the as_member_expression().is_err() stop is the right guard — worth noting the reasoning is well captured in the doc comment and pinned down by segment_referencing_a_dimension_does_not_activate_that_dimensions_binding.
One asymmetry worth being aware of: FilterItem::Item matching in tree.rs:195 compares only the end of the reference chain (resolve_reference_chain().full_name()), while segments match any hop. That's justified here (a dimension binding is about the column, a segment binding about the predicate), but the two matching rules now differ in kind, so a future change to one won't obviously carry to the other.
| sql: \`${events} WHERE \${FILTER_PARAMS.events.start_load.filter("evid = 115 AND action_group = 'load'")} | ||
| AND \${FILTER_PARAMS.events.region.filter('region')}\`, |
There was a problem hiding this comment.
Nit: the events cube uses two FILTER_PARAMS in one sql without wrapping them in FILTER_GROUP, which the docs page this PR edits states is required ("If you use FILTER_PARAMS in your query more than once, you must wrap them with FILTER_GROUP"). It works here because the two are ANDed explicitly, and the grouped_events cube covers the grouped form — but a fixture that contradicts the documented rule is an odd thing to point future readers at. Splitting the second binding into its own cube (or wrapping both) keeps the fixture on-spec without losing any assertion.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11517 +/- ##
==========================================
- Coverage 83.96% 79.46% -4.50%
==========================================
Files 257 480 +223
Lines 80919 98861 +17942
Branches 0 3636 +3636
==========================================
+ Hits 67940 78563 +10623
- Misses 12979 19778 +6799
- Partials 0 520 +520
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:
|
…o always-true A column that takes filter values cannot render for a segment, which supplies none. Raising a user error there fails a query that a model already serving `1 = 1` used to answer, and it is louder than what the same mismatch gets on the dimension path, which drops only the restatement inside the SQL. An argument the binding cannot resolve at all stays silent too, so the arity is not the place to start reporting. Both callback variants now degrade. A callback declaring no parameters is compiled with none, so the raw-callback variant only ever holds a column that wants values — a rest parameter, or a parameter list that could not be read — and invoking it with none interpolated `undefined` into the cube's sql. The binding is looked up by the path the query asked for before scanning the chain, which is both what the model meant and what the dimension path does. Covers the compiled column in the Postgres suite, which the mock DSL cannot express: the renderer parenthesizes a binding it reached, telling an activated-then-dropped column apart from an unselected segment's bare `1 = 1`.
|
Claude finished @waralexrom's task in 2m 8s —— View job Follow-up review of
|
| # | Sev | Status |
|---|---|---|
| 1 | Medium | Fixed. value_params_count > 0 now returns templates.always_true(), matching typed_filter.rs:105. No longer a breaking change for models that render 1 = 1 today. Thread resolved. |
| 2 | Medium | Fixed, and better than the symmetry I asked for. Collapsing Callback into the same arm means the undefined-interpolation path is gone by construction rather than by parsing harder on the JS side — no reliance on Function.length to tell a rest parameter from an unparseable list. Thread resolved. |
| 3.1 | Low | Fixed. Exact-name get first, chain scan only as fallback, so the alphabetical tiebreak now only arbitrates between two non-exact chain matches. |
| 3.2 | Low | Open. The matching_filter_params_column early return still precedes the reading_pre_aggregation branch (base_segment.rs:22-33). Still unreachable today, still a one-line guard. Thread left unresolved. |
| 5 | Low | Fixed. The Postgres suite now covers both remaining arms — renders a callback column that takes no filter values hits Compiled with value_params_count == 0, and leaves a value-taking callback column always-true hits the degrade. Only the compiled_call.is_none() internal-invariant error is uncovered, which is fine. Thread resolved. The Rust mock DSL still emits String only; not worth adding now that the JS suite covers the arms. |
| 6 | Nit ×2 | Open, and the new callback_events fixture repeats the un-grouped-FILTER_PARAMS shape. Threads left unresolved. |
Verification of the new arms
I checked the classification the two arms depend on against MemberSqlTemplateCompiler.js:
() => "evid = 115"→declaredValueParamsgivescount: 0, rest: false,valueParamsAreCertainis true →Compiled { valueParamsCount: 0 }. So the positive test genuinely exercisescall.eval, and the comment "nothing else in this cube's sql states the predicate" is the right way to assert it.(v) => 'evid = ' + v→Compiled { valueParamsCount: 1 }→ degrade.- Rest parameter / unparseable list →
Callback→ degrade.
The negative test's parenthesis check is a real discriminator, not incidental: an unselected binding returns templates.always_true() straight out of eval_filter_group (sql_call.rs:534) unwrapped, while an activated one goes through render_filter_item, which parenthesises. So AND \(1 = 1\) distinguishes activated-then-dropped from never-activated, exactly as the comment claims. The not.toMatch(/undefined|\{fpv:/) assertion pins finding 2 shut.
One consequence worth being aware of, not a defect: a genuinely zero-value rest-parameter column ((...vals) => "evid = 115") could have pushed down and now renders 1 = 1. The docs sentence ("as long as it takes no arguments") covers it, and preferring that over an undefined risk is the right trade.
Docs
The added paragraph is accurate and lands in the right place. Slightly understated in one respect: the degrade is silent, so a model author who writes a value-taking callback gets a working-but-unpushed query with no signal. Given finding 1 that's the deliberate choice; if you ever want a signal, a compile-time warning is the place for it, not render time.
I could not run cargo test or the Postgres suite here (both need approval), so the green-test claims are taken at face value; everything above is from reading the code.
2 participants
Summary
FILTER_PARAMS.<cube>.<segment>.filter(...)compiled without error but rendered1 = 1, so the segment predicate stayed in the outerWHEREafter the joins. On a cube wrapping a largeUNION ALLscan that predicate is the one that makes the scan viable, and the query never completes (Trino timeout). Segments are now matchable asFILTER_PARAMStargets under Tesseract.A segment is compared to no value, so the
filter()argument is the whole predicate rather than the left side of one. It is stated there rather than taken from the segment's ownsql, which prefixes its columns with the cube — not in scope inside the sql that builds that cube, the same reason a dimension binding restates its column.Before / after, for a query selecting the segment:
Resolves CORE-712.
Changes
planner/filter/tree.rs—find_subtree_for_membersmatches aSegmentnode when a target names it, instead of always skipping it. The same function drivesSqlCallFilterParamsItem.active, so the binding activates without extra wiring.planner/filter/base_segment.rs—BaseSegment::matches_member_namematches the path the query asked for, then walks the reference chain, so a view re-exporting a segment activates the underlying cube's binding. The walk stops at the first symbol that is not a member expression: a segment whosesqlis a bare reference resolves on to that dimension, whose binding states a column and not a predicate.physical_plan/filter/base_segment.rs— a segment named by a binding in the filters context renders that binding's column: a string verbatim, a compiled callback evaluated with no values, a rest-parameter callback invoked with none. A compiled callback that declares parameters is a user error rather than a silent1 = 1.FILTER_PARAMS, noting it needs the default SQL planner.A segment absent from the query still renders
1 = 1, and so does a different segment. The predicate now applies both inside the cube'ssqland in the outerWHERE— an idempotentAND, the same duplication dimension bindings already produce.Testing
rust/.../src/tests/filter_params_segment.rs— 5 cases: pushdown, no segment selected, a different segment selected, selection through a view, and a segment referencing a dimension not activating that dimension's binding. Mock DSL gained{FILTER_PARAMS_COLUMN:<cube>.<member>:<column>}for string columns.packages/cubejs-schema-compiler/test/integration/postgres/filter-params-segment.test.ts— 7 cases includingFILTER_GROUPand two row-count checks executed against Postgres, which cover the duplicated predicate.cubesqlplannertests,cargo fmt, andcube-views/sql-generation/sql-generation-logic/yaml-compiler/pre-aggregations/multi-stage-time-shift-filter-paramsunderCUBEJS_TESSERACT_SQL_PLANNER=true.Not in scope
An unresolvable
FILTER_PARAMSargument still renders1 = 1silently — tracked as an open question on the ticket, along with making "at least one segment" mandatory on a cube.🤖 Generated with Claude Code