Fix undefined behavior in quantiles function
#44067
Conversation
alexey-milovidov
added
the
pr-must-backport
robot-ch-test-poll2
added
the
pr-bugfix
| if constexpr (is_decimal<Value>) | ||
| result[indices[i]] = Value(static_cast<typename Value::NativeType>(data.quantileInterpolated(levels[indices[i]]))); | ||
| { | ||
| if (is_empty) |
There was a problem hiding this comment.
Regarding UbSan fix, - this check is actually done inside quantileInterpolated() but this function on empty values, it returns NaN double. I fixed it in #43103, but it leads to changing behavior, see here.
If it's ok, then I guess, it'd be a preferable fix for undefined behavior:
(1) it respects onEmpty policy of ReservoirSampler* (not sure how it's important)
(2) it works for QuantileReservoirSampler::get() as well, current fix addresses only getMany()
There was a problem hiding this comment.
Changing the behavior on an empty column is not ok. Maybe not a big deal, but we are very concerned about incompatibilities recently.
I thought about removing the onEmpty policy, as after this change we have a duplication of logic, but did not get to that.
The point on get is valid, it should be fixed as well...
Backport #44067 to 22.10: Fix undefined behavior in `quantiles` function
Backport #44067 to 22.11: Fix undefined behavior in `quantiles` function
Backport #44067 to 22.9: Fix undefined behavior in `quantiles` function
robot-ch-test-poll
added
the
pr-backports-created
Backport #44067 to 22.8: Fix undefined behavior in `quantiles` function
Backport #44067 to 22.3: Fix undefined behavior in `quantiles` function
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix undefined behavior in the
quantilesfunction, which might lead to uninitialized memory. Found by fuzzer. This closes #44066.