fix: validate Parquet ordering across row groups by rank-p · Pull Request #27163 · apache/datafusion

rank-p · 2026-11-06T12:08:19Z

Which issue does this PR close?

Closes ORDER BY can return wrong results across individually sorted Parquet row groups #27162.

Rationale for this change

Parquet sort declarations apply to individual row groups. Using the first group's declaration for the whole file can remove a required sort. For groups [1, 4] and [2, 3], ORDER BY a ASC NULLS FIRST LIMIT 2 returned [1, 4] instead of [1, 2].

What changes are included in this PR?

Keep only the common leading sort keys whose row-group boundaries are ordered. Use typed bounds that account for NULLs, comparison order, and schema mapping. Missing or incompatible metadata leaves the required sort in place.

What is the testing strategy for this PR?

Query regressions in core/tests/parquet/ordering.rs fail on the old code and pass with the fix, including ORDER BY ... LIMIT.
Query and metadata tests cover valid multi-group files, NULLs, multiple keys, missing metadata, projections, and floating-point order.
301 Parquet crate tests, 13,535 extended Rust tests, and 546 SQL test files pass. Full-workspace Clippy passes with all features enabled.

Are there any user-facing changes?

Correct ordering and row selection when reading Parquet files whose row groups are sorted but the whole file is not. No public API changes.

AI assistance: Codex.

alamb · 2026-11-07T12:14:30Z

@zhuqi-lucas could you perhaps help review this PR (or help find someone that could)?

zhuqi-lucas

Thanks @rank-p for the fix, and @alamb for the ping. I also ran a few row group layouts locally (overlapping groups, ties on the first key, NULLs in a later group, DESC and mixed ASC/DESC keys, strings, timestamps, floats with and without NaN): the inferred ordering and the results were all correct.

zhuqi-lucas · 2026-11-07T13:37:09Z

+                || group
+                    .column(column_index)
+                    .statistics()
+                    .and_then(ParquetStatistics::nan_count_opt)


Files whose float statistics have no nan_count (e.g. written by pyarrow 23, which I checked locally) used to keep a float sort key and now lose it, so the SortExec comes back for them. That seems right since NaNs are not in min/max, but it may be worth a line in the upgrade guide.

rank-p · 2026-11-10T07:20:49Z

Hi @xudong963, I see that you self-requested a review, are you able to take a look at this?

alamb

Thank you for this PR @rank-p

Can we please break this PR up into smaller pieces so we can more easily undertand the implications of the changes?

This looks to me like it will disable floating point statisics in some cases, for example, as well as potentially try to handle statistics for nested strucutres.

Also the tests are really hard to follow as they have a lot of repeated boilerplate that obscures what is desired

alamb · 2026-11-11T14:06:06Z

 }

+#[tokio::test]
+async fn test_parquet_unordered_row_groups_require_sort() -> Result<()> {


I found these tests very hard to read -- is there some way we can convert them to mostly/all sqllogictests?

Or at least insta snapshot tests?

for example, a test that shows:

Input (with relevant sorting)

Query that calls for sorting

Verifificion of the correct results and plan

alamb · 2026-11-11T14:07:23Z


-            // Find the column in the arrow schema
-            let (index, _) = arrow_schema.column_with_name(name)?;
+/// Refine boundaries still tied on earlier keys. A strict earlier boundary


this code feels much more complicated than necessary to me.

Can you add some exampels of what it is actually doing?

alamb · 2026-11-11T14:07:56Z

+        return Ok(None);
+    };
+    let resolver = LeafResolver::new(parquet_schema, &physical_schema);
+    let mut ordering = Vec::with_capacity(sorting_columns.len());


is this code to handle sorting of nested columns new? Is it needed for this PR? If so, I think we should have end to end slt/sql examples that show what is going on

alamb · 2026-11-11T14:08:54Z

-            Some(PhysicalSortExpr::new(expr, options))
-        })
-        .collect()
+fn float_bounds_exclude_zero(


CHanging the sorting statistics for floating point columns seems like a substantial new behavior as well

rank-p · 2026-11-13T11:23:35Z

Hi @alamb, thanks a lot for the review, I really appreciate it. I scoped this PR down.

The core logic in metadata.rs is now:

Get all sorting_columns from row_groups (check if they are equal)
Iterate through row_groups, check if bounds are sorted.
If bounds are equal --> decide sorting decision with the next column

If statistics are unknown the file is labeled as not ordered. This could be the case if null counts are missing.

One question from my side: For the snapshot tests, is this how you are usually doing this?
Concretely, this tests would fail e.g. if the default plan is changed and instead of TopK the plan would use something else. Not sure if this is a problem or an accepted maintenance cost.

Sqllogictest can't test this behavior as it can't write a file like this (sorting_columns specified but row_groups not in order). Which makes me think if this is even worth fixing at all.

github-actions Bot added core Core DataFusion crate datasource Changes to the datasource crate labels Nov 6, 2026

xudong963 self-requested a review November 6, 2026 14:05

alamb requested a review from zhuqi-lucas November 7, 2026 12:14

zhuqi-lucas approved these changes Nov 7, 2026

View reviewed changes

alamb requested changes Nov 11, 2026

View reviewed changes

fix: validate Parquet ordering across row groups

817963e

rank-p force-pushed the fix/parquet-row-group-ordering branch from 7ae491b to 817963e Compare November 13, 2026 11:13

fix: keep the sort when the Parquet ordering check cannot run

6195d09

Navigation Menu

Repository navigation

fix: validate Parquet ordering across row groups - #27163

fix: validate Parquet ordering across row groups#27163
rank-p wants to merge 2 commits into
apache:mainfrom
rank-p:fix/parquet-row-group-ordering

rank-p commented Nov 6, 2026

Uh oh!

alamb commented Nov 7, 2026

Uh oh!

zhuqi-lucas left a comment

Uh oh!

zhuqi-lucas Nov 7, 2026

Uh oh!

rank-p commented Nov 10, 2026

Uh oh!

alamb left a comment

Uh oh!

alamb Nov 11, 2026

Uh oh!

alamb Nov 11, 2026

Uh oh!

alamb Nov 11, 2026

Uh oh!

alamb Nov 11, 2026

Uh oh!

rank-p commented Nov 13, 2026 •

edited

Loading

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

rank-p commented Nov 6, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

alamb commented Nov 7, 2026

Uh oh!

zhuqi-lucas left a comment

Choose a reason for hiding this comment

Uh oh!

zhuqi-lucas Nov 7, 2026

Choose a reason for hiding this comment

Uh oh!

rank-p commented Nov 10, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

rank-p commented Nov 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rank-p commented Nov 13, 2026 •

edited

Loading