aihuaxu commented on code in PR #13195:
URL: https://github.com/apache/iceberg/pull/13195#discussion_r2365936666


##########
core/src/test/java/org/apache/iceberg/expressions/TestInclusiveMetricsEvaluatorWithExtract.java:
##########
@@ -683,4 +690,249 @@ public void testIntegerNotIn() {
         .as("Should read: id above upper bound (85 > 79, 86 > 79)")
         .isTrue();
   }
+
+  private static Stream<Arguments> timestampEqParameters() {
+    return Stream.of(
+        Arguments.of(
+            Types.TimestampNanoType.withoutZone().toString(),
+            "1970-03-21T00:00:01.123456789",
+            Variants.ofIsoTimestampntz("1970-01-31T00:00:01.123456"),
+            Variants.ofIsoTimestampntz("1970-03-31T00:00:01.123456")),
+        Arguments.of(
+            Types.DateType.get().toString(),
+            "1970-03-21",
+            Variants.ofIsoTimestampntz("1970-01-31T00:00:01.123456"),
+            Variants.ofIsoTimestampntz("1970-03-31T00:00:01.123456")));
+  }
+
+  @ParameterizedTest
+  @MethodSource("timestampEqParameters")
+  public void testTimestampEq(
+      String variantType, String literal, VariantValue lowerBound, 
VariantValue upperBound) {
+    // lower bounds
+    Map<Integer, ByteBuffer> lowerBounds =
+        ImmutableMap.of(
+            2, VariantTestUtil.variantBuffer(Map.of("$['event_timestamp']", 
lowerBound)));
+    // upper bounds
+    Map<Integer, ByteBuffer> upperBounds =
+        ImmutableMap.of(
+            2, VariantTestUtil.variantBuffer(Map.of("$['event_timestamp']", 
upperBound)));
+
+    DataFile file =
+        new TestDataFile("file.parquet", Row.of(), 50, null, null, null, 
lowerBounds, upperBounds);
+    Expression expr = equal(extract("variant", "$.event_timestamp", 
variantType), literal);
+    assertThat(shouldRead(expr, file)).as("Should read: many possible 
timestamps" + expr).isTrue();

Review Comment:
   The changes looks good to me. As I checked other tests like testIntegerEq(), 
we have additional ones like below. I think you had that before. Why do we 
remove such check? Seems it's reasonable to have such coverage? 
   
   ```
   assertThat(shouldRead(equal(extract("variant", "$.event_id", "long"), 
INT_MIN_VALUE - 25)))
           .as("Should not read: id below lower bound")
           .isFalse();
   
       assertThat(shouldRead(equal(extract("variant", "$.event_id", "long"), 
INT_MIN_VALUE - 1)))
           .as("Should not read: id below lower bound")
           .isFalse();
   ```



-- 
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]

Reply via email to