gszadovszky commented on code in PR #3393:
URL: https://github.com/apache/parquet-java/pull/3393#discussion_r3411447849
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java:
##########
@@ -28,6 +31,7 @@ public class BinaryStatistics extends Statistics<Binary> {
private static final PrimitiveType DEFAULT_FAKE_TYPE =
Types.optional(PrimitiveType.PrimitiveTypeName.BINARY).named("fake_binary_type");
+ private final boolean isFloat16;
Review Comment:
I think it would be easier to read if we would have a separate
`Float16Statistics` extending `BinaryStatistics`. WDYT?
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/FloatStatistics.java:
##########
@@ -99,6 +105,25 @@ public boolean isSmallerThan(long size) {
}
public void updateStats(float min_value, float max_value) {
+ if (type().columnOrder().equals(ColumnOrder.ieee754TotalOrder())) {
Review Comment:
See above.
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
Review Comment:
The implementation of this method is not correct. It probably did not face
up, because we did not have different practically used column orders.
The indexing in the `columnOrders` list is incorrect. Example:
```
message msg {
group g {
float a; // column_orders[0]
double b; // column_orders[1]
}
double c; // column_orders[2]
}
root buildChildren(..., columnCount=0)
child g:
recurse with columnCount=0
a uses 0
b uses 1
parent increments columnCount to 1
child c:
uses 1, but should use 2
```
##########
parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java:
##########
Review Comment:
Not tightly related, but the ordering is not taken into account.
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/DoubleStatistics.java:
##########
@@ -98,6 +104,25 @@ public boolean isSmallerThan(long size) {
}
public void updateStats(double min_value, double max_value) {
+ if (type().columnOrder().equals(ColumnOrder.ieee754TotalOrder())) {
Review Comment:
Maybe better having an `IEE754DoubleStatistics` extending `DoubleStatistics`?
--
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]
