fix: Make width_bucket() behave like in Presto Java. #11557
Conversation
Summary: Presto Java treats null elements in the input array vector as zeroes, ignoring null flag. This is not entirely correct, but at least consistent. Velox currently just reads what value is in the element, ignoring null flag and is inconsistent as the memory is not necessarily zeroed. Differential Revision: D66043501
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D66043501 |
✅ Deploy Preview for meta-velox canceled.
|
| // Adhere to this behavior for now. | ||
| template <typename T> | ||
| T valueOrZero(const DecodedVector& elementsHolder, vector_size_t idx) { | ||
| return elementsHolder.isNullAt(idx) ? T{0} : elementsHolder.valueAt<T>(idx); |
There was a problem hiding this comment.
I would be strong against this and we should throw whenever we see null here
There was a problem hiding this comment.
@Yuhta I understand your position and I also want to fix it properly.
However, at the moment it will break some queries and behavior will differ from Presto.
This will complicate the adoption and roll out of the Prestissimo for both users and us.
I have created Presto issue to come with the conclusion of what we want to do about this problem: prestodb/presto#24055
Hopefully, soon we can alter Presto's behavior and with it, the Velox's.
Thanks!
There was a problem hiding this comment.
The Java queries have NULLs in bins will for sure not giving what the user expects. If we are worry about user reports, just throw VELOX_FAIL_UNSUPPORTED_INPUT_UNCATCHABLE so the queries will be retried on Java.
spershin
changed the title
Summary:
Presto Java treats null elements in the input array vector as zeroes,
ignoring null flag. This is not entirely correct, but at least consistent.
Velox currently just reads what value is in the element, ignoring null flag and
is inconsistent as the memory is not necessarily zeroed.
Differential Revision: D66043501