andygrove commented on PR #24167: URL: https://github.com/apache/datafusion/pull/24167#issuecomment-5220072050
(LLM-assisted review — I used one to trace the `required_ordering` → `preserve_order` chain, so worth a sanity check on the specifics.) The two fields you documented as not-serialized aren't equal in severity, and I think `required_ordering` deserves its own issue rather than just a comment. `GlobalLimitExec`/`LocalLimitExec::required_ordering` is load-bearing. `enforce_sorting` (`enforce_sorting/mod.rs:518,522`) sets it precisely when it *deletes* a redundant `SortExec` with a fetch — it's the only surviving record that the limit is order-sensitive. `limit_pushdown.rs:420,428` reads it into `preserve_order`, which reaches `FileScanConfig.preserve_order`, whose doc says files must be read in exact order "to produce correct results (e.g. for `ORDER BY ... LIMIT`)". It doesn't bite today because `FileScanConfigBuilder::build()` re-derives `preserve_order = preserve_order || !output_ordering.is_empty()` (`file_scan_config/mod.rs:548`) and `output_ordering` *is* serialized, so a plan with the limit already pushed into the scan self-heals. But `FileScanConfig::with_preserve_order` (`:1153`) is a struct literal that bypasses the builder, so a `false` actively clears it. Decode a plan that still has the limit node, re-run the optimizer, and `ORDER BY a LIMIT 10` can return the wrong ten rows. That's a plausible path for anything that re-plans per stage. `enable_round_robin_repartition` is milder but also not just perf — per its own docs, `false` means a *stable* merge, and the lossy direction restores the `true` default. Nothing in-tree sets it today, so low priority, but "results differ only among tied keys, only after a round-trip" is a nasty one to debug if a downstream consumer starts using it. Nothing blocking — the getter→field substitutions elsewhere look equivalent, including the `dynamic_expressions_produced` and `EmptyExec.partitions` rewrites. -- 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]
