Copilot commented on code in PR #22493:
URL: https://github.com/apache/datafusion/pull/22493#discussion_r3294312103
##########
datafusion/sqllogictest/test_files/sort_pushdown.slt:
##########
@@ -1175,10 +1174,72 @@ SELECT * FROM overlap_parquet ORDER BY id ASC;
5 500
6 600
+# Test 5b: Safety case — no WITH ORDER, files written without ORDER BY (no
+# sorting_columns metadata). Source has no way to declare per-file ordering,
+# so even though min/max stats happen to be non-overlapping, the optimizer
+# must NOT eliminate SortExec.
+statement ok
+CREATE TABLE no_decl_low(id INT, value INT) AS VALUES (1, 100), (3, 300), (2,
200);
+
+statement ok
+CREATE TABLE no_decl_mid(id INT, value INT) AS VALUES (6, 600), (4, 400), (5,
500);
+
+statement ok
+CREATE TABLE no_decl_high(id INT, value INT) AS VALUES (9, 900), (8, 800), (7,
700);
+
+# Write WITHOUT ORDER BY so each file lacks sorting_columns metadata.
+query I
+COPY no_decl_low TO 'test_files/scratch/sort_pushdown/no_decl/a_low.parquet';
+----
+3
+
+query I
+COPY no_decl_mid TO 'test_files/scratch/sort_pushdown/no_decl/b_mid.parquet';
+----
+3
+
+query I
+COPY no_decl_high TO 'test_files/scratch/sort_pushdown/no_decl/c_high.parquet';
+----
+3
+
+statement ok
+CREATE EXTERNAL TABLE no_decl_parquet(id INT, value INT)
+STORED AS PARQUET
+LOCATION 'test_files/scratch/sort_pushdown/no_decl/';
+
Review Comment:
The new Test 5b creates base tables (`no_decl_low/mid/high`) and an external
table (`no_decl_parquet`) but doesn’t drop them afterwards. The rest of this
SLT file generally cleans up created tables; please add corresponding `DROP
TABLE` statements (and any needed cleanup) to avoid state leaking into later
tests and to keep the test file self-contained.
##########
datafusion/datasource/src/file_scan_config/sort_pushdown.rs:
##########
@@ -138,31 +138,51 @@ impl FileScanConfig {
false
};
- if is_exact && all_non_overlapping {
- // Truly exact: within-file ordering guaranteed and files are
non-overlapping.
- // Keep output_ordering so SortExec can be eliminated for each
partition.
- //
- // We intentionally do NOT redistribute files across groups here.
- // The planning-phase bin-packing may interleave file ranges
across groups:
- //
- // Group 0: [f1(1-10), f3(21-30)] ← interleaved with group 1
- // Group 1: [f2(11-20), f4(31-40)]
- //
- // This interleaving is actually beneficial because SPM pulls from
both
- // partitions concurrently, keeping parallel I/O active:
- //
- // SPM: pull P0 [1-10] → pull P1 [11-20] → pull P0 [21-30] →
pull P1 [31-40]
- // ^^^^^^^^^^^^ ^^^^^^^^^^^^
- // both partitions scanning files simultaneously
- //
- // If we were to redistribute files consecutively:
- // Group 0: [f1(1-10), f2(11-20)] ← all values < group 1
- // Group 1: [f3(21-30), f4(31-40)]
- //
- // SPM would read ALL of group 0 first (values always smaller),
then group 1.
- // This degrades to single-threaded sequential I/O — the other
partition
- // sits idle the entire time, losing the parallelism benefit.
+ // Decide whether to keep `output_ordering` (i.e. let the outer
+ // pushdown report `Exact` and drop `SortExec`).
+ //
+ // Two paths can produce a keep:
+ //
+ // 1. `is_exact && all_non_overlapping`: the source already had
+ // validated ordering and the post-sort files still don't
+ // overlap — Exact carries through unchanged.
+ //
+ // 2. `!is_exact && all_non_overlapping`: source returned
+ // `Inexact` because pre-sort `validated_output_ordering()`
+ // stripped the declaration (files were listed out of order
+ // on disk). After our stats-based sort the files are now
+ // non-overlapping — re-validate against the new file
+ // groups and, if it passes, upgrade back to Exact so the
+ // outer wrapper drops the `SortExec`. Without this, the
+ // `Inexact` branch stayed Inexact even when reorder
+ // restored a perfectly valid ordering, leaving an
+ // unnecessary `SortExec` above the source (regression
+ // after #21956's `column_in_file_schema` signal pushed
+ // this scenario into the Inexact branch instead of the
+ // `try_sort_file_groups_by_statistics` fallback).
+ //
+ // We intentionally do NOT redistribute files across groups here.
+ // The planning-phase bin-packing may interleave file ranges across
groups:
+ //
+ // Group 0: [f1(1-10), f3(21-30)] ← interleaved with group 1
+ // Group 1: [f2(11-20), f4(31-40)]
+ //
+ // This interleaving is actually beneficial because SPM pulls from both
+ // partitions concurrently, keeping parallel I/O active.
+ let keep_ordering = if all_non_overlapping {
+ if is_exact {
+ true
+ } else {
+ // Re-validate: now that files are sorted, can the
+ // request be satisfied?
+ let new_eq_props = new_config.eq_properties();
+ new_eq_props.ordering_satisfy(order.iter().cloned())?
Review Comment:
`rebuild_with_source` can now preserve `output_ordering` (and thus allow
`SortExec` elimination) on the `is_exact=false && all_non_overlapping` path,
but unlike `try_sort_file_groups_by_statistics` it does not guard against NULLs
in the sort columns. This can incorrectly claim exact ordering for multi-file
groups where an earlier file contains NULLs (e.g. NULLS LAST), which can break
the concatenated stream ordering. Consider applying the same NULL-safety check
here (or an equivalent per-group/per-position check) before keeping
`output_ordering`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
