-
Notifications
You must be signed in to change notification settings - Fork 1k
check bit width to avoid panic in DeltaBitPackDecoder #8688
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
I wonder would this actually happens? Do someone meets this issue? |
It can happen if an encoder calculates the deltas in the wrong domain (I know because my first attempt at writing an encoder for cudf did just this). The spec had been ambiguous on this point, but we changed the wording to disallow this situation (apache/parquet-format#231). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rambleraptor, this looks good to me (mod lint).
| #[test] | ||
| fn test_delta_bit_packed_invalid_bit_width() { | ||
| // Manually craft a buffer with an invalid bit width | ||
| let mut buffer = vec![]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the documentation here, I think adding #[allow(clippy::vec_init_then_push)] is warranted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a #[allow and pushed a commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to resolve the CI failures
| #[test] | ||
| fn test_delta_bit_packed_invalid_bit_width() { | ||
| // Manually craft a buffer with an invalid bit width | ||
| let mut buffer = vec![]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a #[allow and pushed a commit
Which issue does this PR close?
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
Rationale for this change
The
DeltaBitPackDecodercan panic if it encounters a bit width in the encoded data that is larger than the bit width of the data type being decoded.