feat: implement retract_batch for array_agg sliding window support#22015
feat: implement retract_batch for array_agg sliding window support#22015SubhamSinghal wants to merge 4 commits intoapache:mainfrom
Conversation
github-actions
Bot
added
sqllogictest
There was a problem hiding this comment.
@SubhamSinghal
Thanks for the work here. I found one issue that looks like it can affect sliding window correctness for array_agg(... IGNORE NULLS)
|
|
||
| let val = &values[0]; | ||
| let mut to_retract = if self.ignore_nulls { | ||
| val.len() - val.null_count() |
There was a problem hiding this comment.
I think retract_batch should use the same null definition as update_batch here.
update_batch filters IGNORE NULLS using val.logical_nulls(), but this path counts non-null rows with val.len() - val.null_count(). For arrays where logical nullability differs from physical nullability, such as dictionary or run arrays whose values contain logical nulls, the accumulator can store fewer rows than retract_batch later removes.
That means a sliding array_agg(... IGNORE NULLS) window can over-retract and accidentally drop following values from the frame. Could we switch this to the same null semantics, for example val.len() - val.logical_null_count(), and add a regression test with an input type where logical nulls differ from null_count()?
|
@kosiew Thanks for highlighting the issue. I have fixed null counting bug and added UT |
There was a problem hiding this comment.
@SubhamSinghal
Thanks for addressing the counting issue in retract_batch.
The implementation change looks right to me. I think the regression test still needs one adjustment before this is ready, since it currently does not exercise the logical-null versus physical-null case that caused the original bug.
| builder.append_null(); | ||
| let dict_array: ArrayRef = Arc::new(builder.finish()); | ||
|
|
||
| assert_eq!(dict_array.null_count(), 2); |
There was a problem hiding this comment.
Thanks for adding this regression test. I think it needs one more tweak to cover the original issue properly.
Right now this uses StringDictionaryBuilder::append_null(), which creates null dictionary keys. Because of that, dict_array.null_count() and dict_array.logical_null_count() are both 2, so the test would still pass with the old val.len() - val.null_count() behavior.
Could you please construct a dictionary or run array with valid physical entries that reference null values instead? It would also be helpful to assert that null_count() != logical_null_count() before checking the retract behavior, so the test clearly covers the regression.
There was a problem hiding this comment.
Added assertion for null_count() != logical_null_count().
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
@SubhamSinghal
Thanks for the iteration
Looks 👍 to me
2 participants
Which issue does this PR close?
Closes #21957.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Yes. Using UT
Are there any user-facing changes?
Yes.
array_aggnow works with bounded/sliding window frames. Queries that previously errored now succeed:No breaking API changes