Skip to content

fix(cubestore): keep the projection schema when rewriting a rolling window - #11810

Open
waralexrom wants to merge 2 commits into
lts/v1.6from
cubestore-rolling-window-schema-lts16-backport
Open

fix(cubestore): keep the projection schema when rewriting a rolling window#11810
waralexrom wants to merge 2 commits into
lts/v1.6from
cubestore-rolling-window-schema-lts16-backport

Conversation

@waralexrom

Copy link
Copy Markdown
Member

Backport of #11410 to the 1.6 line.

Problem

A rolling-window query panics in the router instead of returning rows:

panicked at datafusion/common/src/dfschema.rs:345:31:
index out of bounds: the len is 3 but the index is 3
  DFSchema::qualified_field <- datafusion_optimizer::optimize_projections

It reproduces on a cube whose measures are two rolling-window measures plus a
type: number measure defined as their ratio, served from a rollup. Reported in #11647.

Cause

RollingOptimizerRule built rolling_aggs_alias from whichever aliased columns happened to
appear in the projection. A ratio measure (sum(m) / sum(n)) means one of the two aggregates is
never read under its own name, so the alias list came out one short.
RollingWindowAggregate::schema_from zips aggregates with aliases, so the short list silently
truncated the node's schema while the enclosing subquery kept the schema it was built with.
optimize_projections then asked the narrower schema (3 fields) for field index 3 and panicked.

What this does

Cherry-pick of 1bf2dd848e, applied with no adaptation — rolling_optimizer.rs and rolling.rs
are byte-identical to the upstream commit, and both lines pin the same DataFusion fork
(cube-46.0.1 @ ea204976). It:

  • resolves each aggregate's alias from the aggregate it belongs to instead of from projection
    order, and declines the rewrite when an aggregate has no name to output;
  • declines rolling windows carrying more than one aggregate, and aggregates with
    DISTINCT/FILTER/ORDER BY/NULL treatment;
  • restores the replaced projection's output with a projection on top when the rolling node's
    schema is wider;
  • relaxes nullability on partition-key fields, since an empty bucket reports null keys.

Plus a regression test for the reported shape (a projection reading one aggregate as sn and the
other only inside sum(m) / sum(n)).

Note on behaviour

The fix does not make multi-aggregate rolling windows work — it declines the rewrite, so such
a query is served by a plain range join. That is deliberate and matches the upstream line: the
rolling executor advances one shared group counter across its aggregates, so every aggregate past
the first reads slots the others left empty and answers null. Declining trades a panic (and, where
the aliases happened to line up, silent nulls) for a real answer.

Two consequences of that trade are worth naming explicitly, because a review of this diff found
them and they are inherited from upstream rather than introduced here:

  • The rolling rewrite and the plain range join do not answer the same question. The bound
    extractor treats Lt/LtEq and Gt/GtEq identically and maps a bare column bound to 0, so
    for the join filter Cube emits the rewrite computes a two-point trailing window while the SQL
    literally means a single point. Measured on identical data: the rewritten path returns
    10, 10, 3, 3, 5 where the range join returns NULL, 3, NULL, 5, NULL.
  • Because the rewrite is gated on the projection naming the aggregate, adding or removing an
    alias
    on the aggregate flips which path serves the query, and therefore the numbers. Measured
    on the same shape: aliasing the aggregate num rewrites (10, 10, 3, 3, 5), aliasing it win
    so it collides with a grouped-but-unprojected partition key declines (NULL, 3, NULL, 5, NULL).

Neither is a regression from this backport -- both reproduce on the upstream line, where this code
is byte-identical. They argue for fixing the shared group counter (or refusing these shapes
outright) upstream, not for diverging the 1.6 line here.

How it was verified

  • The test fails without the fix with exactly the reported panic, and passes with it.
  • The range-join path returns correct values, not merely "no panic": expected rows were derived
    by hand (the join reduces to b.d == date_from + 1) and cross-checked on a wider shape with
    overlapping buckets and several rows per bucket.
  • The reported aggregate mix (an approximate distinct count alongside a sum, plus their ratio)
    declines cleanly and returns correct values.
  • cargo test -p cubestore: 304 passed. cubestore-sql-tests: in-process 186, multi-process 186,
    migration 132, rolling subset 16/16 in all three configurations.
  • Remaining failures in those runs are environmental and reproduce on this branch's base without
    these commits: create_table_with_url / table_partition_split_threshold download a CSV from an
    external host that currently answers 403, and the full cluster suite is unstable under parallel
    load with a different set of failures on each run.

Risk

Confined to RollingOptimizerRule. Every existing rolling-window test passes unchanged; the three
that fail without the fix are the two from the upstream commit plus the new one. Queries that were
already served by the rolling path keep their plan — only shapes the rule can no longer describe
fall back to the range join.

Release condition: publishing from a non-default line must not move the latest and v1
Docker tags for cube and cubestore. Confirm detect_branch in publish.yml resolves this branch
correctly before cutting a release from it.

waralexrom and others added 2 commits September 9, 2026 11:00
One of the two aggregates is only read through an expression (`sum(m) / sum(n)`),
so nothing in the projection names it. Aggregates are paired with the names the
projection gives them positionally, so a short alias list truncates the rolling
node's schema while the enclosing subquery keeps the wider schema it was built
with; resolving a column against the narrower one panics with an index out of
bounds.

The multi-aggregate rewrite declines for this shape, so the query runs as a
plain aggregate over the range join.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@waralexrom
waralexrom requested a review from a team as a code owner September 9, 2026 09:54
@github-actions github-actions Bot added cube store Issues relating to Cube Store rust Pull requests that update Rust code labels Sep 9, 2026
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.89%. Comparing base (7f7a59e) to head (f2b1948).
⚠️ Report is 1 commits behind head on lts/v1.6.

Additional details and impacted files
@@            Coverage Diff            @@
##           lts/v1.6   #11810   +/-   ##
=========================================
  Coverage     58.89%   58.89%           
=========================================
  Files           225      225           
  Lines         17950    17950           
  Branches       3664     3664           
=========================================
  Hits          10571    10571           
  Misses         6854     6854           
  Partials        525      525           
Flag Coverage Δ
cube-backend 58.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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cube store Issues relating to Cube Store rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant