[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6695: Replace supports_bounded_execution with supports_retract_batch

["mustafasrepo (via GitHub)" <gi...@apache.org>]

mustafasrepo commented on code in PR #6695:
URL: https://github.com/apache/arrow-datafusion/pull/6695#discussion_r1231892829


##########
datafusion/physical-expr/src/window/aggregate.rs:
##########
@@ -155,8 +155,7 @@ impl WindowExpr for PlainAggregateWindowExpr {
     }
 
     fn uses_bounded_memory(&self) -> bool {
-        self.aggregate.supports_bounded_execution()
-            && !self.window_frame.end_bound.is_unbounded()
+        !self.window_frame.end_bound.is_unbounded()

Review Comment:
   After thinking through this logic. Actually, as long as `end_bound` is not bounded (not `UNBOUNDED FOLLOWING` such as in the form `N FOLLOWING`). We can produce results without waiting for the whole data to come (If accumulator do not support `retract_batch` method. We wouldn't be able to run queries in the form `M PRECEDING and N FOLLOWING`, in this case we will give an error anyway.). Hence here we do not need to check for `self.aggregate.supports_bounded_execution()` (`acc.supports_retract_batch()` method with the new API.) 



##########
datafusion/physical-expr/src/window/sliding_aggregate.rs:
##########
@@ -139,8 +139,7 @@ impl WindowExpr for SlidingAggregateWindowExpr {
     }
 
     fn uses_bounded_memory(&self) -> bool {
-        self.aggregate.supports_bounded_execution()
-            && !self.window_frame.end_bound.is_unbounded()
+        !self.window_frame.end_bound.is_unbounded()

Review Comment:
   Similar to the case above.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org