Skip to content

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

Draft
xavlee wants to merge 1 commit into
apache:mainfrom
xavlee:test/issue-24438-group-contiguous-single-partition
Draft

test: cover unsorted contiguous groups in one partition#24737
xavlee wants to merge 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 sorted logical runs into one DataFusion output partition. The resulting rows can be globally unsorted while every distinct (key, time_bin) grouping tuple still occupies one contiguous range. This fixture records how aggregate planning handles that layout today.

What changes are included in this PR?

  • Add a single-partition TestMemoryExec fixture containing two sorted logical runs whose (key, time_bin) order resets between runs.
  • Group by the complete (key, time_bin) tuple.
  • Assert InputOrderMode::Linear, EmissionType::Final, and the ordinary single-hash stream.

Stack

  1. #24737 — test: cover unsorted contiguous groups in one partitionthis PR
  2. #24697 — refactor: separate aggregate group completion from input ordering
  3. #24698 — feat: add narrow group-contiguous source property
  4. #24497 — feat: stream exact group-contiguous aggregates

Are these changes tested?

Yes, this PR itself only adds a test that serves to capture behavior today.

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 72.30769% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.58%. Comparing base (ee59f62) to head (5dfc10b).
⚠️ Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/physical-plan/src/aggregates/mod.rs 72.30% 3 Missing and 15 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24737      +/-   ##
==========================================
+ Coverage   81.42%   81.58%   +0.15%     
==========================================
  Files        1121     1123       +2     
  Lines      402142   406668    +4526     
  Branches   402142   406668    +4526     
==========================================
+ Hits       327460   331760    +4300     
+ Misses      55484    55457      -27     
- Partials    19198    19451     +253     

☔ 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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants