test(tesseract): pin the row set of a share whose denominator drops the query filter

[waralexrom]

Problem

Reported: a multi-stage share measure whose denominator carries group_by naming
members outside the query grain, together with filter: exclude dropping the
query's own filter, returned extra rows with a NULL share — the values the query
filtered out came back alongside the real row.

Cause

The denominator is a sum-of-sum aggregate, so it is eligible for the window path.
That path has one row set serving both the aggregation input and the reported
rows, so once filter.exclude widens the input, the widened rows are reported
too.

The planner already revokes the window path when a filter directive drops a
filter the query restricts the grid by, and that revocation is what saves this
shape — the reported query returns the correct single row on current master.
What was missing is a test: this shape differs from the covered ones in that
group_by intersects the query grain rather than extending it, so the measure
side can end up keeping none of the query's dimensions and the reported rows can
only come from the keys side.

What changed

Tests only. No production code changes.

• integration_multi_stage.yaml: a denominator partitioned by category /
  created_at / created_at.day whose filter.exclude drops the query filter
  on status, plus a share measure consuming it.
• filter_directive.rs: two integration tests over that measure — one where the
  partition intersects the query grain down to nothing, one where the query's
  time dimension survives the intersection so the widened key grid is non-empty.

How it was verified

• Both tests pass on master and fail when the window-path revocation is disabled,
  reproducing the report verbatim — the filtered-out statuses come back carrying
  NULL:

      1     1 │ orders__status | orders__amount_category_day_share_percent | orders__total_amount
      2     2 │ ---------------+-------------------------------------------+---------------------
            3 │+cancelled      | NULL                                      | NULL
            4 │+pending        | NULL                                      | NULL
      3     5 │ completed      | 62.2222222222222222                       | 1400.00
  

  and, in the partially-kept case, once per month:

      3     3 │ completed      | 2024-01-01 00:00:00 | 60.0000000000000000 | 300.00
            4 │+pending        | 2024-01-01 00:00:00 | NULL                | NULL
            5 │+cancelled      | 2024-01-01 00:00:00 | NULL                | NULL
  

• The reported model and query were also run end-to-end through the JS schema
  compiler against Postgres on both planners. Tesseract returns the single
  expected row. The legacy planner returns the same single row with the
  denominator still filtered (share = 1.0), because the filter: directive is
  Tesseract-only — a documented difference, not a regression.

• Full cubesqlplanner suite green with --features integration-postgres
  (1374 passed). CI covers these snapshots: push.yml runs
  --features cubesqlplanner/integration-cubestore, which implies
  integration-postgres.

Risks

Low — the diff adds two tests and two measures to a shared test fixture. The new
measures are not exported by any view in that fixture, so no existing test
changes shape.

Known coverage gap, not addressed here: tesseract/join_types_full is defined
only by the Snowflake dialect, so the full-join assembly strategy has no test
coverage at all. Postgres rejects that join shape at every version ("FULL JOIN is
only supported with merge-joinable or hash-joinable join conditions"), so it
cannot be row-validated in the existing harness. It does not affect this shape:
the denominator CTE carries an explicit keys side, which selects the keys
strategy on every dialect.

🤖 Generated with Claude Code

[claude]

Claude finished @waralexrom's task in 2m 27s —— View job

---

Review: tests-only change, snapshot values check out against the seed — 0 high, 0 medium, 3 low (all nits, posted inline)

Full review

• Gather context (diff, changed files)
• Review YAML fixture additions
• Verify snapshot values against the seed data
• Review new integration tests
• Post review

What I verified

Both snapshots are arithmetically consistent with seeds/integration_multi_stage_tables.sql, which is the part of a snapshot test that is easy to get silently wrong:

test	numerator	denominator (unfiltered)	share
group_by_outside_query_grain	completed = 1400	grand total = 2250	62.2222…% ✓
group_by_partially_kept (Jan)	300	500	60% ✓
” (Feb)	500	750	66.6666…% ✓
” (Mar)	600	1000	60% ✓

That is the right pin: the share reflects a denominator that ignores the status filter, while the row set stays the query's own. It confirms filter.exclude still widens the aggregation input, so a future regression that fixed the row set by also re-applying the filter to the denominator would fail these tests rather than pass them.

The two cases are genuinely distinct, not a copy: with the grain intersected down to nothing the widened key grid collapses to one row, so only the keys side inheriting the wrong state is observable; keeping created_at in the intersection makes the grid non-empty, which is what makes the per-month duplicate rows in the PR body's failure output visible. Worth having both.

Structure follows the file's existing convention exactly (create_context(), build_sql(...).unwrap(), try_execute_pg guarded snapshot, shared SEED), and the new measures are not exported by any view in the fixture, so the "no existing test changes shape" claim holds.

Findings (all low, inline)

1. integration_multi_stage.yaml:874 — mode: relative is the default when omitted (measure_filter_mode_defaults_to_relative_when_omitted, tests/measure_symbol.rs:652); the only other measures that state it are in the block that contrasts relative against fixed. Redundant knob here. (My first suggestion block on that line was mis-scoped and would have duplicated exclude: — corrected in the follow-up comment on 874-875.)
2. integration_multi_stage.yaml:859-864 — six-line comment where the section's norm is three to four; the second half narrates what the two created_at entries already show. Same for the block comment at filter_directive.rs:540-545.
3. integration_multi_stage.yaml:865 — amount_by_category_day_unfiltered names the declared partition, which is never the effective one in either test. A name mentioning the intersection would carry the intent.

None of these affect correctness; the change is mergeable as-is.

What I could not do

I could not run cargo test -p cubesqlplanner — the command needs approval in this environment, so I did not independently reproduce the "passes on master, fails with the window-path revocation disabled" claim, nor the 1374-passing suite run. The verification argument in the PR body is internally consistent and the CI path named (push.yml → cubesqlplanner/integration-cubestore, which implies integration-postgres) does cover these snapshots. Note the harness property that try_execute_pg returns None without the postgres feature, so outside that CI job these two tests only assert that build_sql succeeds — expected, and the same for every test in the file.

The full-join coverage gap called out under "Risks" is real but correctly scoped out: the denominator CTE carries an explicit keys side, so the keys strategy is selected on every dialect and the untested full-join assembly is not on this path.

• branch `tesseract-filter-exclude-null-rows`

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.30%. Comparing base (1699910) to head (0b274c8).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11822       +/-   ##
===========================================
+ Coverage   60.16%   80.30%   +20.13%     
===========================================
  Files         239      500      +261     
  Lines       19206   106357    +87151     
  Branches     3886     3886               
===========================================
+ Hits        11556    85411    +73855     
- Misses       7100    20396    +13296     
  Partials      550      550               

Flag	Coverage Δ
cube-backend	60.16% <ø> (ø)
cubesql	84.74% <ø> (?)

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 1m 14s —— View job

---

Re-review after the latest push: previous nits addressed, no new findings — 0 high, 0 medium, 1 low (a prior naming nit, still open)

Full review

• Gather context (diff, commits since last review)
• List prior review threads
• Re-verify snapshot values against the seed
• Re-check fixture + test additions in current form
• Resolve stale threads, post review

What changed since the last round

a79d72b added the partially-kept case; 0b274c8 dropped both fixture/test comments and the redundant mode: relative. That closes three of the four threads from the previous round — resolved:

• mode: relative gone from the new filter: directive (fixture now matches the section's convention of omitting the default).
• The six-line fixture comment and the block comment at filter_directive.rs:540 are gone.

The current fixture addition is 17 lines with no prose, and the two tests carry no preamble — consistent with the leaner measures in the same block.

Re-verified against the seed

Both snapshots still line up with seeds/integration_multi_stage_tables.sql:

test	numerator (completed)	denominator (all statuses)	share
group_by_outside_query_grain	1400	2250 (grain intersects to nothing → grand total)	62.2222…% ✓
group_by_partially_kept (Jan)	300	500	60% ✓
” (Feb)	500	750	66.6666…% ✓
” (Mar)	600	1000	60% ✓

The pin is still the right one: the denominator ignores the status filter while the row set stays the query's own, so a future "fix" that corrected the row set by re-applying the filter to the denominator would fail these rather than pass them. orders.created_at appearing bare and as .day in group_by is what makes the second case's month time dimension survive the intersection, which is what makes the per-month duplicates in the PR body's failure output observable — the two cases remain genuinely distinct.

Also re-confirmed: neither new measure is in orders_ms_view's includes (fixture lines 1042-1058), so no existing test changes shape; snapshot filenames match insta's naming for the two test functions.

Findings

One low, from the previous round, still applicable and left unresolved (thread on integration_multi_stage.yaml:859): amount_by_category_day_unfiltered names the declared group_by, which is never the effective partition in either test — the intersection reduces it to the whole universe in the first case and to the month in the second. Non-blocking; I did not re-post it. No new issues.

What I could not do

cargo test -p cubesqlplanner needs approval in this environment, so I again could not independently reproduce the "passes on master, fails with the window-path revocation disabled" claim. Unchanged harness caveat: try_execute_pg returns None without the postgres feature, so outside the CI job that enables cubesqlplanner/integration-cubestore these two tests only assert build_sql succeeds — same as every test in this file.

Mergeable as-is.

• branch `tesseract-filter-exclude-null-rows`