2010YOUY01 commented on code in PR #27000:
URL: https://github.com/apache/datafusion/pull/27000#discussion_r4438604807
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1794,6 +1794,18 @@ impl ExecutionPlan for SortExec {
BaselineMetrics::new(&self.metrics_set, partition),
))),
(true, None) => Ok(input),
+ // `TopK` requires `k > 0`. `LIMIT 0` is normally removed by the
+ // logical optimizer, but a `SortExec` with `fetch = Some(0)` can
+ // still reach execution, for example when the logical optimizer
+ // is disabled or the plan is built directly.
+ (false, Some(0)) => {
+ // Count this partition as emitted so the shared TopK dynamic
+ // filter completes once every partition has finished.
+ let filter = self.filter.as_ref();
+ let filter = unwrap_or_internal_err!(filter);
+ filter.read().mark_topk_emitted();
Review Comment:
Thank you for the fix.
I can understand why enter this branch and return empty set, but I don't get
those three lines, for example why don't the above `LimitStream` branch have to
special handle dynamic filter like this
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -4359,6 +4371,54 @@ mod tests {
Ok(())
}
+ /// Regression test for
<https://github.com/apache/datafusion/issues/26431>:
+ /// a `SortExec` with `fetch = Some(0)` over unsorted input used to panic
+ /// in `TopKHeap::new`. It must return no rows, and the shared TopK
+ /// dynamic filter must still complete once every partition has run.
+ #[tokio::test]
Review Comment:
See
https://datafusion.apache.org/contributor-guide/testing.html#choosing-what-kind-of-test-to-write
This UT is asserting on low-level implementations (probing into the dynamic
filter), make it hard to understand and maintain.
We could either:
- write it as only construct the SortExec and run
- or remove it, we already have slt I think it's good already
--
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]
