-
Notifications
You must be signed in to change notification settings - Fork 789
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
Parquet: Verify 32-bit CRC checksum when decoding pages #6290
Conversation
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.
Can we get some tests phase
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 agree with @tustvold that this code needs to have some tests to ensure we don't break the feature in the future
Also I think the feature flag should be documented here https://crates.io/crates/parquet
FYI: https://github.com/apache/parquet-testing/tree/master/data |
Thanks for the pointers. I added the tests and documented the feature flag. Please take a look |
I am depressed about the large review backlog in this crate. We are looking for more help from the community reviewing PRs -- see #6418 for more |
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 for this! Please run cargo +stable fmt --all
and check in the result. Have you run any benchmarks to see if there is a measurable impact from the crc calculation?
Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>
Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>
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 @xmakro! Looks good to me. I'm fine with this as a feature, but will defer to others who may have a stronger opinion.
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.
Looks good to me 👍
Probably just need to clarify wording on the feature flag.
CI failure looks unrelated, will try look into it.
Edit: looks like CI failure was resolved by #6437. Merging in latest from master should resolve the issue
Thanks for the reviews! I applied the comments and merged master, PTAL |
🚀 |
Thanks everyone |
Closes #6289
Please let me know if we should expose this in the reader APIs instead of a crate feature