-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-38039: [C++][Parquet] Fix segfault getting compression level for a Parquet column #38025
Conversation
Would you mind create an issue for this? I guess this is not a "minor" fix for arrow github manangement |
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.
|
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.
+1, thanks a lot @adamreeve . I created an issue for this and will merge.
Thanks for making the issue @pitrou, and apologies for not making one to start with, I should have read the contributing doc more closely |
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 0fbfffb. There were 10 benchmark results indicating a performance regression:
The full Conbench report has more details. |
… for a Parquet column (apache#38025) ### Rationale for this change After the changes in apache#35886, getting the compression level for a Parquet column segfaults if the compression level or other options weren't previously set ### What changes are included in this PR? Adds a null check on the codec options of the column properties before trying to access the compression level. ### Are these changes tested? Yes, I added a unit test. ### Are there any user-facing changes? This fixes a regression added after 13.0.0 so isn't a user-facing fix * Closes: apache#38039 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
… for a Parquet column (apache#38025) ### Rationale for this change After the changes in apache#35886, getting the compression level for a Parquet column segfaults if the compression level or other options weren't previously set ### What changes are included in this PR? Adds a null check on the codec options of the column properties before trying to access the compression level. ### Are these changes tested? Yes, I added a unit test. ### Are there any user-facing changes? This fixes a regression added after 13.0.0 so isn't a user-facing fix * Closes: apache#38039 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
… for a Parquet column (apache#38025) ### Rationale for this change After the changes in apache#35886, getting the compression level for a Parquet column segfaults if the compression level or other options weren't previously set ### What changes are included in this PR? Adds a null check on the codec options of the column properties before trying to access the compression level. ### Are these changes tested? Yes, I added a unit test. ### Are there any user-facing changes? This fixes a regression added after 13.0.0 so isn't a user-facing fix * Closes: apache#38039 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
Rationale for this change
After the changes in #35886, getting the compression level for a Parquet column segfaults if the compression level or other options weren't previously set
What changes are included in this PR?
Adds a null check on the codec options of the column properties before trying to access the compression level.
Are these changes tested?
Yes, I added a unit test.
Are there any user-facing changes?
This fixes a regression added after 13.0.0 so isn't a user-facing fix