You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mapleFU (via GitHub)" <gi...@apache.org> on 2023/07/25 11:43:24 UTC
[GitHub] [arrow] mapleFU opened a new pull request, #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
mapleFU opened a new pull request, #36866:
URL: https://github.com/apache/arrow/pull/36866
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
<!--
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.
-->
As mentioned:
1. https://github.com/apache/arrow/issues/34139
2. https://github.com/apache/arrow/pull/36814
disable `min` `max` when `min_value` `max_value` is not reachable. And sort-order is not `SIGNED`
### What changes are included in this PR?
add a `can_has_legacy_min_max` check here.
<!--
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 these changes tested?
no
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
4. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
Yes, some user may cannot get the statistics here.
<!--
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 uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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
[GitHub] [arrow] wgtmac commented on a diff in pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #36866:
URL: https://github.com/apache/arrow/pull/36866#discussion_r1274288081
##########
cpp/src/parquet/metadata.cc:
##########
@@ -97,12 +97,15 @@ static std::shared_ptr<Statistics> MakeTypedColumnStats(
metadata.statistics.__isset.null_count,
metadata.statistics.__isset.distinct_count);
}
+ // GH-34139: If SortOrder is not SIGNED, ignore min and max.
+ bool can_use_legacy_min_max = descr->sort_order() == SortOrder::SIGNED;
// Default behavior
return MakeStatistics<DType>(
descr, metadata.statistics.min, metadata.statistics.max,
metadata.num_values - metadata.statistics.null_count,
metadata.statistics.null_count, metadata.statistics.distinct_count,
- metadata.statistics.__isset.max || metadata.statistics.__isset.min,
+ can_use_legacy_min_max &&
+ (metadata.statistics.__isset.max || metadata.statistics.__isset.min),
Review Comment:
I tried to change `||` to `&&` in an old PR and it caused some test failure because filter pushdown of dataset scanner can handle the case when either min or max is missing.
--
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
[GitHub] [arrow] mapleFU commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1652828404
Close as already fixed
--
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
[GitHub] [arrow] mapleFU commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1651711260
Oops, I found a `HasCorrectStats` here:
```c++
// Reference:
// parquet-mr/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
// PARQUET-686 has more discussion on statistics
bool ApplicationVersion::HasCorrectStatistics(Type::type col_type,
EncodedStatistics& statistics,
SortOrder::type sort_order) const {
// parquet-cpp version 1.3.0 and parquet-mr 1.10.0 onwards stats are computed
// correctly for all types
if ((application_ == "parquet-cpp" && VersionLt(PARQUET_CPP_FIXED_STATS_VERSION())) ||
(application_ == "parquet-mr" && VersionLt(PARQUET_MR_FIXED_STATS_VERSION()))) {
// Only SIGNED are valid unless max and min are the same
// (in which case the sort order does not matter)
bool max_equals_min = statistics.has_min && statistics.has_max
? statistics.min() == statistics.max()
: false;
if (SortOrder::SIGNED != sort_order && !max_equals_min) {
return false;
}
// Statistics of other types are OK
if (col_type != Type::FIXED_LEN_BYTE_ARRAY && col_type != Type::BYTE_ARRAY) {
return true;
}
}
// created_by is not populated, which could have been caused by
// parquet-mr during the same time as PARQUET-251, see PARQUET-297
if (application_ == "unknown") {
return true;
}
// Unknown sort order has incorrect stats
if (SortOrder::UNKNOWN == sort_order) {
return false;
}
// PARQUET-251
if (VersionLt(PARQUET_251_FIXED_VERSION())) {
return false;
}
return true;
}
```
Though `Statistics` would be wrong when parsing from `ColumnMetadata`, however, when calling `ColumnChunkMetadata::statistics()`, it will found that it's an old file and discard it.
Should I keep this code to avoid generate `Statistics` with ambigious min-max? Or just leave the code here? @pitrou @wgtmac
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36866:
URL: https://github.com/apache/arrow/pull/36866#discussion_r1273570336
##########
cpp/src/parquet/metadata.cc:
##########
@@ -97,12 +97,15 @@ static std::shared_ptr<Statistics> MakeTypedColumnStats(
metadata.statistics.__isset.null_count,
metadata.statistics.__isset.distinct_count);
}
+ // GH-34139: If SortOrder is not SIGNED, ignore min and max.
+ bool can_has_legacy_min_max = descr->sort_order() == SortOrder::SIGNED;
Review Comment:
```suggestion
bool can_use_legacy_min_max = descr->sort_order() == SortOrder::SIGNED;
```
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36866:
URL: https://github.com/apache/arrow/pull/36866#discussion_r1274463827
##########
cpp/src/parquet/metadata.cc:
##########
@@ -97,12 +97,15 @@ static std::shared_ptr<Statistics> MakeTypedColumnStats(
metadata.statistics.__isset.null_count,
metadata.statistics.__isset.distinct_count);
}
+ // GH-34139: If SortOrder is not SIGNED, ignore min and max.
+ bool can_use_legacy_min_max = descr->sort_order() == SortOrder::SIGNED;
// Default behavior
return MakeStatistics<DType>(
descr, metadata.statistics.min, metadata.statistics.max,
metadata.num_values - metadata.statistics.null_count,
metadata.statistics.null_count, metadata.statistics.distinct_count,
- metadata.statistics.__isset.max || metadata.statistics.__isset.min,
+ can_use_legacy_min_max &&
+ (metadata.statistics.__isset.max || metadata.statistics.__isset.min),
Review Comment:
Ah, funny. Let's keep it like this, then.
--
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
[GitHub] [arrow] pitrou commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1652089617
Ah, I see, it's the other way round: `Statistics` used `HasCorrectStatistics` and `MakeTypedColumnStats` under the hood. So I agree this can be closed.
--
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
[GitHub] [arrow] pitrou commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1649693145
What does parquet-mr do in this situation?
--
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
[GitHub] [arrow] mapleFU commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1649700454
1. Parquet-mr: https://github.com/apache/parquet-mr/blob/master/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java#L797
2. Rust: https://github.com/apache/arrow-rs/blob/918959b4ac08d190f8a87fdd18d74ae37c3c22bf/parquet/src/file/statistics.rs#L458 (see `is_min_max_deprecated` )
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36866:
URL: https://github.com/apache/arrow/pull/36866#discussion_r1273581829
##########
cpp/src/parquet/metadata.cc:
##########
@@ -97,12 +97,15 @@ static std::shared_ptr<Statistics> MakeTypedColumnStats(
metadata.statistics.__isset.null_count,
metadata.statistics.__isset.distinct_count);
}
+ // GH-34139: If SortOrder is not SIGNED, ignore min and max.
+ bool can_use_legacy_min_max = descr->sort_order() == SortOrder::SIGNED;
// Default behavior
return MakeStatistics<DType>(
descr, metadata.statistics.min, metadata.statistics.max,
metadata.num_values - metadata.statistics.null_count,
metadata.statistics.null_count, metadata.statistics.distinct_count,
- metadata.statistics.__isset.max || metadata.statistics.__isset.min,
+ can_use_legacy_min_max &&
+ (metadata.statistics.__isset.max || metadata.statistics.__isset.min),
Review Comment:
Note that parquet-mr uses `&&` rather than `||`. It probably doesn't make a difference in practice (either min/max are both available, or none is available).
--
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
[GitHub] [arrow] wgtmac commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1652068673
In this case, I would prefer the current behavior and close this PR.
--
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
[GitHub] [arrow] mapleFU commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1649671976
@wgtmac @pitrou
Also cc @rip-nsk
--
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
[GitHub] [arrow] mapleFU closed pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU closed pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
URL: https://github.com/apache/arrow/pull/36866
--
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
[GitHub] [arrow] mapleFU commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1649901627
I can try to add one in `metadata_test.cc` tomorrow, so tired today
--
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
[GitHub] [arrow] pitrou commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1649888606
@mapleFU Would it be easy to add a test here?
--
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
[GitHub] [arrow] pitrou commented on pull request #36866: GH-34139: [C++][Parquet] disable UNSIGNED min-max when not has min_value and max_value
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36866:
URL: https://github.com/apache/arrow/pull/36866#issuecomment-1652083386
`HasCorrectStatistics` isn't used by `MakeTypedStatistics`, is it?
--
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