adriangb opened a new issue, #24173: URL: https://github.com/apache/datafusion/issues/24173
### Describe the bug `GlobalLimitExec::required_ordering` and `LocalLimitExec::required_ordering` are not serialized. `GlobalLimitExecNode` carries only `input`, `skip` and `fetch`; `LocalLimitExecNode` only `input` and `fetch`. After a protobuf round-trip the field decodes as `None`. This is not cosmetic state. It is the only surviving record that a limit is **order-sensitive**. The analysis below is @andygrove's, from review of #24167; I've verified each link against `main` (`92f4e8f3ee`) and the citations are current. **How the field gets set.** `enforce_sorting` sets it precisely when it *deletes* a redundant `SortExec` that had a fetch — the ordering requirement has nowhere else to live once the sort node is gone ([`enforce_sorting/mod.rs:518,522`](https://github.com/apache/datafusion/blob/main/datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/mod.rs#L518)): ```rust if let Some(fetch) = sort_exec.fetch() { let required_ordering = sort_exec.properties().output_ordering().cloned(); // ... GlobalLimitExec or LocalLimitExec ... global_limit.set_required_ordering(required_ordering); ``` **How it is consumed.** `limit_pushdown` turns it straight into `preserve_order` ([`limit_pushdown.rs:420,428`](https://github.com/apache/datafusion/blob/main/datafusion/physical-optimizer/src/limit_pushdown.rs#L420)): ```rust preserve_order: global_limit.required_ordering().is_some(), ``` which reaches `FileScanConfig.preserve_order`, whose own docs say files must be read in exact order "to produce correct results (e.g. for `ORDER BY ... LIMIT`)". **Why it doesn't bite in the common case.** `FileScanConfigBuilder::build()` re-derives the flag ([`file_scan_config/mod.rs:548`](https://github.com/apache/datafusion/blob/main/datafusion/datasource/src/file_scan_config/mod.rs#L548)): ```rust let preserve_order = preserve_order || !output_ordering.is_empty(); ``` and `output_ordering` *is* serialized — so a plan whose limit has already been pushed into the scan self-heals on decode. **Where the hole is.** `DataSource::with_preserve_order` bypasses that builder entirely ([`file_scan_config/mod.rs:1153`](https://github.com/apache/datafusion/blob/main/datafusion/datasource/src/file_scan_config/mod.rs#L1153)): ```rust let new_config = FileScanConfig { preserve_order, ..self.clone() }; ``` so a `false` written through this path actively clears the flag rather than being re-derived. Decode a plan that still carries the limit node, re-run the optimizer, and `required_ordering` is now `None` → `preserve_order: false` → the scan is free to read files out of order. `ORDER BY a LIMIT 10` can then return the wrong ten rows. ### To Reproduce No reproducer yet — this is a code-path argument, not a demonstrated failure. It needs a consumer that (a) serializes a plan while the limit node is still present, and (b) re-runs the physical optimizer after decoding. That is a normal shape for engines that re-plan per stage, so it seems worth closing rather than waiting for someone to hit it. ### Expected behavior `required_ordering` should survive a round-trip: add it to `GlobalLimitExecNode` / `LocalLimitExecNode` (it is a `LexOrdering`, and sort expressions already have a proto representation), or otherwise guarantee order-sensitivity cannot be silently lost. ### Additional context **Lower-severity sibling, same class:** `SortPreservingMergeExec::enable_round_robin_repartition` ([`sort_preserving_merge.rs:101`](https://github.com/apache/datafusion/blob/main/datafusion/physical-plan/src/sorts/sort_preserving_merge.rs#L101)) is also not on the wire, and decode restores the `true` default. Per its own docs `false` selects a *stable* merge, so the lossy direction can change which of several tied rows is emitted — not purely a performance knob. Nothing in-tree sets it to `false` today (only tests and benches), so priority is low, but "results differ only among tied keys, and only after a round-trip" is an unpleasant thing to debug if a downstream consumer starts using it. Both fields were surfaced by the destructuring refactor in #24167, which documents them in comments rather than changing behaviour — fixing either means a wire-format addition, which didn't belong in a no-op refactor. Related: #23494 (serde migration EPIC), #24165 (`HashJoinExec::fetch`, the same class of silently-dropped field), #24171 (test coverage gaps that let this class survive), #24170 (unchecked `as usize` in the same decode paths). -- 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]
