You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rip-nsk (via GitHub)" <gi...@apache.org> on 2023/07/21 17:31:52 UTC
[GitHub] [arrow] rip-nsk opened a new pull request, #36814: PARQUET-2327: get min/max column statistics by isset flag
rip-nsk opened a new pull request, #36814:
URL: https://github.com/apache/arrow/pull/36814
### Rationale for this change
min/max statistics can be stored in column chunk metadata in min/max or min_value/max_value fields.
current behavior for selecting a specific pair depends on the sort order, which may contradict to isset flag.
### What changes are included in this PR?
Trivial change to get min/max column statistics by isset flag
### Are these changes tested?
tested on customers data.
### Are there any user-facing changes?
no
--
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] github-actions[bot] commented on pull request #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1646031504
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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] github-actions[bot] commented on pull request #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1646031468
:warning: Ticket **has no components in JIRA**, make sure you assign one.
--
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] rip-nsk commented on pull request #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "rip-nsk (via GitHub)" <gi...@apache.org>.
rip-nsk commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1648874404
> Hmmm, for example, if you're using int32 and uint32, the encoded bytes might be the same, but the handling method would be different. As for string, it might has different collations. So just compare the binary usally doesn't get the right result.
just to confirm, let's we have column chunk with values V1..Vn,
I assume that statistic's min <= of any V1..Vn, and correspondently max >= any of V1..Vn
if I understand correctly, this may be guarantied only for min_value/max_value and only if sort order is not UNDEFINED.
If so, silent returning min/max in other cases may be wrong.
--
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 #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1647943990
I agree with @wgtmac and @mapleFU . The original code looks correct to me and the proposed change does not conform to the spec.
--
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 #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1646500521
By the way, do we discard the min-max when order is not `SINGED` @wgtmac ? Seems if order is not SIGNED, using min-max is a bit dangerous
--
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] rip-nsk commented on pull request #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "rip-nsk (via GitHub)" <gi...@apache.org>.
rip-nsk commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1646617898
I have customers file set (table, writer id - "parquet-mr compatible Photon version 0.2")
where min/max_value are set for string column with undefined order - current implementation ignores them.
In general, for row group filtering (using range predicate by min/max) sort order does not matter,
only the correct min/max values are needed.
--
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 #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1694657881
@rip-nsk Can we close this pr? Since the statistic is already disabled.
--
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] github-actions[bot] commented on pull request #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1646031400
https://issues.apache.org/jira/browse/PARQUET-2327
--
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 #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1649159298
As https://github.com/apache/arrow/issues/34139 mentioned, I'll try to fix it this week.
--
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 #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #36814:
URL: https://github.com/apache/arrow/pull/36814#discussion_r1271252407
##########
cpp/src/parquet/metadata.cc:
##########
@@ -87,8 +87,8 @@ std::string ParquetVersionToString(ParquetVersion::type ver) {
template <typename DType>
static std::shared_ptr<Statistics> MakeTypedColumnStats(
const format::ColumnMetaData& metadata, const ColumnDescriptor* descr) {
- // If ColumnOrder is defined, return max_value and min_value
- if (descr->column_order().get_order() == ColumnOrder::TYPE_DEFINED_ORDER) {
+ // Return max_value and min_value if defined
+ if (metadata.statistics.__isset.max_value || metadata.statistics.__isset.min_value) {
Review Comment:
I don't think `ColumnOrder` checking can be removed here. If a writer does not set ColumnOrder but writes both min/max and min_value/max_value, we should still get stats from min/max as per the specs.
--
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 #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1646753000
> In general, for row group filtering (using range predicate by min/max) sort order does not matter,
only the correct min/max values are needed.
Hmmm, for example, if you're using int32 and uint32, the encoded bytes might be the same, but the handling method would be different. As for string, it might has different collations. So just compare the binary usally doesn't get the right result. As the spec saying [1] "without column_orders, the meaning of the min_value and max_value fields in the Statistics object and the ColumnIndex object is undefined". So I guess this code can not checking.
I think you can confirm with Photon contributor, and as for solving it, you can prune using thrift Statistics.
[1] https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L1049
--
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] PARQUET-2327: get min/max column statistics by isset flag [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1951611657
Close this first. Feel free to reopen 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
Re: [PR] PARQUET-2327: get min/max column statistics by isset flag [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU closed pull request #36814: PARQUET-2327: get min/max column statistics by isset flag
URL: https://github.com/apache/arrow/pull/36814
--
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 #36814: PARQUET-2327: get min/max column statistics by isset flag
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36814:
URL: https://github.com/apache/arrow/pull/36814#issuecomment-1648923857
Yes, you're right. Here maybe it's better to check:
1. If the `Columnorder` and `min_value` `max_value` exists, just return it
2. If the sort-order is SIGNED, return `min` `max` is ok
3. If the sort-order is not SIGNED, we cannot gurantee anything
Current implemention may regard hasing `min` `max` as they pretent to use `SIGNED`, as I mentioned here https://github.com/apache/arrow/pull/36814#issuecomment-1646500521 . Maybe I can try to fix it this week
--
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