bvolpato commented on code in PR #24231:
URL: https://github.com/apache/datafusion/pull/24231#discussion_r3772202119
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -111,7 +110,12 @@ impl PhysicalOptimizerRule for PushdownSort {
// Use LocalLimitExec (not Global) since input is
multi-partition.
let inner = if let Some(fetch) = sort_child.fetch() {
inner.with_fetch(Some(fetch)).unwrap_or_else(|| {
- Arc::new(LocalLimitExec::new(inner, fetch))
+ let mut limit =
Review Comment:
Could this remain a `LocalLimitExec` while attaching `required_ordering`? In
this SPM path, `inner` can still have multiple partitions, while
`GlobalLimitExec` requires a single input partition and this rule runs after
distribution enforcement. Would that make the resulting plan fail sanity
checking or execution for an exact-pushdown source that does not support
`with_fetch()`?
##########
datafusion/physical-optimizer/src/limit_pushdown.rs:
##########
@@ -96,7 +97,7 @@ pub struct GlobalRequirements {
fetch: Option<usize>,
skip: usize,
satisfied: bool,
- preserve_order: bool,
+ required_ordering: Option<LexOrdering>,
Review Comment:
`GlobalRequirements` is public, and replacing the `bool` with
`Option<LexOrdering>` removes its `UnwindSafe` and `RefUnwindSafe` auto-trait
implementations according to cargo-semver-checks. Is this compatibility break a
concern here, or should ordering state be carried without changing those public
auto traits?
--
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]
