jayzhan211 commented on code in PR #24525:
URL: https://github.com/apache/datafusion/pull/24525#discussion_r3837437182
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -57,6 +58,54 @@ use std::sync::Arc;
/// would be too unreliable otherwise.
const PARTIAL_NDV_THRESHOLD: f64 = 0.75;
+fn requires_unsigned_byte_array_order(column: &ColumnDescriptor) -> bool {
+ matches!(
+ column.physical_type(),
+ PhysicalType::BYTE_ARRAY | PhysicalType::FIXED_LEN_BYTE_ARRAY
+ ) && column.sort_order() != SortOrder::SIGNED
+}
+
+/// Whether byte-array bounds lack a recognized unsigned comparison order.
+///
+/// The deprecated Parquet `min`/`max` fields use signed comparison, unlike
+/// Arrow's string and binary comparisons. Even the modern bounds cannot be
+/// interpreted without the corresponding footer `column_orders` entry.
+/// Signed logical types, such as decimals, retain their existing behavior.
+pub(crate) fn has_untrusted_byte_array_order(
Review Comment:
```suggestion
/// Whether a column's min/max bounds lack a comparison order matching
Arrow's.
///
/// Two cases make bounds unusable:
///
/// * Byte arrays. The deprecated Parquet `min`/`max` fields use signed
/// comparison, unlike Arrow's string and binary comparisons, and even the
/// modern bounds cannot be interpreted without the corresponding footer
/// `column_orders` entry. Signed logical types, such as decimals, retain
/// their existing behavior.
/// * Any column whose sort order is `UNDEFINED`, such as `INT96`. Those
/// bounds have no defined comparison at all. parquet-rs has no `INT96`
/// statistics iterator, so it never surfaces such bounds to Arrow today
and
/// this arm is defensive, but it keeps the rule complete if that changes.
/// Undefined byte-array orders, such as `INTERVAL`, are already rejected
by
/// the unsigned-order check below.
pub(crate) fn has_untrusted_min_max_order(
parquet_schema: &SchemaDescriptor,
column_orders: Option<&[ColumnOrder]>,
parquet_column_index: usize,
) -> bool {
let column = parquet_schema.column(parquet_column_index);
// `ColumnDescriptor::sort_order` is derived from the logical, converted
// and physical type, independent of the footer.
if column.sort_order() == SortOrder::UNDEFINED {
return true;
}
requires_unsigned_byte_array_order(&column)
&& (column.sort_order() != SortOrder::UNSIGNED
|| column_orders
.and_then(|orders| orders.get(parquet_column_index))
.copied()
!=
Some(ColumnOrder::TYPE_DEFINED_ORDER(SortOrder::UNSIGNED)))
}
```
We might need to check int96 case too
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
