fix(tesseract): render FILTER_PARAMS on the measure side of a join back

[waralexrom]

Fixes #11740.

Problem

When a measure that needs the full-key plan is requested together with a dimension of a one_to_many joined cube, the planner builds a keys subquery joined back to a second copy of the fact source by primary key.

The keys-side copy is planned with the query's filters, so the cube's FILTER_PARAMS bindings render as real predicates. The measure side was planned with no filter context at all, so every binding collapsed to always-true:

  FROM (SELECT * FROM orders
        WHERE (tenant_id = $1) AND (created_at >= $2 AND created_at <= $3)
       ) AS "orders_key_orders"              -- keys side: restricted
  LEFT JOIN (SELECT * FROM orders
             WHERE 1 = 1 AND 1 = 1           -- measure side: both bindings gone
            ) AS "orders_key_orders"
    ON "keys"."orders__id" = "orders_key_orders".id

Results stayed correct — the keys side restricts the output — but the database built the join against the entire unfiltered fact table: all tenants, all time. On a large fact table the hash build outgrows the memory limit and the query fails.

The legacy planner is unaffected: its FILTER_PARAMS proxy reads the query-level allFilters regardless of which sub-select is being rendered, so both copies come out restricted. CUBEJS_TESSERACT_SQL_PLANNER=false is therefore a workaround, but the legacy planner is scheduled for removal.

Cause

AggregateMultipliedSubquery's outer select and MeasureSubquery's select carry no WHERE clause of their own — the keys subquery already restricts the rows. In Tesseract the filter set that FILTER_PARAMS / FILTER_GROUP bindings resolve against is taken from the select's WHERE filter, so for these two selects it was empty and every binding fell back to always_true.

What changed

• SelectBuilder now takes the filters its sources' FILTER_PARAMS / FILTER_GROUP bindings resolve against separately from its WHERE clause (set_filter_params_filters), defaulting to the WHERE filter as before.
• Both sources of the join back set it from the keys subquery's own filter, read through one accessor (KeysSubQuery::where_filter) that the keys select's WHERE also goes through, so the two copies of the fact source cannot drift apart:
  • AggregateMultipliedSubquerySource::Cube — the bare cube;
  • AggregateMultipliedSubquerySource::MeasureSubquery — reached by a measure whose filters: reach another cube; it had the same gap.
• The same filter set also reaches the VisitorContext of the join-back ON clause, so a binding inside the primary key's own sql renders identically on both sides of the comparison.
• The filters are an argument on the one edge that carries them — the aggregate hands them to its measure subquery directly. Neither the logical plan nor the build context holds a copy, and MeasureSubquery is no longer a ProcessableNode, so the generic path that cannot carry them does not compile.
• Removed SelectBuilder::new_from_select, which had no callers and would have carried the two filter sets inconsistently.

No WHERE clause is added anywhere, and no other plan shape changes.

On the reporter's proposal

@icoolguy1995 suggested pushing the subset of the query's filters whose members belong to the key cube into the bare cube source. That is the right diagnosis, and this change is the narrower form of it: rather than synthesising a predicate over the key cube's members, it lets the cube's existing FILTER_PARAMS bindings resolve, which is what the legacy planner does and what the issue's SQL actually shows missing. It needs no member-ownership filtering (a binding renders the column stated at the binding site, never the member's own SQL, so a filter on another cube simply matches no binding) and cannot produce a reference to a cube that is not joined on the measure side.

What this does to results

The measure side is brought into agreement with the keys side — and with what a plain, non-multiplied query over the same model already renders. That is the intended reading of a FILTER_PARAMS binding, and it is what the legacy planner produces. It is not, in general, a no-op:

For a binding in the cube sql's top-level WHERE — the shape the issue reports — the result cannot move. Both copies read the same fact rows over the same columns, and the join back is by primary key. The keys side applies pushdown ∩ WHERE, the measure side applies pushdown only, so the measure side stays a superset of the key set the join looks up: no matched row can disappear and no LEFT JOIN can turn into a NULL. Only the hash build shrinks. The Postgres tests measure exactly this.

Two shapes outside that argument can move a measure, both toward the value the keys side and the legacy planner already produce:

• A binding that restricts values rather than rows — inside a LEFT JOIN ... ON in the cube's sql, or inside a projected CASE. Row count is unchanged, so the superset argument says nothing; a sum over the affected column answers differently.
• A binding in a cube reached only by the measure join tree. That tree is built from the measure's own join hints and is not the keys-side tree, so it can contain cubes with no keys-side counterpart. A binding in such a cube's sql now resolves and can drop rows from that join, with nothing on the keys side to offset it.

Cumulative and rolling measures are planned outside this branch, so no window-widening semantics are involved.

How it was verified

• New unit tests assert both fact copies render the same predicates, for both sources of the join back. A plain count, rewritten to COUNT(DISTINCT id) over a single filtered copy, anchors them.
• New Postgres integration tests state the equivalence in numbers: the same model with and without the bindings answers the same, for both branches.
• Both new SQL tests were confirmed to fail before the fix (1 = 1 AND 1 = 1) and pass after.
• A test pins that the join-back ON clause renders the key as one expression on both sides, for a primary key whose own sql carries a binding.
• Postgres tests pin both shapes listed above that do move a measure — a value gated by a joined table's column, and a cube reached only by the measure join tree. Both fail without the fix, returning the unfiltered measure side.
• Full planner suite, unit plus Postgres integration, all passing. cargo fmt, cargo clippy --all-targets clean.

Risks

The changed filter set feeds nothing but FILTER_PARAMS / FILTER_GROUP resolution (VisitorContext::all_filters, read only by SqlCall::eval_filter_group), so no plan shape and no WHERE clause moves. What can move is a measure value, in the two shapes listed above plus a binding used negated (WHERE NOT (...)), whose measure side goes from empty to filtered. In every case the new value is the one the keys side and the legacy planner already produce, so a model that reads correctly under CUBEJS_TESSERACT_SQL_PLANNER=false is unaffected; a model that had come to rely on the measure side being unfiltered under Tesseract will see its numbers change.

🤖 Generated with Claude Code

[claude]

Claude finished @waralexrom's task in 3m 49s —— View job

---

Review: sound, narrowly scoped fix — no blocking issues (1 medium, 3 low)

Full review

• Read the diff
• Review SelectBuilder filter-params change
• Review join-back planner wiring
• Review tests
• Post review

Verdict

The diagnosis and the fix both hold up. Separating "the filters bindings resolve against" from "the WHERE clause" is the right seam: VisitorContext's all_filters is documented as existing solely to feed FILTER_PARAMS/FILTER_GROUP (physical_plan/visitor_context.rs:18), so widening what these two selects pass there cannot add or remove rows on its own.

I checked the two claims the "cannot change results" argument rests on, since both are load-bearing:

1. A binding renders the binding-site column, not the member's SQL — confirmed. SqlCall::eval_filter_group builds VisitorContext::new_for_filter_params with a symbol→binding-column map (planner/sql_call.rs:534-547), so a filter on a cube that is not joined on the measure side simply matches no binding.
2. An OR group cannot leak a foreign cube's SQL into the bare-cube copy — confirmed. FilterItem::find_subtree_for_members_inner returns None for an OR group unless every child matches the target members (planner/filter/tree.rs:175-186), so a mixed OR group falls back to always_true on both copies rather than rendering half of it.

Also verified: MeasureSubquery's new filter field is not carried by inputs()/with_inputs incorrectly (it is cloned, and no transform or optimizer rewrites a node's LogicalFilter in place, so it cannot drift from keys_subquery.filter()); the added pretty_print line breaks no snapshot (no committed snapshot renders the logical plan); and SelectBuilder::new_from_select really has no remaining callers (the surviving new_from_select hit is Cte::new_from_select, unrelated).

The query_filter() extraction is a genuine dedup — the two inline LogicalFilter literals were identical, and having one source makes the "both copies see the same filters" invariant structural rather than coincidental.

I could not run cargo test in this environment (the sandbox declined the compound cd && cargo test invocation), so the test results below are read, not executed. The seed data and both new snapshots do check out by hand: tag a → orders 1,2 → count 2 / buyers 1 = 2 and 100+200 = 300 vip; tag b → orders 1,3 → 2 / 2 = 1 and 100 vip (order 3's buyer is not VIP). The June order and the t2 order sit outside the range on purpose, which is what makes the pushdown/full-scan equivalence meaningful rather than vacuous.

Findings

#	Sev	Where	Issue
1	Medium	physical_plan/builder/select.rs:354	filter_params_filters.or(self.filter) replaces the WHERE filter for binding resolution. Fine for both current callers (neither has a WHERE), but a future caller setting both would silently lose its own predicates from binding resolution — the same failure mode this PR fixes. Suggested a debug_assert! to pin the invariant. (inline)
2	Low	processors/aggregate_multiplied_subquery.rs:112-116	Not part of the diff, so noted here: the Cube branch's join_visitor_context is still built with all_filters = None. A primary-key dimension whose sql uses a FILTER_PARAMS binding would therefore render 1 = 1 on the measure side of the ON clause while the keys side renders the real predicate — the two sides of the join key stop being the same expression. Exotic (a binding inside a PK dimension), but it is one line from the fix and the same class of asymmetry. Worth either passing the same filters or a one-line note that PK expressions are assumed binding-free.
3	Low	processors/measure_subquery.rs:51-56	The same five-line rationale is written three times (field declaration + both processors). One statement at the field, one-liners at the call sites. (inline)
4	Low	tests/filter_params_multiplied_join_back.rs:172-177	params[6..9] vs params[0..3] pins the whole statement's parameter layout; an unrelated reordering panics or silently compares the wrong pair. (inline)

Test coverage

Good shape overall: two positive cases (one per AggregateMultipliedSubquerySource variant), a negative anchor (a_count_measure_needs_no_second_copy) that keeps the positive tests honest about actually reaching the join back, and end-to-end numeric equivalence on Postgres. Confirming both SQL tests fail before the fix is the right thing to have done.

Two gaps, neither blocking:

• FILTER_GROUP is untested. It is named in every comment and in the API doc, and it takes a different code path than a plain FILTER_PARAMS binding (eval_filter_group → find_subtree_for_members, where the OR-group rule above lives). A single FILTER_GROUP variant of the bare-cube test would cover the branch that the correctness argument actually leans on.
• No case with an OR filter spanning two cubes, which is the one shape where the subtree search decides between rendering and always_true. The reasoning is right; a test would keep it right.

Minor: assert_pushdown_is_result_neutral returning Option means the Postgres tests pass silently when no database is available. That matches the existing convention in this suite, so no change requested — just noting that the numeric half of the verification is conditional in CI.

• branch `tesseract-join-back-filter-params-pushdown`

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.52%. Comparing base (2d9c13a) to head (0ba7cb0).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11814       +/-   ##
===========================================
+ Coverage   61.23%   80.52%   +19.28%     
===========================================
  Files         247      508      +261     
  Lines       19824   107359    +87535     
  Branches     4043     4043               
===========================================
+ Hits        12140    86454    +74314     
- Misses       7112    20333    +13221     
  Partials      572      572               

Flag	Coverage Δ
cube-backend	61.23% <ø> (ø)
cubesql	84.89% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @waralexrom's task in 5m 43s —— View job

---

No new findings — the fix is sound; 3 prior threads re-affirmed, 0 new inline comments

Review details

• Gather context (PR diff, issue Tesseract planner: measure-side fact copy of a multiplied-measure join-back renders FILTER_PARAMS as 1 = 1 (unbounded scan) #11740)
• Read changed files in full
• Review correctness of the filter-params plumbing
• Review tests
• Handle prior review threads
• Post findings

Verification of the mechanism

VisitorContext's third argument feeds nothing but FILTER_PARAMS / FILTER_GROUP (physical_plan/visitor_context.rs:18, reaching only SqlEvaluatorVisitor::all_filters → SqlCall::eval_filter_group), so routing the keys-subquery filter there cannot add a predicate anywhere. select.rs:347 still carries self.filter as the WHERE clause unchanged.

The "cannot reference a cube that isn't joined" claim holds: eval_filter_group renders the subtree through VisitorContext::new_for_filter_params with filter_params_columns (sql_call.rs:536), so a bound member renders the column written at the binding site, and FilterItem::find_subtree_for_members (planner/filter/tree.rs:121) keeps only items over bound members — OR groups only when every child matches. A filter over order_tags.tag therefore cannot leak an order_tags reference into the measure-side scan, which is the failure mode the narrower fix avoids relative to the reporter's proposal.

Both sides resolve the same filter set: KeysSubQueryProcessor uses keys_subquery.filter().all_filters() for its WHERE (keys_sub_query.rs:46,114) and the two new call sites use the same LogicalFilter. The MeasureSubquery branch snapshots it at logical-plan time instead of reading it at physical-build time; that is safe today — no optimizer rewrites a keys subquery's filter, and the pre-aggregation optimizer clones both keys_subquery and source together (optimizers/pre_aggregation/optimizer.rs:329). Multi-stage time shifts stay consistent because the keys query and the source are processed under the same PushDownBuilderContext after apply_evaluation_context (aggregate_multiplied_subquery.rs:61,72,152).

SelectBuilder::new_from_select removal: confirmed no remaining callers.

No logical-plan pretty-print snapshots exist, so the new filter: line in MeasureSubquery::pretty_print causes no snapshot churn.

Tests

The unit tests pin the actual defect (both fact copies, both sources of the join back) and a_count_measure_needs_no_second_copy anchors that the plan shape assumption is what the other two rely on. The Postgres snapshots check out by hand against the seed: for vip_amount, tag a = orders 1 (100, vip) + 2 (200, vip) = 300, tag b = order 1 (100) + order 3 (u2, not vip, excluded) = 100 — and order 1 carrying two tags means multiplication is genuinely exercised. FILTER_PARAMS_COLUMN: follows the existing fixture convention (test_fixtures/cube_bridge/mock_member_sql.rs:276).

Not covered: a FILTER_GROUP binding, which the new comments and the PR body both name as affected. Worth one case, given the group path is where the member-subtree selection (and so the "no foreign cube reference" argument) actually gets exercised.

Not run here

No Cargo cache on this runner, so the Rust suite was not executed — the analysis above is from reading. The Rust, Build and Bridge tests workflows were still in progress at review time; nothing had failed. JS-side Tesseract SQL assertions in packages/cubejs-schema-compiler/test were not run either, and they are the place a measure-side 1 = 1 could be pinned in an existing expectation.

Review threads

3 of my own threads returned by the listing; 0 resolved (all three still live against the current code), 3 re-affirmed:

• select.rs — set_filter_params_filters overrides rather than unions the WHERE filter (self.filter_params_filters.or(self.filter), select.rs:354), so a caller setting both would silently drop the WHERE filters from binding resolution. No current caller does; it is a footgun on a new setter.
• measure_subquery.rs — the 5-line rationale is repeated near-verbatim at three sites (processors/measure_subquery.rs:51, processors/aggregate_multiplied_subquery.rs:190, logical_plan/measure_subquery.rs:10), over the repo's 3-line limit.
• tests/filter_params_multiplied_join_back.rs:177 — hard-coded params[6..9] vs params[0..3].

Both findings I had this round duplicate the first two threads, so nothing was posted inline.

· branch [`tesseract-join-back-filter-params-pushdown`](https://github.com/cube-js/cube/tree/tesseract-join-back-filter-params-pushdown)

[claude]

Claude finished @waralexrom's task in 6m 25s —— View job

---

Approach is sound; 4 findings posted inline — 0 high, 1 medium, 3 low

Review details

• Gather context (PR diff, linked issue Tesseract planner: measure-side fact copy of a multiplied-measure join-back renders FILTER_PARAMS as 1 = 1 (unbounded scan) #11740)
• Read changed files in full, plus sql_call.rs, keys_sub_query.rs, query.rs, dimension_subquery_planner.rs, the pre-aggregation optimizer
• Verify the fix covers the reported symptom
• Review the tests
• Handle prior review threads
• Post findings

Verdict

The diagnosis is right and the mechanism is the minimal one. VisitorContext's filter set feeds nothing but FILTER_PARAMS / FILTER_GROUP — sql_call.rs:521 is its only consumer (eval_filter_group, reached from prepare_template_params for both filter_params and filter_groups), and {fpv:N} values come from eval_with_filter_values, not from it. So threading the keys subquery's filter into the two filter-less selects adds no WHERE and touches nothing else, as claimed.

The issue's reproduction is the AggregateMultipliedSubquerySource::Cube branch (orders_per_buyer), and it is covered, including the ON-clause context that commit c1d25ba fixed. SelectBuilder::new_from_select really has no callers (new_from_select elsewhere is Cte::new_from_select), so its removal is safe. MeasureSubquery has exactly one construction site, and both with_inputs impls carry filter through, so the stored copy can't drift from keys_subquery.filter() — no transform rebuilds either (LogicalFilter { .. } is constructed only in the three planners). Nothing snapshots MeasureSubquery's pretty-print, so the new filter: line breaks no existing .snap.

Tests match the conventions of the neighbouring filter_params_* files (seed fixture + try_execute_pg returning None when Postgres is absent), and the count and cross-cube-OR cases usefully pin the boundaries of the change rather than just the happy path.

I could not run cargo test / clippy in this environment (cargo invocations are not permitted here), so the PR's own claim of 1377 passing tests is unverified from my side. Everything above is static reading.

Findings

#	Severity	Site	Concern
1	medium	processors/measure_subquery.rs:52	The "cannot change results" argument covers the two fact copies, not this branch: the measure-side join tree is built from measure join hints and can contain cubes absent from the keys tree, whose bindings now render with no keys-side counterpart. Matches the legacy planner, so a Risks-section fix, not a code fix.
2	low	physical_plan/builder/select.rs:337	debug_assert! is a no-op in release, where .or() then silently drops the WHERE filter from binding resolution. Union the sets or make the combination unrepresentable.
3	low	logical_plan/measure_subquery.rs:15	filter is the name KeysSubQuery uses for a field that is a WHERE clause; only a 5-line comment separates the meanings. Rename so the code carries it and the comment shrinks.
4	low	tests/filter_params_multiplied_join_back.rs:341	.expect("a join back\nsql: {sql}") never interpolates — the SQL is lost exactly when the test regresses.

Review threads

The listing returned 3 of my own threads (single page, no human replies). All 3 resolved as addressed by the current code — the select.rs invariant is now asserted, the triplicated rationale is down to one-liners over a single field doc, and the hard-coded params[6..9] slices are replaced by indices parsed out of the rendered placeholders. 0 re-affirmed, 0 findings skipped as duplicates.

· [branch `tesseract-join-back-filter-params-pushdown`](https://github.com/cube-js/cube/tree/tesseract-join-back-filter-params-pushdown)

[claude]

Claude finished @waralexrom's task in 5m 20s —— View job

---

Approach is sound and results-neutral for the fact copies — 1 new low finding posted, 3 prior threads re-affirmed, 0 resolved.

Full review

• Read issue Tesseract planner: measure-side fact copy of a multiplied-measure join-back renders FILTER_PARAMS as 1 = 1 (unbounded scan) #11740 and PR diff
• Review changed files in full
• Check tests
• Handle prior review threads
• Post findings

Verdict

The fix matches the symptom in #11740: all_filters only feeds FILTER_PARAMS / FILTER_GROUP resolution (physical_plan/visitor_context.rs:18), so routing the keys subquery's own filter into both sources of the join back changes nothing but binding resolution. Deriving it from keys_subquery.filter() rather than re-deriving it from query_properties is the right call — the two copies of the fact source cannot drift. Passing it into the ON-clause VisitorContext (physical_plan_builder/processors/aggregate_multiplied_subquery.rs:122) is needed, not incidental: a binding inside the primary key's own sql would otherwise make the two sides different expressions, and the_join_key_renders_the_same_expression_on_both_sides pins it.

Tests are well-targeted: both branches of AggregateMultipliedSubquerySource, a FILTER_GROUP, the cross-cube OR that legitimately stays 1 = 1, and a plain count anchoring that the single-copy plan is untouched.

New finding

• logical_plan/measure_subquery.rs:33 (low) — MeasureSubquery::filter is a snapshot of keys_subquery.filter() carried outside inputs()/with_inputs(), so a future transform replacing the keys subquery leaves it stale and the copies silently drift again. No optimizer rewrites KeysSubQuery today, so it is latent.

Re-affirmed from earlier rounds (not re-posted)

• physical_plan/builder/select.rs:337 — filter_params_filters.or(self.filter) guarded only by debug_assert!; a release build that sets both silently loses the WHERE-derived bindings.
• logical_plan/measure_subquery.rs:15 — field name filter collides with KeysSubQuery::filter, which really is a WHERE clause; comment is 5 lines.
• tests/filter_params_multiplied_join_back.rs:342 — .expect("a join back\nsql: {sql}") prints {sql} literally.
• Risk statement: the "cannot change results" argument covers the two copies of the fact cube only. In the MeasureSubquery branch, bindings inside a cube the measure side alone joins (e.g. a many_to_one dimension cube — fpmjb_users in the new fixture, were its sql to carry one) now resolve and can drop rows from that join, changing measure values. That matches the legacy planner and is what the model author asked for, but it is neither in the PR's Risks section nor in a test.

Review threads

4 unresolved threads returned, all mine; 0 resolved (all four concerns still live in the working tree), 3 re-affirmed as duplicates of the findings above.

Not run

cargo check / cargo test were not permitted in this environment, so the compile and the 1377-test run quoted in the PR body were not independently reproduced. No Postgres was available for the integration tests, which skip silently via try_execute_pg returning None.

· branch `tesseract-join-back-filter-params-pushdown`

[claude]

Claude finished @waralexrom's task in 6m 8s —— View job

---

Approach is sound — 0 blocking, 2 medium, 3 prior threads still open. The fix targets exactly the gap #11740 reports, and the keys side is confirmed to render the identical filter set.

Full review

• Read issue Tesseract planner: measure-side fact copy of a multiplied-measure join-back renders FILTER_PARAMS as 1 = 1 (unbounded scan) #11740 and the PR description
• Read the changed source files in full
• Trace FILTER_PARAMS / FILTER_GROUP binding resolution against all_filters
• Check the keys side renders the same filter set
• Review the new tests and fixtures
• Check prior review threads
• Post findings

Verdict

The diagnosis matches the issue. KeysSubQueryProcessor builds its WHERE from keys_subquery.filter().all_filters() (physical_plan_builder/processors/keys_sub_query.rs:46,114), and the two new call sites feed the measure side the same expression — processors/aggregate_multiplied_subquery.rs:90-93 reads it off keys_subquery, and processors/measure_subquery.rs:52 reads it off the field the planner fills from the same place (planner/planners/multiplied_measures_query_planner.rs:206). The two copies cannot drift.

Checks that came back clean:

• FILTER_GROUP cross-cube safety. find_subtree_for_members (planner/filter/tree.rs:121) only keeps an OR group when every item names a bound member, so a filter on a cube absent from the measure-side join renders always_true on both copies rather than emitting a dangling reference. an_or_filter_across_cubes_stays_always_true_on_both_copies pins it.
• Join-key rendering. be7a22c extends the same filter set to the ON-clause VisitorContext (aggregate_multiplied_subquery.rs:119-123), which is required — a binding inside the primary key's own sql would otherwise make the two sides different expressions. the_join_key_renders_the_same_expression_on_both_sides covers it.
• Early returns. pre_aggregation_override and context.dimensions_query both return before the new code, so neither plan shape moves.
• No stale snapshots. The PrettyPrint addition on MeasureSubquery touches no committed snapshot (MeasureSubquery appears in none under tests/snapshots/).
• new_from_select removal. No callers remain anywhere in cubesqlplanner.

Tests are good: assert_copies_agree checks predicate text and bound values, and a_count_measure_needs_no_second_copy anchors the copy-counting helper so the other assertions can't pass vacuously.

I could not run cargo test / cargo clippy here — the toolchain is not available to this job, so the PR's claim of 1377 passing tests is unverified from my side.

Findings posted

#	Location	Concern
1	tests/filter_params_multiplied_join_back.rs:7	The "can only shrink the build, never change the result" invariant holds only for bindings in the cube sql's top-level WHERE; a binding in a LEFT JOIN ... ON changes column values, so measure values can move. The new values are the right ones — the framing is what needs fixing, here and in the PR's Risks section.
2	physical_plan/builder/select.rs:340	The (Some, Some) arm of binding_filters is unreachable and untested; a double-set select would hand FILTER_GROUP a duplicated subtree.

Review threads

The listing returned 5 threads first-authored by claude. Resolved 1: PRRT_kwDOCOH19M6i97AB — the debug_assert! / "picks one filter" concern, which 5660c3c addressed by conjoining both filters. Re-affirmed 3 still-live threads (no reply posted; the unresolved state already says so):

• PRRT_kwDOCOH19M6i961d — processors/measure_subquery.rs:52, the same blanket result-neutrality claim, argued via cubes reachable only through the measure join tree.
• PRRT_kwDOCOH19M6i97Sa — logical_plan/measure_subquery.rs:15, the 5-line field comment (over the repo's 3-line guidance, and restated at three other sites). My equivalent finding this round was skipped as a duplicate of it.
• PRRT_kwDOCOH19M6i97YI — tests/filter_params_multiplied_join_back.rs:342, .expect("a join back\nsql: {sql}") is not a formatting macro, so {sql} prints literally.
• PRRT_kwDOCOH19M6kWfLd — filter is still pretty-printed and cloned in with_inputs.

· branch [`tesseract-join-back-filter-params-pushdown`](https://github.com/cube-js/cube/tree/tesseract-join-back-filter-params-pushdown)

[claude]

Claude finished @waralexrom's task in 8m 32s —— View job

---

Fix is correctly targeted at the reported symptom — 0 high, 1 medium, 3 low, 4 posted inline.

Review details

• Read issue Tesseract planner: measure-side fact copy of a multiplied-measure join-back renders FILTER_PARAMS as 1 = 1 (unbounded scan) #11740 and the PR description
• Read all changed files in full (not just the diff)
• Traced where VisitorContext::all_filters is consumed
• Checked both branches of the join back for result-neutrality
• Reviewed the new tests and the Postgres fixture
• Resolved stale review threads / checked for duplicates
• Posted findings

Verdict

The diagnosis in the issue is right and the fix matches it. visitor.all_filters() is read in exactly one place — SqlCall::eval_filter_group (planner/sql_call.rs:521), reached from both the FILTER_PARAMS and FILTER_GROUP substitution paths (sql_call.rs:354-372). Nothing else consumes it, so setting it on a select that has no WHERE of its own cannot change the plan shape, only whether a binding resolves or falls back to always_true. The two fact copies were confirmed to be driven by the same filter set: keys_sub_query.rs:46 sets the keys select's WHERE from keys_subquery.filter().all_filters(), and aggregate_multiplied_subquery.rs:90-93 now derives the binding set from the same call.

The superset argument holds for the reason stated — a binding renders the column text written at the binding site, never the member's own SQL, so a query filter on a cube that is absent from the measure side simply matches no binding and cannot emit a dangling reference. The MeasureSubquery branch brings the fact copy in line with what a plain non-multiplied query over the same join already renders, so it is a removal of an anomaly rather than a new behaviour. pre_aggregation_override returns before any of this (aggregate_multiplied_subquery.rs:67), and SelectBuilder::new_from_select did have no remaining callers.

Not verified: I could not run cargo in this environment (cargo test/cargo check denied), so the PR's own test results stand unchecked by me. I did confirm no existing snapshot under src/tests/snapshots/ contains a 1 = 1, so none of them pin the old behaviour.

Findings

#	Severity	Where	Concern
A	medium	processors/aggregate_multiplied_subquery.rs:90	The binding filter set and the keys select's WHERE are derived by two independent copies of keys_subquery.filter().all_filters(); drift re-breaks the copies silently, and in the direction that loses rows
C	low	context.rs:27	The 5-line field comment restates select.rs:241-245
D	low	context.rs:18	Debug dropped from the build context only because Filter lacks the impl
E	low	tests/filter_params_multiplied_join_back.rs:331	The join-key test never compares the two sides, and locates the ON clause with rsplit_once(" ON ") + lines().next()

Review threads

The listing returned 6 of my own threads, no human replies on any. Resolved 2 — one asked to rename a MeasureSubquery::filter field and one asked for those filters to travel in the build context instead of on the logical node; commit 99dbfea did exactly that, so both sites are gone. Re-affirmed 3 that are still live and were not re-posted: the PR description's Risks section, the test module doc's "never change the result" claim, and the literal {sql} in the expect message at filter_params_multiplied_join_back.rs:342. One new finding was suppressed as a duplicate of an open thread — the unreachable (Some, Some) arm of SelectBuilder::binding_filters, which no call site can reach today since nothing sets both a WHERE and filter_params_filters.

· branch `tesseract-join-back-filter-params-pushdown`

[claude]

Claude finished @waralexrom's task in 7m 3s —— View job

---

Review complete — approach is sound; 0 high, 1 medium (test gap), 2 low. 3 inline comments posted.

Full review

• Gather context (diff vs. base 2d9c13a, PR body, linked issue Tesseract planner: measure-side fact copy of a multiplied-measure join-back renders FILTER_PARAMS as 1 = 1 (unbounded scan) #11740)
• Read changed files in full, not just the hunks
• Verify the claim that the changed filter set feeds only FILTER_PARAMS / FILTER_GROUP
• Verify the claim that a binding never renders the member's own SQL
• Check the context field for leakage into unrelated nodes
• Review test quality and fixture arithmetic
• Resolve stale threads, check new findings against existing ones
• Post findings

Verdict

The diagnosis and the fix match the symptom in #11740. Both central claims in the PR body hold as written:

• The changed filter set reaches nothing but binding resolution. VisitorContext::all_filters (physical_plan/visitor_context.rs:18) is read only through SqlEvaluatorVisitor::all_filters (physical_plan/sql_visitor.rs:69), whose sole caller is SqlCall::eval_filter_group (planner/sql_call.rs:521). The WHERE clause comes from logical_plan.filter().all_filters() in processors/query.rs:70, a separate path. So no plan shape and no WHERE moves.
• A binding renders the column stated at the binding site. eval_filter_group builds VisitorContext::new_for_filter_params from filter_params_columns (sql_call.rs:528-541), and the subtree is narrowed by find_subtree_for_members to items naming a bound symbol. Single FILTER_PARAMS goes through the same function with a one-element slice (sql_call.rs:354-365). A filter on a cube absent from the measure-side FROM therefore cannot produce a reference to it — the concern that would have made this change unsafe.

Coverage of the join back looks complete: the Cube branch is served by the outer select's context (the cube's sql is rendered from From under Select's own VisitorContext), the MeasureSubquery branch needs its own select and gets it, and the ON clause gets the same set via join_visitor_context. The where_filter() accessor is the right shape — keys_sub_query.rs:46 and aggregate_multiplied_subquery.rs:90 now read one thing, so the keys select's WHERE and the measure side's binding set cannot drift.

Fixture arithmetic checks out against both snapshots (t1 + the July 29–Aug 27 range keeps orders 1–3; orders_per_buyer = 2 / 1, vip_amount = 300 / 100 — and order 4 being out of range and order 5 being t2 means both bindings are load-bearing, not decorative).

Findings

Severity	Site	Finding
Medium	tests/filter_params_multiplied_join_back.rs:383	No test pins the two shapes the PR says can move a measure value. Both Postgres tests use PUSHED_DOWN_SCAN, i.e. the top-level-WHERE shape the module doc argues is neutral by construction; the join-key test asserts SQL text only. The behaviour-changing part of the fix lands unpinned.
Low	physical_plan_builder/context.rs:30	filter_params_filters is correct for exactly one processor edge but rides a context every processor clones; the invariant is enforced only by a comment and the take(). Failure mode if it ever leaks is a silently wrong measure.
Low	physical_plan/builder/select.rs:241	5-line doc comment where 3 carry the meaning.

binding_filters' (Some, Some) arm still has no production caller — no select sets both set_filter and set_filter_params_filters. Not re-posted: an earlier thread on select.rs already carries it and is still open (the unit-test half of its ask landed, the unreachable arm did not).

Removing SelectBuilder::new_from_select is a clean call — confirmed no remaining callers in rust/ (the only surviving new_from_select is physical_plan/cte.rs:26, unrelated). The fmt::Debug impls in filter_debug.rs are load-bearing, not test scaffolding: PushDownBuilderContext derives Debug and now holds an Option<Filter>.

Not verified

cargo test / cargo clippy were not run — no Rust toolchain is reachable from this sandbox (cargo --version is outside the allowed tool set and rust/cube/target is absent). The findings above are static. The PR reports the full planner suite, cargo fmt and cargo clippy --all-targets clean, and that both new SQL tests were confirmed failing before the fix.

Review threads

The listing returned 8 unresolved threads, all opened by claude, none by a human (single page). 7 resolved as addressed by the current diff; 1 re-affirmed and left open (the binding_filters unreachable arm, folded into the note above rather than re-posted).

· branch [`tesseract-join-back-filter-params-pushdown`](https://github.com/cube-js/cube/tree/tesseract-join-back-filter-params-pushdown)

[claude]

Five lines where three would do. The load-bearing sentences are what the field is for and that it is conjoined rather than substituted; "Set it where a select carries no WHERE of its own but its sources still have to see the query's filters" restates the two call sites, which already say it in their own comments.

Suggested change

	/// Filters an enclosing construct applies on this select's behalf, for
	/// the `FILTER_PARAMS` and `FILTER_GROUP` bindings of its sources to
	/// resolve against. Set it where a select carries no WHERE of its own but
	/// its sources still have to see the query's filters; it is conjoined
	/// with the WHERE filter, never substituted for it.
	/// Filters an enclosing construct applies on this select's behalf, for the
	/// `FILTER_PARAMS` and `FILTER_GROUP` bindings of its sources to resolve
	/// against. Conjoined with the WHERE filter, never substituted for it.

[claude]

Claude finished @waralexrom's task in 5m 56s —— View job

---

No blocking defects. 1 low (test gap, inline), 1 note (no legacy-parity guard), 2 prior threads re-affirmed.

Review details

• Read issue Tesseract planner: measure-side fact copy of a multiplied-measure join-back renders FILTER_PARAMS as 1 = 1 (unbounded scan) #11740 and the PR diff
• Read the changed Rust files in full (not just the hunks)
• Trace all_filters to every consumer
• Check tests, seed data and snapshots
• Resolve/dedupe prior review threads
• Post findings

Verdict

The mechanism is right and narrowly scoped. VisitorContext::all_filters is read in exactly one place — SqlCall::eval_filter_group (planner/sql_call.rs:521) — so nothing but FILTER_PARAMS / FILTER_GROUP resolution moves, and find_subtree_for_members (planner/filter/tree.rs:121) keeps only subtrees whose every leaf names a bound member, which is why a filter over a cube absent from the measure-side join tree cannot leak a reference to it: the OR case degrades to always_true on both copies, and the test at tests/filter_params_multiplied_join_back.rs:287 pins that.

Both copies of the fact source now read the same accessor — the keys select's WHERE goes through KeysSubQuery::where_filter() at physical_plan_builder/processors/keys_sub_query.rs:46 and the measure side through the same call at processors/aggregate_multiplied_subquery.rs:90 — so the two cannot drift. The test model reproduces the issue's schema and query verbatim, and the seed data makes the two "moves the measure" snapshots check out by hand (tag a = 300, tag b = 100+700 = 800 for vip_amount; NULL / 0 under the two binding shapes).

Findings

Low — tests/filter_params_multiplied_join_back.rs:393, join-key equivalence pinned only for the Cube source. Posted inline: the_join_key_renders_the_same_expression_on_both_sides uses orders_per_buyer, so the MeasureSubquery branch — where the key is projected inside the subquery rather than rendered in the ON clause — has no equivalent assertion.

Note (no inline) — the legacy-parity claim is untested. The PR's risk argument rests on the moved values being the ones CUBEJS_TESSERACT_SQL_PLANNER=false already produces, and the new snapshots pin only the post-fix Tesseract numbers. Nothing guards that equivalence, and this change can silently move a customer's numbers in the two documented shapes. A single legacy-planner comparison over the same model would turn the prose into a regression test.

Re-affirmed, not re-posted

• filter_params_filters (physical_plan_builder/context.rs:27) is a one-hop channel on a context every processor clones and forwards — set at aggregate_multiplied_subquery.rs:157, take()n at measure_subquery.rs:30. It is order-dependent: the take must stay above process_node(measure_subquery.source…).
• The 5-line doc on SelectBuilder::set_filter_params_filters (physical_plan/builder/select.rs:240-244) is over the repo's 3-line guideline; so is the 17-line module doc atop the new test file, which largely restates the PR body.

Threads

4 of my own threads returned by the listing: 2 resolved (PRRT_kwDOCOH19M6kX26l — the (Some, Some) arm is now covered by the binding_filters_* unit tests at select.rs:403-425; PRRT_kwDOCOH19M6kZ5mV — the value-moving shapes are now pinned by the two new Postgres snapshot tests), 2 re-affirmed (the two listed above, code unchanged).

Not verified

cargo check / cargo test were not run — no build cache in this checkout and cargo is outside the allowed tool set here, so compilation of the new test module and the two Debug impls in planner/filter/filter_debug.rs was reviewed statically only. CI covers it.

· branch `tesseract-join-back-filter-params-pushdown`

[claude]

The join-key equivalence is pinned only for the Cube source. the_join_key_renders_the_same_expression_on_both_sides queries orders_per_buyer, which takes AggregateMultipliedSubquerySource::Cube, where the measure-side key is rendered in the ON clause through join_visitor_context (aggregate_multiplied_subquery.rs:116). The MeasureSubquery branch reaches the key by a different path: the subquery projects it (measure_subquery.rs:45-47) and the ON clause compares aliases only (aggregate_multiplied_subquery.rs:169-173), so the binding inside the PK's sql resolves through select_builder.set_filter_params_filters at measure_subquery.rs:57 instead. That is the arm where a regression is silent: an unfiltered PK rendering there makes CASE WHEN 1 = 1 THEN id END on the measure side vs CASE WHEN (tenant_id = $1) THEN id END on the keys side, the join matches only the rows the binding kept, and the measure comes back NULL rather than wrong-and-obvious.

Same schema, one more test:

let ctx = TestContext::new(schema_with_key(
    PUSHED_DOWN_SCAN,
    "CASE WHEN {FILTER_PARAMS_COLUMN:fpmjb_orders.tenant_id:tenant_id} THEN id END",
))
.unwrap();
let (sql, _) = ctx
    .build_sql_and_params(&query_for("fpmjb_orders.vip_amount"))
    .unwrap();
let renderings = key_expressions(&sql);
assert_eq!(
    renderings,
    vec!["CASE WHEN (tenant_id = $) THEN id END".to_string(); renderings.len()],
    "sql: {}",
    sql
);

Fix this →

[claude]

Claude finished @waralexrom's task in 6m 28s —— View job

---

Approach is sound — no correctness defect found. 0 high, 0 medium, 3 low (2 comment/dead-branch cleanups, 1 test-assertion gap). 3 inline comments posted.

Full review

• Read PR diff and linked issue Tesseract planner: measure-side fact copy of a multiplied-measure join-back renders FILTER_PARAMS as 1 = 1 (unbounded scan) #11740
• Read changed files in full (not just the diff)
• Trace the filter path end to end
• Review tests, fixtures and snapshots
• Handle prior review threads
• Post findings

What I verified

The fix reaches the symptom in #11740. The measure-side fact copy's FILTER_PARAMS now resolve because SingleSource::Cube::to_sql (physical_plan/from.rs:26) renders the cube's sql under the enclosing select's VisitorContext, and that context's all_filters is now binding_filters(...) rather than None. Both arms of the join back are covered: the bare cube via the outer select at aggregate_multiplied_subquery.rs:198, and the measure subquery via measure_subquery.rs:61.

Both sides really do read one filter set: keys_sub_query.rs:46 and aggregate_multiplied_subquery.rs:91 both go through KeysSubQuery::where_filter(), and the keys select sets it as its WHERE (keys_sub_query.rs:114) with no params filters, so binding_filters returns the identical Filter on both.

I checked the PR's claim that a resolved binding can never emit a reference to a cube absent from the measure-side FROM, since that would be a hard SQL error rather than a moved number:

• FilterItem::find_subtree_for_members (planner/filter/tree.rs:189-211) keeps a leaf only when its member name is a binding key, and keeps an OR group only when every child matches — the new an_or_filter_across_cubes_stays_always_true_on_both_copies test pins that.
• Every retained leaf then finds its binding column: the dimension path matches on resolve_base_symbol(raw).resolve_reference_chain().full_name() (physical_plan/filter/base_filter.rs:22-27), which is exactly what BaseFilter::member_evaluator() computes — the same key the subtree search matched on. The segment path uses the same matches_member_name in both places (base_segment.rs:57-70 vs tree.rs:202-211), so a retained segment cannot fall through to visitor.apply(member_evaluator) and render its own cube-qualified SQL.

So the fallback-to-own-SQL branch is unreachable for anything the subtree search retains, and the blast radius really is confined to SqlCall::eval_filter_group (planner/sql_call.rs:521) — the only reader of VisitorContext::all_filters. No WHERE clause and no plan shape moves.

Seed data and the two "moves the measure" snapshots are self-consistent: for USERS_BINDING_SCAN, user 1 (signup 2026-01-15) drops out of the users scan, so orders 1-2 lose is_vip and tag a goes to NULL while tag b keeps order 6's 700. For JOINED_VALUE_BINDING_SCAN the same rows survive but their amount is gated to 0, giving a → 0. Both match the snapshots.

Removing SelectBuilder::new_from_select is safe — the only remaining new_from_select in the tree is Cte::new_from_select (physical_plan/cte.rs:26), unrelated.

Findings

#	Site	Severity	Concern
1	processors/measure_subquery.rs:12	low	Nine lines of doc comment across the struct and process, mostly narrating the removed ProcessableNode impl
2	physical_plan/builder/select.rs:340	low	binding_filters' conjunction branch has no production caller; only the unit test reaches it
3	tests/filter_params_multiplied_join_back.rs:449	low	The two "moves the measure" tests snapshot the new value but never assert it equals the non-multiplied query's — which is the PR's justification for accepting the move

One note on the PR description

The filters travel in the physical build context, alongside the time shifts and render flags already carried there; the logical plan holds no copy.

The last commit (0ba7cb0, "hand the measure subquery its filters directly") undid that. filter_params_filters is now a local in AggregateMultipliedSubqueryProcessor::process and an explicit parameter of MeasureSubqueryProcessor::process — PushDownBuilderContext carries no such field. Worth correcting before merge, since the description is what a future reader will search for.

Not verified

cargo check / cargo test / cargo clippy are not permitted in this environment (the Bash tool denied every cargo invocation), so I read for compilation rather than compiling. The one thing I'd have caught by building — PartialEq on FilterItem/FilterGroup for the new select.rs unit test — I confirmed by reading planner/filter/tree.rs:7,20,38. The Postgres integration tests follow the existing try_execute_pg-returns-Option convention and pass silently when no database is reachable, so their result depends on CI having one.

Review threads

The listing returned 3 threads of mine. Resolved 1 (PRRT_kwDOCOH19M6kZ513, the build-context-channel thread — PushDownBuilderContext no longer holds the field and the filters are now an explicit parameter). Re-affirmed 2 that still apply: the 5-line doc on set_filter_params_filters (select.rs:245) is verbatim unchanged, and the join-key test still exercises only the Cube arm, leaving the MeasureSubquery arm's key rendering unpinned.

· branch `tesseract-join-back-filter-params-pushdown`

[claude]

Nine lines of doc comment across this struct and process below, most of it narrating the refactor rather than stating a reason a later edit could violate. "Going through the generic process_node would lose them silently, so there is no way in" describes a ProcessableNode impl that no longer exists — a reader of the current file has no way to reintroduce it by accident, because the only call site already passes the filters.

The load-bearing sentence is that this select carries no WHERE of its own and its sources' bindings resolve against the enclosing keys subquery's filters. One line on filter_params_filters says it; the struct doc says nothing the signature doesn't.

Suggested change

	/// Deliberately not a `ProcessableNode`: a measure subquery is only meaningful
	/// inside the aggregate that owns it, which is the only thing that knows the
	/// filters its sources must resolve their bindings against. Going through the
	/// generic `process_node` would lose them silently, so there is no way in.
	/// Not a `ProcessableNode`: only the enclosing aggregate knows the filters
	/// this subquery's sources must resolve their bindings against.

Same on process (lines 25-29) — five lines for "these are the enclosing keys subquery's filters; this select has no WHERE of its own".

Fix this →

[claude]

The conjunction branch is unreachable in production. The only two set_filter_params_filters callers — aggregate_multiplied_subquery.rs:198 and measure_subquery.rs:61 — never call set_filter on the same builder, and the only set_filter callers (keys_sub_query.rs:114, query.rs:223/:235) never set the params filters. So binding_filters always takes the (x, None) / (None, y) arm; the merge is exercised only by the unit test right below it.

That is defensible as the safe composition for a future caller, but it means the doc on set_filter_params_filters ("it is conjoined with the WHERE filter, never substituted for it") documents behaviour no plan shape currently produces, and the unit test asserting item ordering pins a contract nothing reads. Worth either a one-line note that no call site sets both today, or dropping the branch to filter.or(from_enclosing) and letting the next caller that needs the merge add it with a real test behind it.

[claude]

These two tests pin the moved value but not the claim that justifies moving it. The PR's argument for accepting the change is "the new value is the one the keys side and the legacy planner already produce" — an insta snapshot of a → NULL, b → 700 records only that Tesseract answers something, and a future regression that moves it again just re-blesses the snapshot.

The equivalence is expressible with the harness already here: the same model and filters without fpmjb_order_tags.tag is the non-multiplied query that renders one filtered copy of the fact source, and its per-measure total is what the multiplied plan must sum to. For a_binding_in_a_measure_side_cube_moves_the_measure, vip_amount over USERS_BINDING_SCAN with no tag dimension is 700 (order 6, user 3 signed up in range; orders 1-2's user 1 dropped) — the same 700 the tag rows total. assert_pushdown_is_result_neutral is already the pattern; a sibling that compares multiplied against non-multiplied would state it.

As written, if the pushdown ever started dropping a row on the measure side rather than a value, a → NULL would still be a green snapshot.

Fix this →