rdblue commented on a change in pull request #1641:
URL: https://github.com/apache/iceberg/pull/1641#discussion_r521660283
##########
File path: core/src/test/java/org/apache/iceberg/TestMetrics.java
##########
@@ -347,14 +327,120 @@ public void testMetricsForNullColumns() throws
IOException {
Record secondRecord = GenericRecord.create(schema);
secondRecord.setField("intCol", null);
- InputFile recordsFile = writeRecords(schema, firstRecord, secondRecord);
-
- Metrics metrics = getMetrics(recordsFile);
+ Metrics metrics = getMetrics(schema, firstRecord, secondRecord);
Assert.assertEquals(2L, (long) metrics.recordCount());
assertCounts(1, 2L, 2L, metrics);
assertBounds(1, IntegerType.get(), null, null, metrics);
}
+ @Test
+ public void testMetricsForNaNColumns() throws IOException {
+ Record firstRecord = GenericRecord.create(FLOAT_DOUBLE_ONLY_SCHEMA);
+ firstRecord.setField("floatCol", Float.NaN);
+ firstRecord.setField("doubleCol", Double.NaN);
+ Record secondRecord = GenericRecord.create(FLOAT_DOUBLE_ONLY_SCHEMA);
+ secondRecord.setField("floatCol", Float.NaN);
+ secondRecord.setField("doubleCol", Double.NaN);
+
+ Metrics metrics = getMetrics(FLOAT_DOUBLE_ONLY_SCHEMA, firstRecord,
secondRecord);
+ Assert.assertEquals(2L, (long) metrics.recordCount());
+ assertCounts(1, 2L, 0L, 2L, metrics);
+ assertCounts(2, 2L, 0L, 2L, metrics);
+ // below: current behavior; will be null once NaN is excluded from
upper/lower bound
+ assertBounds(1, FloatType.get(), Float.NaN, Float.NaN, metrics);
+ assertBounds(2, DoubleType.get(), Double.NaN, Double.NaN, metrics);
+ }
+
+ @Test
+ public void testColumnBoundsWithNaNValueAtFront() throws IOException {
+ Record nonNaNRecord1 = GenericRecord.create(FLOAT_DOUBLE_ONLY_SCHEMA);
+ nonNaNRecord1.setField("floatCol", 1.2F);
+ nonNaNRecord1.setField("doubleCol", 3.4D);
+
+ Record nonNaNRecord2 = GenericRecord.create(FLOAT_DOUBLE_ONLY_SCHEMA);
+ nonNaNRecord2.setField("floatCol", 5.6F);
+ nonNaNRecord2.setField("doubleCol", 7.8D);
+
+ Record nanRecord = GenericRecord.create(FLOAT_DOUBLE_ONLY_SCHEMA);
+ nanRecord.setField("floatCol", Float.NaN);
+ nanRecord.setField("doubleCol", Double.NaN);
+
+ Metrics metrics = getMetrics(FLOAT_DOUBLE_ONLY_SCHEMA, nanRecord,
nonNaNRecord1, nonNaNRecord2);
+ Assert.assertEquals(3L, (long) metrics.recordCount());
+ assertCounts(1, 3L, 0L, 1L, metrics);
+ assertCounts(2, 3L, 0L, 1L, metrics);
+
+ // below: current behavior; will be non-NaN values once NaN is excluded
from upper/lower bound. ORC and Parquet's
+ // behaviors differ due to their implementation of comparison being
different.
+ if (fileFormat() == FileFormat.ORC) {
+ assertBounds(1, FloatType.get(), Float.NaN, Float.NaN, metrics);
+ assertBounds(2, DoubleType.get(), Double.NaN, Double.NaN, metrics);
Review comment:
NaN as an upper bound should be safe, but NaN as a lower bound may not
be. Does this mean we need to fix our evaluators to check for NaN?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
