You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Fischer0522 (via GitHub)" <gi...@apache.org> on 2024/07/19 09:25:23 UTC
[PR] fix: check has_min_max_set before invoking min()/max() [arrow-rs]
Fischer0522 opened a new pull request, #6092:
URL: https://github.com/apache/arrow-rs/pull/6092
# 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. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #6091.
# Rationale for this change
When column is null in parquet,the ValueStatistics's min max will be set None,so if we call ValueStatistics.min()/max() directly, the program will panic
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
use `ValueStatistics.has_min_max_set()` to check before invoking min/max
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix panic in `ParquetMetadata::memory_size`: check has_min_max_set before invoking min()/max() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6092:
URL: https://github.com/apache/arrow-rs/pull/6092#issuecomment-2241080025
> This test seems odd. In the code, I noticed that the heap size for types like i32 is set to 0, so I set my expected value to 1320. However, the result I got was 2024. When I reran the test, the result changed to 1320. Could you tell me what might be causing this ？@alamb
I think it is because the heap size includes not just the size for the `Statistics` but also the size for the newly added `ColumnMetadata` and `RowGroupMetadata` as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix panic in `ParquetMetadata::memory_size`: check has_min_max_set before invoking min()/max() [arrow-rs]
Posted by "Fischer0522 (via GitHub)" <gi...@apache.org>.
Fischer0522 commented on PR #6092:
URL: https://github.com/apache/arrow-rs/pull/6092#issuecomment-2241056752
This test seems odd. In the code, I noticed that the heap size for types like i32 is set to 0, so I set my expected value to 1320. However, the result I got was 2024. When I reran the test, the result changed to 1320. Could you tell me what might be causing this ？@alamb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: check has_min_max_set before invoking min()/max() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6092:
URL: https://github.com/apache/arrow-rs/pull/6092#issuecomment-2239723884
I think I see the issue and this PR makes sense. Thank you. I still think we need to add test coverage
Perhaps we can add something here https://github.com/apache/arrow-rs/blob/9be0eb530de85a68358dc31c146f71ccb97739ca/parquet/src/file/metadata/mod.rs#L1288-L1287
Here is an issue that tracks making this API less easy to misuse: https://github.com/apache/arrow-rs/issues/6093
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: check has_min_max_set before invoking min()/max() [arrow-rs]
Posted by "Fischer0522 (via GitHub)" <gi...@apache.org>.
Fischer0522 commented on PR #6092:
URL: https://github.com/apache/arrow-rs/pull/6092#issuecomment-2240882956
I add test in https://github.com/apache/arrow-rs/blob/9be0eb530de85a68358dc31c146f71ccb97739ca/parquet/src/file/metadata/mod.rs#L1288-L1287, i check memory with and without statistics. Please review it @alamb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix panic in `ParquetMetadata::memory_size`: check has_min_max_set before invoking min()/max() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6092:
URL: https://github.com/apache/arrow-rs/pull/6092
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: check has_min_max_set before invoking min()/max() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6092:
URL: https://github.com/apache/arrow-rs/pull/6092#discussion_r1685329758
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -1291,7 +1291,11 @@ mod tests {
let columns = schema_descr
.columns()
.iter()
- .map(|column_descr| ColumnChunkMetaData::builder(column_descr.clone()).build())
+ .map(|column_descr| {
Review Comment:
I verified this test covers the bug by running it without the changes in this PR and it panic'd
```
called `Option::unwrap()` on a `None` value
thread 'file::metadata::tests::test_memory_size' panicked at parquet/src/file/statistics.rs:542:27:
called `Option::unwrap()` on a `None` value
stack backtrace:
```
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix panic in `ParquetMetadata::memory_size`: check has_min_max_set before invoking min()/max() [arrow-rs]
Posted by "Fischer0522 (via GitHub)" <gi...@apache.org>.
Fischer0522 commented on PR #6092:
URL: https://github.com/apache/arrow-rs/pull/6092#issuecomment-2241083799
> > This test seems odd. In the code, I noticed that the heap size for types like i32 is set to 0, so I set my expected value to 1320. However, the result I got was 2024. When I reran the test, the result changed to 1320. Could you tell me what might be causing this ？@alamb
>
> I think it is because the heap size includes not just the size for the `Statistics` but also the size for the newly added `ColumnMetadata` and `RowGroupMetadata` as well
I got it, since all checks in CI are passed, can this PR be merged into main？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix panic in `ParquetMetadata::memory_size`: check has_min_max_set before invoking min()/max() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6092:
URL: https://github.com/apache/arrow-rs/pull/6092#issuecomment-2241087345
Thanks again @Fischer0522
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org