[GitHub] [iceberg] pvary commented on pull request #2283: Hive: Quick fix for broken TestHiveIcebergFilterFactory.testTimestampType test

[GitBox <gi...@apache.org>]

pvary commented on pull request #2283:
URL: https://github.com/apache/iceberg/pull/2283#issuecomment-788078056


   > On these tests that check timezone sensitive types, and with the new changes in #2278 should we check with a few timezones to validate something like in https://github.com/apache/iceberg/blob/master/orc/src/test/java/org/apache/iceberg/orc/TestExpressionToSearchArgument.java#L120 - Thanks!
   
   I am not sure I understand your suggestion correctly, so please correct me if I missed something.
   
   - We have end-to-end tests in `TestHiveIcebergStorageHandlerTimezone`. There we test the `date`/`timestamp`/`timestamp withzone` - same as in TestExpressionToSearchArgument. Might be useful for add tests with all of the `FileFormat`-s, but my feeling is that it should be tested on the individual FileFormats, and not here.
   - We test the individual conversion in the `TestHiveIcebergFilterFactory`. I am not fan of this test as it is basically inverting the `microsFromTimestamp(Timestamp timestamp)` method, and only testing if `timestamp.toLocalDateTime()` is working correctly or not 😄 


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org