etseidl commented on code in PR #9619:
URL: https://github.com/apache/arrow-rs/pull/9619#discussion_r3318581525
##########
parquet/src/basic.rs:
##########
@@ -1025,14 +1029,36 @@ impl ColumnOrder {
converted_type: ConvertedType,
physical_type: Type,
) -> SortOrder {
- Self::sort_order_for_type(logical_type.as_ref(), converted_type,
physical_type)
+ Self::column_order_for_type(logical_type.as_ref(), converted_type,
physical_type)
+ .sort_order()
+ }
+
+ /// Returns the `ColumnOrder` for a physical/logical type.
+ pub fn column_order_for_type(
+ logical_type: Option<&LogicalType>,
+ converted_type: ConvertedType,
+ physical_type: Type,
+ ) -> ColumnOrder {
+ if Some(&LogicalType::Float16) == logical_type
+ || matches!(physical_type, Type::FLOAT | Type::DOUBLE)
+ {
+ ColumnOrder::IEEE_754_TOTAL_ORDER
+ } else {
+ let sort_order =
+ Self::sort_order_for_type(logical_type, converted_type,
physical_type, true);
+ ColumnOrder::TYPE_DEFINED_ORDER(sort_order)
+ }
}
/// Returns sort order for a physical/logical type.
+ ///
+ /// `is_type_defined` indicates whether the column order for this type is
Review Comment:
There's kind of a chicken/egg thing going on here. `TYPE_DEFINED_ORDER`
contains a `SortOrder`, which is not yet known when trying to create the
`ColumnOrder`. I think this function should not be public, and instead
`SortOrder` should be obtained via `ColumnOrder::sort_order`.
How do you feel about making this `pub(crate)`?
--
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]
