Skip to content

test: cover unsorted contiguous groups in one partition - #24737

Merged
xudong963 merged 1 commit into
apache:mainfrom
xavlee:test/issue-24438-group-contiguous-single-partition
Sep 28, 2026
Merged

xudong963 merged 1 commit into
apache:mainfrom
xavlee:test/issue-24438-group-contiguous-single-partition

Conversation

@xavlee

@xavlee xavlee commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR relate to?

Rationale for this change

A source can concatenate several sorted logical runs into one DataFusion output partition. The resulting stream may be globally unsorted while every distinct (key, time_bin) tuple still occupies one contiguous range.

This PR records how aggregate planning handles that layout before grouped input properties are available. It provides the behavioral baseline for the remaining PRs in the stack.

What changes are included in this PR?

  • Add a single-partition TestMemoryExec fixture containing two sorted logical runs whose (key, time_bin) order resets at the record-batch boundary.
  • Aggregate by the complete (key, time_bin) tuple and verify the result.
  • Assert that planning selects InputOrderMode::Linear, EmissionType::Final, and SingleHashAggregateStream.

Stack

  1. #24737 — test: cover unsorted contiguous groups in one partition ← this PR
  2. #24697 — refactor: separate aggregate group completion from input ordering
  3. #24698 — feat: add grouped equivalence properties
  4. #24497 — feat: stream aggregates over grouped input

Are these changes tested?

The new aggregate test executes the single-partition input and snapshot-checks all four grouped sums.

Are there any user-facing changes?

No.

Review this layer

View only this PR layer

@codecov-commenter

codecov-commenter commented Aug 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.03448% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.44%. Comparing base (ebdb657) to head (36969e7).

Files with missing lines Patch % Lines
datafusion/physical-plan/src/aggregates/mod.rs 81.03% 3 Missing and 8 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24737      +/-   ##
==========================================
- Coverage   82.45%   82.44%   -0.01%     
==========================================
  Files        1140     1140              
  Lines      436434   436492      +58     
  Branches   436434   436492      +58     
==========================================
+ Hits       359840   359879      +39     
- Misses      54840    54848       +8     
- Partials    21754    21765      +11     

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

@xavlee
xavlee force-pushed the test/issue-24438-group-contiguous-single-partition branch from 060dd38 to 3b6d7ab Compare August 27, 2026 20:54

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

approved with some suggstions for displaying expected behavior

Comment thread datafusion/physical-plan/src/aggregates/mod.rs
Comment thread datafusion/physical-plan/src/aggregates/mod.rs Outdated
@xavlee
xavlee force-pushed the test/issue-24438-group-contiguous-single-partition branch from 3b6d7ab to 5dfc10b Compare August 31, 2026 17:50

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

this guy looking good now, thank you @xavlee 💃

@NGA-TRAN NGA-TRAN 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.

LGTM

@xavlee
xavlee force-pushed the test/issue-24438-group-contiguous-single-partition branch 3 times, most recently from 7100fae to fef01ed Compare September 7, 2026 20:23
@xavlee
xavlee force-pushed the test/issue-24438-group-contiguous-single-partition branch from fef01ed to 0e9cafe Compare September 16, 2026 14:24
@gene-bordegaray

Copy link
Copy Markdown
Contributor

rebumping this, just went over again and looks good.

@xavlee
xavlee force-pushed the test/issue-24438-group-contiguous-single-partition branch from 0e9cafe to 7ba0a9b Compare September 22, 2026 17:44
@xavlee
xavlee force-pushed the test/issue-24438-group-contiguous-single-partition branch from 7ba0a9b to 36969e7 Compare September 23, 2026 02:04
@xavlee
xavlee marked this pull request as ready for review September 23, 2026 19:30
@xudong963
xudong963 added this pull request to the merge queue Sep 28, 2026
@xudong963

Copy link
Copy Markdown
Member

Thanks

Merged via the queue into apache:main with commit 59d74ca Sep 28, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants