test: add range-partitioned sorted time-bin aggregation coverage - #24500
Conversation
Pin today's Partial + hash RepartitionExec + Final plan for GROUP BY key, date_bin(timestamp) on a table that is already Range([timestamp]) and sorted on (key, timestamp), so a follow-up can remove the shuffle. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@jayshrivastava : Can you help review this? These are tests for your ticket #23569. @xavlee : can you review this, too? It will be useful for #24438 CC: @gene-bordegaray and @gabotechs |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24500 +/- ##
==========================================
+ Coverage 81.27% 81.31% +0.03%
==========================================
Files 1114 1117 +3
Lines 393619 396012 +2393
Branches 393619 396012 +2393
==========================================
+ Hits 319931 322008 +2077
- Misses 54950 55181 +231
- Partials 18738 18823 +85 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I will review in depth tomorrow, at a first glance this looks nice 👍 |
| /// grouping by `(key, time_bin)` is partition-disjoint. Today's planner still | ||
| /// inserts a hash shuffle; the test pins that plan so a follow-up can remove it. |
Use generic column and table names so the coverage does not expose a metrics-specific schema. Co-authored-by: Cursor <cursoragent@cursor.com>
gene-bordegaray
left a comment
There was a problem hiding this comment.
non blocking boiler plate suggestion.
cc: @gabotechs
Accept RecordBatches and optional file sort order so the time-bin table reuses the same registration path. Co-authored-by: Cursor <cursoragent@cursor.com>
gabotechs
left a comment
There was a problem hiding this comment.
Nice! thanks @NGA-TRAN for the PR and @gene-bordegaray and @xavlee for the reviews!
|
since this is a test let's just merge it in -- I don't think it is needed to wait for 24 hours. We can update the tests as a follow on if anyone has any other thoughts |
…che#24500 Co-authored-by: Cursor <cursoragent@cursor.com>
Which issue does this PR close?
GROUP BY key, date_bin(timestamp)onRange([timestamp])).This PR does not close those issues; it only pins today's plan. A follow-up will update the expected plan once the shuffle is removed.
Rationale for this change
Adds a table that matches a common metrics layout:
key, zone, host, pod, service, timestamp, valuetimestampinto two 60-minute file groups(key, timestamp)and pins today's physical plan for:
The scan already advertises
Range([timestamp]),output_ordering=[key, timestamp], and two parallel file groups. Time bins do not straddle the hour split, so(key, time_bin)is partition-disjoint.Today DataFusion still inserts a hash
RepartitionExecbetween Partial and Final, even thoughordering_mode=Sortedis already recognized. This PR documents that plan so a follow-up can remove the shuffle and the expected plan will shrink to a singleSinglePartitionedaggregate.What changes are included in this PR?
metrics_range_sortedfor sqllogictests.Are these changes tested?
This PR is tests only.
Are there any user-facing changes?
No.
Test plan
cargo test --test sqllogictests -- range_sorted_time_bin_agg.slt