Copilot commented on code in PR #50157:
URL: https://github.com/apache/arrow/pull/50157#discussion_r3407944408
##########
cpp/src/parquet/schema.h:
##########
@@ -382,6 +382,19 @@ class PARQUET_EXPORT ColumnDescriptor {
return la ? GetSortOrder(la, pt) : GetSortOrder(converted_type(), pt);
}
+ // Whether ColumnOrder-governed min/max values have a supported ordering.
+ bool can_use_min_max() const {
+ switch (column_order().get_order()) {
+ case ColumnOrder::TYPE_DEFINED_ORDER:
+ return sort_order() != SortOrder::UNKNOWN;
+ case ColumnOrder::UNDEFINED:
+ return sort_order() == SortOrder::SIGNED;
+ case ColumnOrder::UNKNOWN:
+ return false;
+ }
+ return false;
Review Comment:
The PR description says page index min/max should be ignored when
ColumnOrder is missing/unsupported (or SortOrder is unknown), but the only new
gating helper here (`can_use_min_max`) still returns true for
`UNDEFINED`+`SIGNED` and I can’t find any corresponding guard in the page index
read path (e.g., `RowGroupPageIndexReaderImpl::GetColumnIndex()` /
`ColumnChunkMetaData::GetColumnIndexLocation()` don’t check column/sort order).
This looks like either (a) a mismatch with the PR description or (b) missing
changes to ensure callers can’t mistakenly use ColumnIndex min/max when
ordering is unknown/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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
