test: cover the arrangement bucketing site and name the reduce one - #38807
Open
antiguru wants to merge 1 commit into
Open
test: cover the arrangement bucketing site and name the reduce one#38807antiguru wants to merge 1 commit into
antiguru wants to merge 1 commit into
Conversation
An error occurred while trying to automatically change base from
columnar-ti-persist-source-split
to
columnar-tg-temporal-bucket-native
September 12, 2026 00:16
An error occurred while trying to automatically change base from
columnar-ti-persist-source-split
to
columnar-tg-temporal-bucket-native
September 12, 2026 00:17
An error occurred while trying to automatically change base from
columnar-ti-persist-source-split
to
columnar-tg-temporal-bucket-native
September 12, 2026 00:17
An error occurred while trying to automatically change base from
columnar-ti-persist-source-split
to
columnar-tg-temporal-bucket-native
September 12, 2026 09:52
The runtime fixtures claimed to cover the arrangement re-encode in `ensure_collections`, but a temporal filter above a GROUP BY buckets the keyed `(key, val)` stream inside `render_reduce`, which bypasses `ensure_collections` entirely. So no fixture reached either arrangement site, and the one site whose result propagates was untested: `arrange_collection` hands back a passthrough that becomes the bundle's collection for every later consumer. Add an index on a temporal-filtered view, which routes through the loop that builds the requested arrangements, and pin its plan so the fixture cannot silently stop reaching the site if lowering stops marking the `ArrangeBy`. Point the reduce fixture's comment at `render_reduce`, and rewrite the block header to say which site each fixture covers. Two neighboring comments went stale when bucketing became columnar-native and the edge collapsed to a single representation: there is no re-encode at the TopK site any more, and both legs of the mixed `Union` are columnar whatever their strategy, so the interesting part is that one leg skipped the bucketer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
antiguru
force-pushed
the
columnar-tj-bucketing-coverage
branch
from
September 12, 2026 11:39
8b0cbc5 to
293c46e
Compare
antiguru
changed the base branch from
columnar-ti-persist-source-split
to
main
September 12, 2026 11:39
bobbyiliev
approved these changes
Sep 14, 2026
bobbyiliev
left a comment
bobbyiliev
left a comment
Contributor
There was a problem hiding this comment.
LGTM otherwise, the relabels match what I see in reduce.rs, top_k.rs and context.rs. A few small questions inline, none blocking.
| 1 | ||
| 3 | ||
|
|
||
| # Bucketed arrangement: an index on a temporal-filtered view. `ensure_collections` |
Contributor
There was a problem hiding this comment.
If I'm reading this right, the index is the terminal consumer here and the SELECT reads the arrangement, so the passthrough itself might not have a runtime consumer in this shape? Not blocking, just wondering if that part is covered.
| CREATE DEFAULT INDEX ON rt_indexed | ||
|
|
||
| query I rowsort | ||
| SELECT k FROM rt_indexed |
Contributor
There was a problem hiding this comment.
I think all four rows sit inside the window, so this would catch dropped records but maybe not a leaked expired row? Might not be worth the churn to the earlier pinned results though.
| # output, which reaches `concat_many` as a columnar edge like the other leg. | ||
| # Runtime tests. These turn `enable_compute_temporal_bucketing` on and assert | ||
| # that the bucketed dataflows still produce the correct logical results. There | ||
| # is one fixture per site that applies bucketing, because each site hands the |
Contributor
There was a problem hiding this comment.
Tiny nit: maybe say "arrangement loop" here, so this doesn't read as covering both ensure_collections sites?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to a review comment on #37787, which pointed out that the runtime fixtures did not cover what their comments claimed.
rt_reducewas labelled as hitting the arrangement re-encode incontext.rs, but a temporal filter above a GROUP BY buckets the keyed(key, val)stream insiderender_reduce, which builds its arrangement from aKeyValPlanand bypassesensure_collections. The file's own header already said so three hundred lines above the label. With that fixture accounted for elsewhere, neither arrangement site had a runtime test, and the one whose result propagates was the gap:arrange_collectionreturns a passthrough that becomes the bundle's collection for every later consumer, so a dropped record there left the whole runtime block green.This adds an index on a temporal-filtered view, which reaches the loop that builds the requested arrangements, and asserts both its physical plan and its query results. The plan is pinned because the site is only reached while lowering marks the
ArrangeBywithstrategy=TemporalBucketing; without that assertion the fixture could stop covering the site without failing. The remaining site needs the raw collection to be formed from an arrangement in the sameensure_collectionscall that builds one, and I could not construct a SQL shape that reaches it with bucketing selected. Amz_now()predicate is applied at the top of an MFP chain, so it lifts above a join rather than landing in anArrangeBy's input MFP, and an index on a temporal-filtered view splits into two dataflow objects whose first half builds no arrangement. That site may be unreachable for bucketing, but this PR does not claim so.Two neighboring comments also went stale earlier in the stack. There is no re-encode at the TopK site since bucketing became columnar-native, and both legs of the mixed
Unionare columnar whatever their strategy since the edge collapsed to a single representation, so the property worth stating is that one leg skipped the bucketer.Only
test/sqllogictest/temporal_bucketing.sltchanges.🤖 Generated with Claude Code