Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 64bit shift bug in avro reader #13276

Merged
merged 3 commits into from
May 13, 2023
Merged

Fix 64bit shift bug in avro reader #13276

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7025d19
fix 64bit shift bug
karthikeyann May 2, 2023
a9cbfa3
Merge branch 'branch-23.06' of github.com:rapidsai/cudf into fix-avro…
karthikeyann May 12, 2023
8e8a209
address review comments
karthikeyann May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cpp/src/io/avro/avro.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ template <>
uint64_t container::get_encoded()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another varint decode :)

{
uint64_t val = 0;
for (uint64_t len = 0; len < 64; len += 7) {
auto const byte = get_raw<uint8_t>();
for (auto len = 0; len < 64; len += 7) {
// 64-bit int since shift left is upto 64.
uint64_t const byte = get_raw<uint8_t>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val |= (byte & 0x7f) << len;
if (byte < 0x80) break;
}
Expand Down