You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "emcake (via GitHub)" <gi...@apache.org> on 2023/11/14 13:24:16 UTC
[PR] Enable truncation of binary statistics columns [arrow-rs]
emcake opened a new pull request, #5076:
URL: https://github.com/apache/arrow-rs/pull/5076
# Which issue does this PR close?
Closes #5037.
# Rationale for this change
Similar to parquet-mr (https://github.com/apache/parquet-mr/pull/696), we allow truncation of statistics for binary and fix-length binary columns.
# What changes are included in this PR?
`7b37fd4` introduces the min/max exactness parameters and parses them for various statistics, and ensures round-tripping.
`e57634d` creates a new writer property, and implements the truncation. It's tested for both strings and for decimals, and in the decimal case we ensure that re-constructed min and max decimals of the correct byte length will properly bound the true value.
# Are there any user-facing changes?
Introduction of new functionality to set the truncation length, but no breaking changes.
--
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] Enable truncation of binary statistics columns [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5076:
URL: https://github.com/apache/arrow-rs/pull/5076#discussion_r1392801975
##########
parquet/src/file/statistics.rs:
##########
@@ -152,6 +158,12 @@ pub fn from_thrift(
stats.max_value
};
+ // Whether or not the min/max values are exact. Due to pre-existing truncation
+ // in other libraries such as parquet-mr, we can't assume that any given parquet file
Review Comment:
> No, parquet-mr only applies this to binary statistics
In which case perhaps we can have slightly less pessimistic defaulting behaviour here?
> by splitting the statistics_new_func macro into a statistics_new_func_always_exact and statistics_new_func_inexact
I think I would prefer to avoid this being a breaking change at all, this approach would be consistent with other structures in this codebase
--
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] Enable truncation of binary statistics columns [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5076:
URL: https://github.com/apache/arrow-rs/pull/5076#discussion_r1392780160
##########
parquet/src/file/statistics.rs:
##########
@@ -88,13 +90,17 @@ macro_rules! statistics_new_func {
distinct: Option<u64>,
nulls: u64,
is_deprecated: bool,
+ is_max_value_exact: bool,
+ is_min_value_exact: bool,
Review Comment:
Changing this function signature will result in some non-trivial code churn, what do you think of keeping this function as-is, defaulting the values to `true` and then adding two methods like
```
pub fn with_is_max_value_exact(self, exact: bool) -> Self {
...
}
pub fn with_is_min_value_exact(self, exact: bool) -> Self {
...
}
```
##########
parquet/src/file/statistics.rs:
##########
@@ -152,6 +158,12 @@ pub fn from_thrift(
stats.max_value
};
+ // Whether or not the min/max values are exact. Due to pre-existing truncation
+ // in other libraries such as parquet-mr, we can't assume that any given parquet file
Review Comment:
Does parquet-mr truncate non-binary columns?
--
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] Enable truncation of binary statistics columns [arrow-rs]
Posted by "emcake (via GitHub)" <gi...@apache.org>.
emcake commented on code in PR #5076:
URL: https://github.com/apache/arrow-rs/pull/5076#discussion_r1392951502
##########
parquet/src/file/statistics.rs:
##########
@@ -152,6 +158,12 @@ pub fn from_thrift(
stats.max_value
};
+ // Whether or not the min/max values are exact. Due to pre-existing truncation
+ // in other libraries such as parquet-mr, we can't assume that any given parquet file
Review Comment:
@tustvold should be OK to re-review? I've also fixed the lints that were causing issues with the checks.
--
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] Enable truncation of binary statistics columns [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5076:
URL: https://github.com/apache/arrow-rs/pull/5076#discussion_r1392801975
##########
parquet/src/file/statistics.rs:
##########
@@ -152,6 +158,12 @@ pub fn from_thrift(
stats.max_value
};
+ // Whether or not the min/max values are exact. Due to pre-existing truncation
+ // in other libraries such as parquet-mr, we can't assume that any given parquet file
Review Comment:
> No, parquet-mr only applies this to binary statistics
In which case perhaps we can have slightly less pessimistic defaulting behaviour here?
> by splitting the statistics_new_func macro into a statistics_new_func_always_exact and statistics_new_func_inexact
I think I would prefer to avoid this being a breaking change at all, the approach in https://github.com/apache/arrow-rs/pull/5076#discussion_r1392780160 would be consistent with other structures in this codebase
--
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] Enable truncation of binary statistics columns [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5076:
URL: https://github.com/apache/arrow-rs/pull/5076#discussion_r1392801975
##########
parquet/src/file/statistics.rs:
##########
@@ -152,6 +158,12 @@ pub fn from_thrift(
stats.max_value
};
+ // Whether or not the min/max values are exact. Due to pre-existing truncation
+ // in other libraries such as parquet-mr, we can't assume that any given parquet file
Review Comment:
> No, parquet-mr only applies this to binary statistics
In which case perhaps we can have slightly less pessimistic defaulting behaviour here?
> by splitting the statistics_new_func macro into a statistics_new_func_always_exact and statistics_new_func_inexact
I think I would prefer to avoid this being a breaking change at all, the approach in https://github.com/apache/arrow-rs/pull/5076#discussion_r1392780160 would be consistent with other structures in this codebase and would be my preference unless there is a reason it is insufficient.
--
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] Enable truncation of binary statistics columns [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #5076:
URL: https://github.com/apache/arrow-rs/pull/5076
--
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] Enable truncation of binary statistics columns [arrow-rs]
Posted by "emcake (via GitHub)" <gi...@apache.org>.
emcake commented on code in PR #5076:
URL: https://github.com/apache/arrow-rs/pull/5076#discussion_r1392798590
##########
parquet/src/file/statistics.rs:
##########
@@ -152,6 +158,12 @@ pub fn from_thrift(
stats.max_value
};
+ // Whether or not the min/max values are exact. Due to pre-existing truncation
+ // in other libraries such as parquet-mr, we can't assume that any given parquet file
Review Comment:
No, parquet-mr only applies this to binary statistics: https://github.com/apache/parquet-mr/pull/696/files#diff-1afc9f89a782ddd4e7cd17546ca048954091627d7a31597ab88892eb2a7a76abR618
Pertaining to the conversation above as well - I could reduce churn by only allowing the setting of min/max exactness on the constructors for binary-like stats, by splitting the `statistics_new_func` macro into a `statistics_new_func_always_exact` and `statistics_new_func_inexact` that generates a `binary_with_inexact` method? Given that there's only one place in the code in `column/mod.rs` where we set these to something other than `true`, would reduce the churn significantly.
--
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] Enable truncation of binary statistics columns [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5076:
URL: https://github.com/apache/arrow-rs/pull/5076#discussion_r1392801975
##########
parquet/src/file/statistics.rs:
##########
@@ -152,6 +158,12 @@ pub fn from_thrift(
stats.max_value
};
+ // Whether or not the min/max values are exact. Due to pre-existing truncation
+ // in other libraries such as parquet-mr, we can't assume that any given parquet file
Review Comment:
I think I would prefer to avoid this being a breaking change at all
--
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] Enable truncation of binary statistics columns [arrow-rs]
Posted by "emcake (via GitHub)" <gi...@apache.org>.
emcake commented on code in PR #5076:
URL: https://github.com/apache/arrow-rs/pull/5076#discussion_r1392814811
##########
parquet/src/file/statistics.rs:
##########
@@ -152,6 +158,12 @@ pub fn from_thrift(
stats.max_value
};
+ // Whether or not the min/max values are exact. Due to pre-existing truncation
+ // in other libraries such as parquet-mr, we can't assume that any given parquet file
Review Comment:
OK, will update to that.
--
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