jayzhan211 commented on code in PR #24231:
URL: https://github.com/apache/datafusion/pull/24231#discussion_r4046685556
##########
datafusion/physical-optimizer/src/limit_pushdown.rs:
##########
@@ -92,11 +93,11 @@ pub struct LimitPushdown {}
///
/// [`LimitPushdown`]: crate::limit_pushdown::LimitPushdown
#[derive(Default, Clone, Debug)]
-pub struct GlobalRequirements {
+pub(crate) struct GlobalRequirements {
Review Comment:
`GlobalRequirements` and `pushdown_limit_helper` went from `pub` to
`pub(crate)`. That is a larger break than the `UnwindSafe` loss it was meant to
avoid, and the PR body does not declare it. An external caller that compiles on
`main` now fails:
```
error[E0603]: struct `GlobalRequirements` is private
error[E0603]: function `pushdown_limit_helper` is private
```
Fix: keep both public. If the removal is intended, declare it under
"user-facing changes" and request the `api change` label.
```diff
-pub(crate) struct GlobalRequirements {
+pub struct GlobalRequirements {
@@
-pub(crate) fn pushdown_limit_helper(
+pub fn pushdown_limit_helper(
```
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -108,10 +107,16 @@ impl PhysicalOptimizerRule for PushdownSort {
match sort_input.try_pushdown_sort(required_ordering)? {
SortOrderPushdownResult::Exact { inner } => {
// Preserve fetch (LIMIT) from the eliminated SortExec.
- // Use LocalLimitExec (not Global) since input is
multi-partition.
+ // Use LocalLimitExec (not Global) since input is
multi-partition:
+ // this rule runs after distribution enforcement, and
+ // GlobalLimitExec requires a single input 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 = LocalLimitExec::new(inner,
fetch);
Review Comment:
The two new `set_required_ordering` calls are untested. Reverting this file
to `main` leaves every `pushdown_sort` and `limit_pushdown` test green, because
the snapshots do not display `required_ordering`.
Fix: assert on the node in
`test_sort_pushdown_exact_preserves_fetch_with_global_limit`. Add the same
check for the `SortPreservingMergeExec` → `LocalLimitExec` path.
```rs
let plan = sort_exec_with_fetch(ordering.clone(), Some(10), source);
let optimized = PushdownSort::new()
.optimize(plan, &ConfigOptions::new())
.unwrap();
let limit = optimized.downcast_ref::<GlobalLimitExec>().unwrap();
assert_eq!(limit.required_ordering().as_ref(), Some(&ordering));
```
--
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]
