Fix 64bit shift bug in avro reader #13276
Conversation
karthikeyann
added
bug
cpp/src/io/avro/avro.cpp
Outdated
| @@ -27,8 +27,8 @@ template <> | |||
| uint64_t container::get_encoded() | |||
| { | |||
| uint64_t val = 0; | |||
| for (uint64_t len = 0; len < 64; len += 7) { | |||
| auto const byte = get_raw<uint8_t>(); | |||
| for (uint8_t len = 0; len < 64; len += 7) { | |||
There was a problem hiding this comment.
changed to auto.
| @@ -27,8 +27,8 @@ template <> | |||
| uint64_t container::get_encoded() | |||
| for (uint64_t len = 0; len < 64; len += 7) { | ||
| auto const byte = get_raw<uint8_t>(); | ||
| for (uint8_t len = 0; len < 64; len += 7) { | ||
| uint64_t const byte = get_raw<uint8_t>(); |
There was a problem hiding this comment.
From some Godbolt experiments, it turns out that byte gets cast to a 32-bit type during (byte & 0x7f). So this is not a fix from 8-bit to 64-bit, but from 32 to 64.
This also explains why Avro reader was not completely broken :)
I would appreciate a comment about this above this line, and an update to the PR description. The original behavior is definitely not obvious.
Also, it would be great to add a test, but I'm not sure how feasible that is.
There was a problem hiding this comment.
0x7f is int32 literal. So, byte is upcasted to int32. That's why it worked for 32 bit.
Can't add test. I don't know avro well enough to add the test for this fix.
There was a problem hiding this comment.
To test, we would have to have an encoded int larger than uint32::max in the Avro file (and the int might have to be in the metadata??).
I'm okay with skipping the test here, as the fix is very safe.
|
/merge |
Description
Replace 8 bit uint with 64bit so that the shift upto 64 happens correctly. (
cudf::io::avro::container::get_encoded())Checklist