You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2023/01/13 16:25:32 UTC
[GitHub] [arrow-rs] tustvold opened a new pull request, #3527: Write backwards compatible row group statistics (#3526)
tustvold opened a new pull request, #3527:
URL: https://github.com/apache/arrow-rs/pull/3527
# 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 #3526
# 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.
-->
This helps with compatibility with pyarrow, which doesn't understand min_value yet https://github.com/apache/arrow/issues/13976
# What changes are included in this PR?
<!--
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?
<!--
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
[GitHub] [arrow-rs] alamb commented on pull request #3527: Write backwards compatible row group statistics (#3526)
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3527:
URL: https://github.com/apache/arrow-rs/pull/3527#issuecomment-1382750186
I think this may also fix https://github.com/apache/arrow-rs/issues/799 filed by @tfiasco and which @pjmore also commented on
--
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-rs] alamb commented on pull request #3527: Write backwards compatible row group statistics (#3526)
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3527:
URL: https://github.com/apache/arrow-rs/pull/3527#issuecomment-1384720807
Thanks @tustvold
--
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-rs] tustvold commented on a diff in pull request #3527: Write backwards compatible row group statistics (#3526)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3527:
URL: https://github.com/apache/arrow-rs/pull/3527#discussion_r1070627685
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1893,6 +1896,7 @@ mod tests {
fn test_bool_statistics() {
let stats = statistics_roundtrip::<BoolType>(&[true, false, false, true]);
assert!(stats.has_min_max_set());
+ assert!(!stats.is_min_max_backwards_compatible());
Review Comment:
Yup, added a doc to that effect
--
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-rs] tustvold commented on pull request #3527: Write backwards compatible row group statistics (#3526)
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3527:
URL: https://github.com/apache/arrow-rs/pull/3527#issuecomment-1383187534
I've updated the docs to hopefully be a bit clearer, PTAL.
I dropped the mentions of parquet v1, as I'm not entirely sure when the deprecation actually happened - the parquet changelog isn't particularly clear on this aspect...
--
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-rs] ursabot commented on pull request #3527: Write backwards compatible row group statistics (#3526)
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #3527:
URL: https://github.com/apache/arrow-rs/pull/3527#issuecomment-1383274606
Benchmark runs are scheduled for baseline = 5a7ec4624d35af0a860d84274308ba4f5303cada and contender = 95cf030137cd7b925e89c102fb060b971e4a3836. 95cf030137cd7b925e89c102fb060b971e4a3836 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/47086e10f5c34faa88337627d9938584...0fdcbf83f9e348ddbf564858bbe5bf85/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/4004929f6d5f4c0094f136170d0deb60...7c2d12fd50d448e38558ba4a86c2ce9b/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/12bbcbd5da3f4ce2a942310cce949ae0...bf9508cf0bfb4b519ca3f3b3e19c84b6/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/6ece3f1836854eb5b418819b9f8fa659...cb0ac943643445e3af6cdfb054181e8e/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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-rs] tustvold commented on pull request #3527: Write backwards compatible row group statistics (#3526)
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3527:
URL: https://github.com/apache/arrow-rs/pull/3527#issuecomment-1382124864
Test failures fixed in #3528
--
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-rs] alamb commented on a diff in pull request #3527: Write backwards compatible row group statistics (#3526)
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #3527:
URL: https://github.com/apache/arrow-rs/pull/3527#discussion_r1070593191
##########
parquet/src/file/statistics.rs:
##########
@@ -329,6 +331,12 @@ impl Statistics {
statistics_enum_func![self, is_min_max_deprecated]
}
+ /// Returns `true` if the statistics are backwards compatible with the
+ /// deprecated `min` and `max` fields
Review Comment:
```suggestion
/// deprecated `min` and `max` fields in parquet version `1.0` spec
```
##########
parquet/src/column/writer/mod.rs:
##########
@@ -817,13 +817,16 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
.set_dictionary_page_offset(dict_page_offset);
if self.statistics_enabled != EnabledStatistics::None {
- let statistics = Statistics::new(
+ let statistics = ValueStatistics::<E::T>::new(
self.column_metrics.min_column_value.clone(),
self.column_metrics.max_column_value.clone(),
self.column_metrics.column_distinct_count,
self.column_metrics.num_column_nulls,
false,
);
+ let statistics = statistics
Review Comment:
```suggestion
// Some common readers only support "old" statistics format
// so write them out if possibe. See
// https://github.com/apache/arrow-rs/issues/799
let statistics = statistics
```
##########
parquet/src/file/statistics.rs:
##########
@@ -405,7 +413,13 @@ pub struct ValueStatistics<T> {
// Distinct count could be omitted in some cases
distinct_count: Option<u64>,
null_count: u64,
+
+ /// If `true` populate the deprecated `min` and `max` fields instead of
+ /// `min_value` and `max_value`
is_min_max_deprecated: bool,
+
+ /// If `true` should always write deprecated `min` and `max` fields
Review Comment:
is this in addition to `min_value` and `max_value`?
I found it hard to understand how these two flags were supposed to interact
##########
parquet/src/file/statistics.rs:
##########
@@ -478,6 +501,12 @@ impl<T: ParquetValueType> ValueStatistics<T> {
fn is_min_max_deprecated(&self) -> bool {
self.is_min_max_deprecated
}
+
+ /// Returns `true` if the statistics are backwards compatible
Review Comment:
is it that the statistics are backwards compatible or they are desired to be?
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1893,6 +1896,7 @@ mod tests {
fn test_bool_statistics() {
let stats = statistics_roundtrip::<BoolType>(&[true, false, false, true]);
assert!(stats.has_min_max_set());
+ assert!(!stats.is_min_max_backwards_compatible());
Review Comment:
is it just that bool is not backwards compatibel?
##########
parquet/src/file/statistics.rs:
##########
@@ -252,10 +252,12 @@ pub fn to_thrift(stats: Option<&Statistics>) -> Option<TStatistics> {
(None, None)
};
- if stats.is_min_max_deprecated() {
- thrift_stats.min = min;
- thrift_stats.max = max;
- } else {
+ if stats.is_min_max_backwards_compatible() {
Review Comment:
```suggestion
// Copy to min/max values from parquet 1.0 for compatibility
// with other writers
if stats.is_min_max_backwards_compatible() {
```
##########
parquet/src/file/statistics.rs:
##########
@@ -423,6 +437,15 @@ impl<T: ParquetValueType> ValueStatistics<T> {
distinct_count,
null_count,
is_min_max_deprecated,
+ is_min_max_backwards_compatible: is_min_max_deprecated,
+ }
+ }
+
+ /// Set whether to write the deprecated `min` and `max` fields
Review Comment:
```suggestion
/// Set whether to write the deprecated `min` and `max` fields
/// for compatibility with older parquet writers
```
--
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-rs] tustvold merged pull request #3527: Write backwards compatible row group statistics (#3526)
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #3527:
URL: https://github.com/apache/arrow-rs/pull/3527
--
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