adriangb commented on code in PR #26434: URL: https://github.com/apache/datafusion/pull/26434#discussion_r4300698464
########## docs/source/library-user-guide/upgrading/56.0.0.md: ########## @@ -1183,3 +1183,30 @@ A registered plan needs no `PhysicalExtensionCodec` at all, and two crates' plan `PhysicalExtensionCodec` is not deprecated: extension `PhysicalExpr`s and UDF payloads still need it, and it remains the fallback for unmigrated plans. See [issue #24625](https://github.com/apache/datafusion/issues/24625) for details. + +### Order-preservation hooks for limit pushdown are deprecated + +A file scan that advertises an output ordering now always reads its files in +order, so `ORDER BY ... LIMIT` queries no longer depend on `LimitPushdown` +telling the scan to keep its order. Before this change, pushing the fetch of a +`SortPreservingMergeExec` into an ordered Parquet scan could return the wrong +rows, because the scan then shared files between partitions or skipped row +groups based on its limit. + +The plumbing that carried this request is deprecated and no longer used: + +- `ExecutionPlan::with_preserve_order` and `DataSource::with_preserve_order` + are never called. Built-in operators no longer override them. Review Comment: Custom `DataSource` and `ExecutionPlan` implementations that override `with_preserve_order` do not get a compile error or a warning: `#[deprecated]` on a trait method warns callers only, not implementors. After this change, DataFusion never calls their override, so their behaviour changes silently. Please add to this entry: - Overrides of these methods are now dead code. - A custom scan that advertises an output ordering must keep that order by itself, also when a limit is pushed into it. ########## datafusion/datasource/src/file_scan_config/mod.rs: ########## @@ -1220,7 +1206,7 @@ impl DataSource for FileScanConfig { &self, config: &ConfigOptions, ) -> Option<Arc<dyn Any + Send + Sync>> { - if self.preserve_order + if self.preserves_file_order() Review Comment: This also turns off work stealing for a plain `LIMIT` (no `ORDER BY`) over a declared-ordering table, and the PR does not measure skewed file groups. I measured it: 16 sorted files, 38M rows (4 files of 8M rows and 12 of 0.5M), `WITH ORDER (a ASC)`, `target_partitions = 8`, release-nonlto, base = merge-base `81824e82`. Median of 5 interleaved runs, best of 3 per run (ms): | Query | pushdown_filters | base | this PR | |---|---|---|---| | `SELECT * FROM t WHERE b > 0.999999 LIMIT 10` (skewed) | off | 13 | 25 | | same (skewed) | on | 5 | 6 | | same (uniform, 16 × 2M) | off | 9 | 12 | | `SELECT * FROM t LIMIT 10` / `WHERE b > 0.99 LIMIT 10` | off / on | 3-5 | 3-5 | So a selective filter over skewed groups is about 2x slower with `pushdown_filters = false`. The planner puts `f00` and `f01` (both 8M rows) in one group, and without work stealing that partition sets the wall time. Please do one of these: - Keep work stealing when no parent needs the order. That keeps the fix for `ORDER BY ... LIMIT`. - Or state this cost in the PR description and the upgrade guide, so the trade-off is an explicit decision. ########## datafusion/physical-plan/src/limit.rs: ########## @@ -347,21 +344,16 @@ impl GlobalLimitExec { input, skip, fetch, - required_ordering, + // Deprecated, ignored. + required_ordering: _, Review Comment: Please add a test that decodes a `GlobalLimitExecNode` / `LocalLimitExecNode` with a non-empty `required_ordering` (a payload from an older writer) and checks that the decode succeeds and ignores the field. Without it, nothing guards the "ignored when read" promise in the proto comment. -- 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]
