You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/02/28 19:04:32 UTC
[GitHub] [iceberg] pvary opened a new pull request #2283: Hive: Quick fix for broken TestHiveIcebergFilterFactory.testTimestampType test
pvary opened a new pull request #2283:
URL: https://github.com/apache/iceberg/pull/2283
#2254 modified how the Timestamp type is handled in `HiveIcebergFilterFactory.microsFromTimestamp` but forgot to update the tests for filter conversion.
The original change (#2254) fixed the Timestamp / Date filters to work not only in UTC timezone but other timezones as well. The change was verified by adding new tests which were testing with multiple timezones, but the original `TestHiveIcebergFilterFactory.testTimestampType` test was checking the base behavior which were originally wrong and fixed in #2254, so we need to change the test too.
----------------------------------------------------------------
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
[GitHub] [iceberg] edgarRd commented on pull request #2283: Hive: Quick fix for broken TestHiveIcebergFilterFactory.testTimestampType test
Posted by GitBox <gi...@apache.org>.
edgarRd commented on pull request #2283:
URL: https://github.com/apache/iceberg/pull/2283#issuecomment-788028555
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!
----------------------------------------------------------------
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
[GitHub] [iceberg] pvary commented on pull request #2283: Hive: Quick fix for broken TestHiveIcebergFilterFactory.testTimestampType test
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2283:
URL: https://github.com/apache/iceberg/pull/2283#issuecomment-787520537
@marton-bod: Thanks for the review!
@shardulm94: Based on the dev list discussion created a quick fix and pushed the change, but I would like to have your review if possible
Thanks,
Peter
----------------------------------------------------------------
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
[GitHub] [iceberg] pvary commented on pull request #2283: Hive: Quick fix for broken TestHiveIcebergFilterFactory.testTimestampType test
Posted by 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
[GitHub] [iceberg] edgarRd commented on pull request #2283: Hive: Quick fix for broken TestHiveIcebergFilterFactory.testTimestampType test
Posted by GitBox <gi...@apache.org>.
edgarRd commented on pull request #2283:
URL: https://github.com/apache/iceberg/pull/2283#issuecomment-788155594
I agree that this test is not particularly interesting. But, moving forward my suggestion goes to suggest how can we avoid test cases to not be detected by CI when ran in different timezones - as was the case with the changes that lead to this test failing.
If you think we're covered with the existing test harness then that should be enough 👍🏽 .
----------------------------------------------------------------
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
[GitHub] [iceberg] pvary merged pull request #2283: Hive: Quick fix for broken TestHiveIcebergFilterFactory.testTimestampType test
Posted by GitBox <gi...@apache.org>.
pvary merged pull request #2283:
URL: https://github.com/apache/iceberg/pull/2283
----------------------------------------------------------------
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