Skip to content

Commit ff12311

Browse files
zhuqi-lucasclaude
andcommitted
docs: accurate PR description and code comments for sort pushdown
Update architecture comments and module docs to reflect actual behavior: - Core value: fix wrong file order via statistics sorting → Exact upgrade - EnforceSorting already handles correct file order cases - PushdownSort only triggers when byte-range split hasn't happened - Removed misleading claims about multi-partition optimization Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent e4dd17d commit ff12311

2 files changed

Lines changed: 42 additions & 50 deletions

File tree

datafusion/datasource/src/file_scan_config.rs

Lines changed: 37 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -898,57 +898,51 @@ impl DataSource for FileScanConfig {
898898

899899
/// Push sort requirements into file-based data sources.
900900
///
901-
/// # Sort Pushdown Architecture
901+
/// # When does this matter?
902902
///
903-
/// ```text
904-
/// Query: SELECT ... ORDER BY col ASC [LIMIT N]
903+
/// `EnforceSorting` (which runs before `PushdownSort`) already eliminates
904+
/// `SortExec` when `validated_output_ordering()` confirms the file order is
905+
/// correct. However, if files are listed in wrong order (e.g., alphabetical
906+
/// order doesn't match sort key order), `validated_output_ordering()` strips
907+
/// the ordering and `EnforceSorting` cannot help.
905908
///
906-
/// PushdownSort optimizer
907-
/// │
908-
/// ▼
909-
/// FileScanConfig::try_pushdown_sort()
910-
/// │
911-
/// ├─► FileSource::try_pushdown_sort()
912-
/// │ │
913-
/// │ ├─ natural ordering matches? ──► Exact
914-
/// │ │ (e.g. Parquet WITH ORDER) │
915-
/// │ │ ▼
916-
/// │ │ rebuild_with_source(exact=true)
917-
/// │ │ ├─ sort files by stats within groups
918-
/// │ │ ├─ verify non-overlapping
919-
/// │ │ └─► keep output_ordering → SortExec removed
920-
/// │ │
921-
/// │ ├─ reversed ordering matches? ──► Inexact
922-
/// │ │ (reverse_row_groups=true) │
923-
/// │ │ ▼
924-
/// │ │ rebuild_with_source(exact=false)
925-
/// │ │ ├─ sort files by stats
926-
/// │ │ └─► clear output_ordering → SortExec kept
927-
/// │ │
928-
/// │ └─ neither ──► Unsupported
929-
/// │
930-
/// └─► try_sort_file_groups_by_statistics()
931-
/// (best-effort: reorder files by min/max stats)
932-
/// └─► Inexact if reordered, Unsupported if already in order
933-
/// ```
909+
/// This is where `PushdownSort` adds value: it **sorts files by statistics**
910+
/// to fix the ordering, then re-checks — enabling sort elimination even when
911+
/// files were originally in wrong order.
934912
///
935-
/// # Result Plans
913+
/// # Architecture
936914
///
937915
/// ```text
938-
/// Exact (single partition): DataSourceExec [files sorted, non-overlapping]
939-
/// Exact (multi partition): SPM ─► DataSourceExec [group 0] | [group 1]
940-
/// Inexact (reverse scan): SortExec ─► DataSourceExec [reverse_row_groups]
941-
/// Inexact (stats reorder): SortExec ─► DataSourceExec [files reordered]
916+
/// PushdownSort optimizer finds SortExec
917+
/// │
918+
/// ▼
919+
/// FileScanConfig::try_pushdown_sort()
920+
/// │
921+
/// ├─► FileSource returns Exact
922+
/// │ (natural ordering already satisfies request)
923+
/// │ → rebuild_with_source: sort files by stats, verify non-overlapping
924+
/// │ → SortExec removed, fetch (LIMIT) pushed to DataSourceExec
925+
/// │
926+
/// ├─► FileSource returns Inexact
927+
/// │ (reverse_row_groups=true)
928+
/// │ → SortExec kept, scan optimized
929+
/// │
930+
/// └─► FileSource returns Unsupported
931+
/// (ordering was stripped because files in wrong order)
932+
/// → try_sort_file_groups_by_statistics():
933+
/// 1. Sort files within groups by min/max statistics
934+
/// 2. Re-check: are files now non-overlapping + ordering valid?
935+
/// YES → upgrade to Exact → SortExec removed
936+
/// NO → Inexact (files reordered but Sort stays)
942937
/// ```
943938
///
944-
/// # Trade-offs
939+
/// # Note on multi-partition plans
945940
///
946-
/// - **Exact + single partition**: No sort, no merge, but sequential I/O only.
947-
/// Best for LIMIT queries (reads minimal data and stops).
948-
/// - **Exact + multi partition**: No sort per partition, cheap O(n) SPM merge,
949-
/// parallel I/O. Best for full scans on large datasets.
950-
/// - **Inexact**: Sort still required, but statistics-aware file ordering helps
951-
/// TopK discard data earlier via dynamic filters.
941+
/// In the default configuration, `EnforceDistribution` byte-range splits
942+
/// files into single-file groups before `PushdownSort` runs. Single-file
943+
/// groups pass `validated_output_ordering()` trivially, so `EnforceSorting`
944+
/// already eliminates `SortExec`. In this case, `PushdownSort` finds no
945+
/// `SortExec` and does nothing.
952946
fn try_pushdown_sort(
953947
&self,
954948
order: &[PhysicalSortExpr],

datafusion/physical-optimizer/src/pushdown_sort.rs

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,15 +37,13 @@
3737
//!
3838
//! ## Capabilities
3939
//!
40-
//! - **Sort elimination**: when data source's natural ordering already satisfies the
41-
//! request (e.g., Parquet files with matching `WITH ORDER`), return `Exact` and
42-
//! remove the `SortExec` entirely
40+
//! - **Statistics-based file sorting + sort elimination**: when files within a
41+
//! partition are non-overlapping and internally sorted but listed in wrong order,
42+
//! sort them by min/max statistics to fix the ordering. After sorting, the
43+
//! ordering becomes valid and `SortExec` can be removed entirely. Also preserves
44+
//! `fetch` (LIMIT) from the eliminated `SortExec` for early termination.
4345
//! - **Reverse scan optimization**: when required sort is the reverse of the data source's
4446
//! natural ordering, enable reverse scanning (reading row groups in reverse order)
45-
//! - **Statistics-based file reordering**: sort files within each group by their min/max
46-
//! statistics to approximate the requested order, improving TopK and limit performance
47-
//! - **Non-overlapping detection**: when files have non-overlapping ranges and matching
48-
//! within-file ordering, the combined scan is `Exact` (sort eliminated)
4947
//! - **Prefix matching**: if data has ordering [A DESC, B ASC] and query needs
5048
//! [A DESC], the existing ordering satisfies the requirement (`Exact`).
5149
//! If the query needs [A ASC] (reverse of the prefix), a reverse scan is

0 commit comments

Comments
 (0)