alamb commented on code in PR #7687:
URL: https://github.com/apache/arrow-rs/pull/7687#discussion_r2167307696
##########
parquet/src/data_type.rs:
##########
@@ -132,6 +132,23 @@ impl Int96 {
}
}
+impl PartialOrd for Int96 {
Review Comment:
In my mind this rationale for this code is super unobvious from the code and
it would be great to document the rationale as a comment.
Thus I would like to request to add some documentation explaining that this
is implementing comparisons using the logical timestamp type semantics with a
link to the relevant portion of the spec?
I think it is
https://github.com/apache/parquet-format/blob/cf943c197f4fad826b14ba0c40eb0ffdab585285/src/main/thrift/parquet.thrift#L1079
Perhaps something like
```suggestion
/// Order `Int96` correctly for (deprecated) timestamp types
///
/// Note: this is done even though the Int96 type is deprecated and the
/// [spec does not define the sort order]
/// because some engines, notably Spark and DataBricks photon still write
/// Int96 timestamps and rely on their order for optimization.
///
/// [spec does not define the sort order]:
https://github.com/apache/parquet-format/blob/cf943c197f4fad826b14ba0c40eb0ffdab585285/src/main/thrift/parquet.thrift#L1079
impl PartialOrd for Int96 {
```
--
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]
