rdblue commented on code in PR #13195: URL: https://github.com/apache/iceberg/pull/13195#discussion_r2229808504
########## core/src/test/java/org/apache/iceberg/expressions/TestInclusiveMetricsEvaluatorWithExtract.java: ########## @@ -91,7 +165,22 @@ public class TestInclusiveMetricsEvaluatorWithExtract { "$['event_id']", Variants.of(INT_MIN_VALUE), "$['str']", - Variants.of("abc")))), + Variants.of("abc"), + "$['event_uuid']", + Variants.ofUUID(VAR_UUID), + "$['event_timestamp_long']", + Variants.of(TS_MIN_VALUE), + "$['event_timestamp_ts_type']", + Variants.ofIsoTimestamptz(DateTimeUtil.microsToIsoTimestamptz(TS_MIN_VALUE)), + "$['event_timestamp_nano_long']", + Variants.of(TS_NANO_MIN_VALUE), + "$['event_timestamp_nano_ts_type']", + Variants.ofIsoTimestamptzNanos( + DateTimeUtil.nanosToIsoTimestamptz(TS_NANO_MIN_VALUE)), + "$['event_time_long']", + Variants.of(TIME_MIN_VALUE), + "$['event_time_ts_type']", + Variants.ofIsoTime(DateTimeUtil.microsToIsoTime(TIME_MIN_VALUE))))), Review Comment: The tests in this file validate expression types, not data types. To add type tests, I think this only needs to validate that the expression is passed and handled correctly. To do that, I think you should use a parameterized test with a type, a literal, a variant lower bound, and a variant upper bound. The test itself should construct the `DataFile` and then call `shouldRead`. I think it should be a lot simpler than the current changes. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org