zabetak commented on code in PR #5309:
URL: https://github.com/apache/hive/pull/5309#discussion_r1768445745
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java:
##########
@@ -134,7 +135,7 @@ public static String convertTimestampToString(Timestamp
timestamp) {
*/
public static Timestamp convertStringToTimestamp(String timestamp) {
LocalDateTime val =
LocalDateTime.from(TIMESTAMP_FORMATTER.parse(timestamp));
Review Comment:
Since now we are converting to `Instant` do we need to pass from
`LocalDateTime`? Can we use directly `Instant.from` here?
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java:
##########
@@ -134,7 +135,7 @@ public static String convertTimestampToString(Timestamp
timestamp) {
*/
public static Timestamp convertStringToTimestamp(String timestamp) {
LocalDateTime val =
LocalDateTime.from(TIMESTAMP_FORMATTER.parse(timestamp));
- return Timestamp.valueOf(val);
+ return Timestamp.from(val.toInstant(ZoneOffset.UTC));
Review Comment:
Note that the result of this method (i.e., Timestamp) is not used anywhere
at the moment in production code. The main consumers of the API right now, just
want to ensure that the string input can be parsed without raising an
exception. Except tests nobody cares about what the Timestamp value really is.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreUtils.java:
##########
@@ -92,7 +118,7 @@ public void testStringToDate() {
@Test
public void testStringToTimestamp() {
- assertEquals(Timestamp.valueOf(timestamp),
MetaStoreUtils.convertStringToTimestamp(timestamp));
+ assertEquals(timestamp,
MetaStoreUtils.convertStringToTimestamp(localTimestampString));
Review Comment:
The `convertStringToTimestamp` and `convertTimestampToString` were meant to
be dual. The modifications in the tests indicate that they are not anymore
since we are not using the same input/output for each parameterized run.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreUtils.java:
##########
@@ -67,9 +69,31 @@ public static Collection<Object[]>
generateZoneTimestampPairs() {
params.add(new Object[] { zone, datetime });
}
});
+ // Timestamp and Date do not have the year 0000. So, the year 0000 gets
converted to year 0001.
+ // This is one such test case with year as 0000: 0000-01-07 22:44:36
+ params.add(new Object[] {"Asia/Kolkata",
LocalDateTime.ofEpochSecond(-62166618924L, 0, ZoneOffset.UTC)});
+ generateDaylightSavingTimestampPairs(params);
return params;
}
+ public static void generateDaylightSavingTimestampPairs(List<Object[]>
params) {
+ // For U.S timezones, the timestamp 2024-03-10 02:01:00 falls between the
transition from 02:00 AM to 03:00 AM for daylight savings
+ params.add(new Object[] {"America/Anchorage",
LocalDateTime.ofEpochSecond(1710036060L, 0, ZoneOffset.UTC)});
Review Comment:
Instead of using the `LocalDateTime.ofEpochSecond` constructor you could use
the basic constructor which allows you to specify all fields explicitly. By
doing this the comment above becomes redundant since it is clear from the code
what is the date.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreUtils.java:
##########
@@ -67,9 +69,31 @@ public static Collection<Object[]>
generateZoneTimestampPairs() {
params.add(new Object[] { zone, datetime });
}
});
+ // Timestamp and Date do not have the year 0000. So, the year 0000 gets
converted to year 0001.
+ // This is one such test case with year as 0000: 0000-01-07 22:44:36
+ params.add(new Object[] {"Asia/Kolkata",
LocalDateTime.ofEpochSecond(-62166618924L, 0, ZoneOffset.UTC)});
+ generateDaylightSavingTimestampPairs(params);
return params;
}
+ public static void generateDaylightSavingTimestampPairs(List<Object[]>
params) {
+ // For U.S timezones, the timestamp 2024-03-10 02:01:00 falls between the
transition from 02:00 AM to 03:00 AM for daylight savings
+ params.add(new Object[] {"America/Anchorage",
LocalDateTime.ofEpochSecond(1710036060L, 0, ZoneOffset.UTC)});
+ params.add(new Object[] {"America/St_Johns",
LocalDateTime.ofEpochSecond(1710036060L, 0, ZoneOffset.UTC)});
+ params.add(new Object[] {"America/Chicago",
LocalDateTime.ofEpochSecond(1710036060L, 0, ZoneOffset.UTC)});
+ params.add(new Object[] {"America/Indiana/Indianapolis",
LocalDateTime.ofEpochSecond(1710036060L, 0, ZoneOffset.UTC)});
+ params.add(new Object[] {"America/Los_Angeles",
LocalDateTime.ofEpochSecond(1710036060L, 0, ZoneOffset.UTC)});
Review Comment:
These 5 tests seem kinda identical maybe we just need one of them.
--
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: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
