rdblue commented on code in PR #584:
URL: https://github.com/apache/parquet-format/pull/584#discussion_r3456026286
##########
src/main/thrift/parquet.thrift:
##########
@@ -1195,6 +1198,18 @@ union ColumnOrder {
* or max_values indicates that all non-null values are NaN.
*/
2: IEEE754TotalOrder IEEE_754_TOTAL_ORDER;
+
+ /*
+ * The INT96 timestamp type is ordered chronologically. Only columns of
+ * physical type INT96 may use this ordering.
+ *
+ * When writing statistics for columns with INT96_TIMESTAMP_ORDER, two values
+ * must be compared as follows:
Review Comment:
> the spec should clearly specify what an INT96 value means
It should not do this here, in the definition of a sort order. How to handle
these issues is not appropriate for a sort order definition. This definition
should be simple and clear. Instead, this says how to deserialize and sort
based on parts of the value without considering the other parts. As it is, this
is insufficient to specify how INT96 timestamps work and dangerous because it
depends on assumptions about how those values are normalized before they are
serialized.
The simplest solution is to remove the part about how to interpret the
value. It is clear without this for anyone actually writing INT96 timestamps.
--
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]
