etseidl commented on code in PR #221:
URL: https://github.com/apache/parquet-format/pull/221#discussion_r1402657927
##########
src/main/thrift/parquet.thrift:
##########
@@ -288,7 +288,7 @@ struct MapType {} // see LogicalTypes.md
struct ListType {} // see LogicalTypes.md
struct EnumType {} // allowed for BINARY, must be encoded with UTF-8
struct DateType {} // allowed for INT32
-struct Float16Type {} // allowed for FIXED[2], must encoded raw FLOAT16 bytes
+struct Float16Type {} // allowed for FIXED[2], must encoded raw FLOAT16 bytes
(see LogicalTypes.md)
Review Comment:
'must encode' or 'must be encoded as'?
##########
src/main/thrift/parquet.thrift:
##########
@@ -962,15 +967,19 @@ union ColumnOrder {
* BYTE_ARRAY - unsigned byte-wise comparison
* FIXED_LEN_BYTE_ARRAY - unsigned byte-wise comparison
*
- * (*) Because the sorting order is not specified properly for floating
- * point values (relations vs. total ordering) the following
+ * (*) Because the precise sorting order is ambiguous for floating
+ * point types due to underspecified handling of NaN and -0/+0,
+ * it is recommended that writers use IEEE_754_TOTAL_ORDER
+ * for these types.
+ *
+ * If TYPE_ORDER is used for floating point types, then the following
Review Comment:
This line threw me (at least while using my phone 😉...on my computer I can
see `TYPE_ORDER` below). Maybe this could instead say "If this ordering is used
for floating..." or "If this type-defined ordering..."
##########
src/main/thrift/parquet.thrift:
##########
@@ -962,15 +967,19 @@ union ColumnOrder {
* BYTE_ARRAY - unsigned byte-wise comparison
* FIXED_LEN_BYTE_ARRAY - unsigned byte-wise comparison
*
- * (*) Because the sorting order is not specified properly for floating
- * point values (relations vs. total ordering) the following
+ * (*) Because the precise sorting order is ambiguous for floating
+ * point types due to underspecified handling of NaN and -0/+0,
+ * it is recommended that writers use IEEE_754_TOTAL_ORDER
+ * for these types.
+ *
+ * If TYPE_ORDER is used for floating point types, then the following
Review Comment:
This line threw me (at least while using my phone 😉...on my computer I can
see `TYPE_ORDER` below). Maybe this could instead say "If this ordering is used
for floating..." or "If this type-defined ordering..."
--
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]
