colexec: fix external aggregator fallback and bool agg functions reset #59055
Conversation
Previously, `reset` method of the ordered aggregator would always set the flag to reset the internal batch to `true`. However, that batch is only allocated when `Next` is called at least once with a non-zero batch coming from the input, which is not the case when the fallback to the disk-backed strategy occurs in the external aggregator (there, we call `reset` before we used the operator every time). This would lead to a nil pointer exception, and it is now fixed. Our unit tests didn't catch it because we forgot to set the forced number of repartitions which is now also fixed. This also revealed a bug with resetting of `bool_and` and `bool_or` aggregates - we forgot to reset whether they have seen a non-null value or not. Release note: None (no stable release with these bugs)
|
This change is |
There was a problem hiding this comment.
Nice, could you add a small regression test for the bool_{and,or} bug?
Reviewed 8 of 8 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
|
Note the |
|
But those tests test mainly correctness in the presence of disk spilling, right? What I mean is maybe a logic test or something simple that would trigger the |
|
As discussed in standup, |
|
I'll probably violate some rule by not waiting for an LGTM on this PR, but I had a verbal approval from Alfonso during the standup, and this PR fixes the last known release blocker from our team, so I'll go ahead and merge as is. If anything comes up, we can fix it up later. TFTR! bors r+ |
|
Build succeeded: |
Previously,
resetmethod of the ordered aggregator would always setthe flag to reset the internal batch to
true. However, that batch isonly allocated when
Nextis called at least once with a non-zero batchcoming from the input, which is not the case when the fallback to the
disk-backed strategy occurs in the external aggregator (there, we call
resetbefore we use the operator every time). This would lead toa nil pointer exception, and it is now fixed.
Our unit tests didn't catch it because we forgot to set the forced
number of repartitions which is now also fixed.
This also revealed a bug with resetting of
bool_andandbool_oraggregates - we forgot to reset whether they have seen a non-null value
or not.
Fixes: #59043.
Release note: None (no stable release with these bugs)