alamb commented on code in PR #5587:
URL: https://github.com/apache/arrow-datafusion/pull/5587#discussion_r1139185484
##########
datafusion/core/src/physical_plan/windows/bounded_window_agg_exec.rs:
##########
@@ -548,9 +548,9 @@ impl SortedPartitionByBoundedWindowStream {
for (partition_row, WindowState { state: value, .. }) in
window_agg_state {
let n_prune =
min(value.window_frame_range.start,
value.last_calculated_index);
- if let Some(state) =
n_prune_each_partition.get_mut(partition_row) {
- if n_prune < *state {
- *state = n_prune;
+ if let Some(current) =
n_prune_each_partition.get_mut(partition_row) {
Review Comment:
The changes in this file appear to be a drive by cleanup (as in not directly
related to this PR) -- that is fine I am just confirming my understanding
##########
datafusion/core/tests/window_fuzz.rs:
##########
@@ -227,9 +227,7 @@ fn get_random_window_frame(rng: &mut StdRng) -> WindowFrame
{
} else if rand_num < 2 {
WindowFrameUnits::Rows
} else {
- // For now we do not support GROUPS in BoundedWindowAggExec
implementation
- // TODO: once GROUPS handling is available, use
WindowFrameUnits::GROUPS in randomized tests also.
- WindowFrameUnits::Range
+ WindowFrameUnits::Groups
Review Comment:
👍
##########
datafusion/physical-expr/src/window/partition_evaluator.rs:
##########
@@ -38,9 +38,15 @@ pub trait PartitionEvaluator: Debug + Send {
Ok(BuiltinWindowState::Default)
}
+ /// Updates the internal state for Built-in window function
Review Comment:
thank you for adding these comments
--
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]
