apache
HIVE-28337: TestMetaStoreUtils fails for invalid timestamps
What changes were proposed in this pull request?
Timestamp represented as LocalDateTime is changed to ZonedDateTime to take time-zone into account while creating the timestamp to avoid creating invalid timestamps.
Why are the changes needed?
https://issues.apache.org/jira/browse/HIVE-28337
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No
How was this patch tested?
mvn test -Dtest=TestMetaStoreUtils
Before the patch:
with Random(seed: 23) in line 65(default case)
with Random(seed: 24) in line 65
After the patch:
Nice catch~ This failed test was also found in https://github.com/apache/hive/pull/5004 and we can revert https://github.com/kgyrtkirk/hive-dev-box/pull/16 once this patch is merged. @zabetak could you also take a look?
The 2 test cases mentioned in https://issues.apache.org/jira/browse/HIVE-28337 are failing for me locally in my machine with Java version 1.8.0_331. I tried running these test cases in https://github.com/apache/hive/pull/5004, but the DST related one was successful. I was investigating as to why this was happening, and now I get it seeing the previous comments that this is occurring due to JDK bug.
The intention of the test is to ensure that conversions from/to string do not alter the value no matter the timezone because the DATE/TIMESTAMP data types should be timezone agnostic.
Changing LocalDateTime to ZonedDateTime defeats the purpose of the tests and actually hides a bug that affects the MetaStoreUtils APIs.
I left a similar comment under the JIRA ticket.
Yes, I agree with you regarding the intention and purpose of the test. I am looking into the issue and would re-work this PR taking the MetaStoreUtils APIs into account.
I have updated the tests and the MetaStoreUtils APIs to always have Timestamp created in UTC timezone, since the timestamps during the daylight savings transition could not be represented in the local time zone.
However, there are 2 known issues for the timestamps mentioned below but it is the expected behaviour from Java's Timestamp:
- Timestamps with year 0000 would be converted to year 0001.
- Timestamps in the range [1582-10-05 00:00:00, 1582-10-14 23:59:59] (both inclusive) would have 10 days added due to the shift from Julian calendar to Gregorian calendar. (The next day after 1582-10-04 is 1582-10-15)
References: [1] https://www.ibm.com/docs/en/db2/11.5?topic=dttmddtija-date-time-timestamp-values-that-can-cause-problems-in-jdbc-sqlj-applications#imjcc_r0053436__title__5 [2] https://strasbourg.eleusal.com/en/what-happened-between-5-and-14-october-1582/
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarCloud
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
I understand that due to bugs the tests may fail in certain environments.
If the main motivation of this PR is to avoid the failures when this happens it seems more appropriate to add conditions for detecting skipping the specific test with appropriate bug reference.
If the intention is to fix the problems revealed by the tests then more work is needed since the proposed changes cannot land as they are.
The initial motivation was to fix the test failures, which later changed to solve the problem revealed in MetaStoreUtils. Upon your comments, I have reworked my PR which now solves the problem in MetaStoreUtils and have accordingly modified TestMetaStoreUtils to validate the tests properly.
@zabetak @wecharyu Please review. Thank you.
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarCloud