Skip to content

fix: include nulls in file ordering bounds - #26280

Open
rank-p wants to merge 2 commits into
apache:mainfrom
rank-p:fix/parquet-sort-order-validation
Open

rank-p wants to merge 2 commits into
apache:mainfrom
rank-p:fix/parquet-sort-order-validation

Conversation

@rank-p

@rank-p rank-p commented Oct 13, 2026 •

Copy link
Copy Markdown

Which issue does this PR close?

Closes #26278.

Rationale for this change

Ignoring NULLs in file bounds can make a combined scan appear sorted, causing ORDER BY to return rows in the wrong order.

What changes are included in this PR?

Include NULLs in file bounds using NULL counts, schema nullability, and sort options. Keep the existing sort-pushdown policy.

What is the testing strategy for this PR?

  • Add regressions covering both sort directions, NULL placement, NULL counts, and all-NULL/empty files beside non-NULL data. Each fails when its relevant fix is removed.
  • Passed 185 crate tests, 12,581 extended Rust tests, 533 SQL test files, and 110 CLI tests.
  • CI-style lint checks pass except the security audit, which flags five existing quinn-proto 0.11.15 advisories. Separate --all-features Clippy warnings remain in unchanged optimizer code.
  • Local file-grouping benchmark: approximately 1.60 → 1.67 ms for 1,000 files.

Are there any user-facing changes?

Correct NULL placement when sorting across files. No public API changes.

AI assistance: Codex.

@github-actions github-actions Bot added the datasource Changes to the datasource crate label Oct 13, 2026

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

The NULL-aware bounds and four-way regression test look correct. I verified that reverting only the production change makes the new test fail.

One non-blocking coverage suggestion: could we add an all-NULL file next to a non-NULL file? That would exercise the new all_null branch and distinguish a NULL-only point interval from an exact-empty file.

@rank-p

rank-p commented Oct 14, 2026

Copy link
Copy Markdown
Author

The NULL-aware bounds and four-way regression test look correct. I verified that reverting only the production change makes the new test fail.

One non-blocking coverage suggestion: could we add an all-NULL file next to a non-NULL file? That would exercise the new all_null branch and distinguish a NULL-only point interval from an exact-empty file.

Added

@rank-p
rank-p force-pushed the fix/parquet-sort-order-validation branch from 108bee4 to 61c57a6 Compare October 14, 2026 18:42
@rank-p

rank-p commented Oct 14, 2026 •

Copy link
Copy Markdown
Author

The contributor guide says to tag a committer for CI runs/review. @xudong963 can you help with this maybe?

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.32143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.80%. Comparing base (0d79252) to head (61c57a6).
⚠️ Report is 62 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/datasource/src/file_scan_config/mod.rs 97.43% 0 Missing and 2 partials ⚠️
datafusion/datasource/src/statistics.rs 97.05% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #26280    +/-   ##
========================================
  Coverage   82.80%   82.80%            
========================================
  Files        1149     1149            
  Lines      454146   454248   +102     
  Branches   454146   454248   +102     
========================================
+ Hits       376035   376129    +94     
- Misses      55260    55262     +2     
- Partials    22851    22857     +6     

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

@rank-p
rank-p force-pushed the fix/parquet-sort-order-validation branch from 61c57a6 to 16198c9 Compare October 19, 2026 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

datasource Changes to the datasource crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ORDER BY can return NULLs out of order across sorted Parquet files

3 participants