Skip to content

test: add range-partitioned sorted time-bin aggregation coverage - #24500

Merged
alamb merged 3 commits into
apache:mainfrom
NGA-TRAN:test/range-sorted-time-bin-agg
Aug 20, 2026
Merged

test: add range-partitioned sorted time-bin aggregation coverage#24500
alamb merged 3 commits into
apache:mainfrom
NGA-TRAN:test/range-sorted-time-bin-agg

Conversation

@NGA-TRAN

@NGA-TRAN NGA-TRAN commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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:

  • columns key, zone, host, pod, service, timestamp, value
  • range-partitioned on timestamp into two 60-minute file groups
  • sorted within each file on (key, timestamp)

and pins today's physical plan for:

SELECT key, date_bin(INTERVAL '60 seconds', timestamp) AS time_bin, sum(value)
FROM metrics_range_sorted
WHERE service = 'a'
GROUP BY key, time_bin

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 RepartitionExec between Partial and Final, even though ordering_mode=Sorted is already recognized. This PR documents that plan so a follow-up can remove the shuffle and the expected plan will shrink to a single SinglePartitioned aggregate.

What changes are included in this PR?

  • Register metrics_range_sorted for sqllogictests.
  • EXPLAIN + result coverage for the filtered and unfiltered queries.

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
  • CI sqllogictests

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>
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 19, 2026
@NGA-TRAN
NGA-TRAN marked this pull request as ready for review August 19, 2026 19:44
@NGA-TRAN

NGA-TRAN commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

@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-commenter

codecov-commenter commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.31%. Comparing base (e2e161d) to head (6006096).
⚠️ Report is 24 commits behind head on main.

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.
📢 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.

@gene-bordegaray

Copy link
Copy Markdown
Contributor

I will review in depth tomorrow, at a first glance this looks nice 👍

Comment on lines +331 to +332
/// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be addressed in #24438 👍

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 gene-bordegaray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non blocking boiler plate suggestion.

cc: @gabotechs

Comment thread datafusion/sqllogictest/src/test_context/range_partitioning.rs
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 gabotechs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! thanks @NGA-TRAN for the PR and @gene-bordegaray and @xavlee for the reviews!

@alamb

alamb commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

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

@alamb
alamb added this pull request to the merge queue Aug 20, 2026
Merged via the queue into apache:main with commit 8332cfa Aug 20, 2026
44 of 62 checks passed
NGA-TRAN added a commit to NGA-TRAN/arrow-datafusion that referenced this pull request Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants