You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "simhadri-g (via GitHub)" <gi...@apache.org> on 2024/05/07 10:19:53 UTC
[PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
simhadri-g opened a new pull request, #5241:
URL: https://github.com/apache/hive/pull/5241
…29th feb and fails with not a leap year exception
Example: Invalid date 'February 29' as '200' is not a leap year
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
When handling legacy time stamp conversions in parquet,'February 29' year '200' is an edge case.
This is because, according to this: https://www.lanl.gov/Caesar/node202.html
The Julian day for 200 CE/02/29 in the Julian calendar is different from the Julian day in Gregorian Calendar .
Date (BC/AD) | Date (CE) | Julian Day | Julian Day
------------- | ---------- | -----------------|----------
| - | - | (Julian Calendar)| (Gregorian Calendar) |
200 AD/02/28 | 200 CE/02/28 | 1794166 | 1794167
200 AD/02/29 | 200 CE/02/29 | 1794167 | 1794168
200 AD/03/01 | 200 CE/03/01 | 1794168 | 1794168
300 AD/02/28 | 300 CE/02/28 | 1830691 | 1830691
300 AD/02/29 | 300 CE/02/29 | 1830692 | 1830692
300 AD/03/01 | 300 CE/03/01 | 1830693 | 1830692
As a result since hive stores timestamp in UTC, when converting 200 CE/03/01 between timezones, hive runs into an exception and fails with "not a leap year exception" for 29th Feb 200 even if the actual record inserted was 200 CE/03/01 in Asia/Singapore timezone.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Even if the actual record inserted was 200 CE/03/01 in Asia/Singapore timezone hive runs into an exception and fails with "not a leap year exception" for 29th Feb 200 when reading the file.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### Is the change a dependency upgrade?
<!--
If yes, please attach a file with output from mvn dependency:tree to validate a complete upgrade of dependency.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Qtest and manual test
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995099
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
@simhadri-g , how about this
````
JulianDate jDateTime = JulianDate.of((double) julianDay);
LocalDateTime localDateTime;
int leapYearAdjustment = 0;
try {
localDateTime = jDateTime.toLocalDateTime();
} catch (DateTimeException e) {
if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
localDateTime = jDateTime.add(++leapYearAdjustment).toLocalDateTime();
} else {
throw e;
}
}
Calendar calendar = getGMTCalendar();
calendar.set(Calendar.YEAR, localDateTime.getYear());
calendar.set(Calendar.MONTH, localDateTime.getMonthValue() - 1); //java calendar index starting at 1.
calendar.set(Calendar.DAY_OF_MONTH, localDateTime.getDayOfMonth());
int hour = (int) (remainder / (NANOS_PER_HOUR));
remainder = remainder % (NANOS_PER_HOUR);
int minutes = (int) (remainder / (NANOS_PER_MINUTE));
remainder = remainder % (NANOS_PER_MINUTE);
int seconds = (int) (remainder / (NANOS_PER_SECOND));
long nanos = remainder % NANOS_PER_SECOND;
calendar.set(Calendar.HOUR_OF_DAY, hour);
calendar.set(Calendar.MINUTE, minutes);
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
.minusDays(leapYearAdjustment);
````
Timestamp
````
public Timestamp minusDays(long days) {
localDateTime = localDateTime.minusDays(days);
return this;
}
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1596434754
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ localDateTime = LocalDateTime.ofEpochSecond(jDateTime.toMilliseconds() / 1000,
+ (int) jDateTime.toMilliseconds() % 1000 * 1000000, ZoneOffset.UTC);
+ if (!localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth().name().equals("FEBRUARY") &&
Review Comment:
````
notLeapYear = !localDateTime.toLocalDate().isLeapYear()
&& localDateTime.getMonth() == Month.FEBRUARY
&& legacyConversion
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595464852
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
We need to use the legacyDateFormatter because of HIVE-24074: Incorrect handling of timestamp in Parquet/Avro when written in certain time zones in versions before Hive 3.x . Only with this can we get the correct timezone conversion for these dates.
So formatDate(ts, fromZone, toZone) uses legacyDateFormatter and returns `0200-02-29 00:00:00`. If we don't use the legacyDateFormatter we get `0200-02-28 22:55:25`which is incorrect.
Once we have `0200-02-29 00:00:00` we need to parse it with DateTimeFormatter with ResolverStyle.LENIENT , this converts 0200-02-29 00:00:00 to the correct value `0200-03-01 00:00:00.0 ` .
We need to format this again to return the timestamp.
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
We need to use the legacyDateFormatter because of HIVE-24074: Incorrect handling of timestamp in Parquet/Avro when written in certain time zones in versions before Hive 3.x . Only with this can we get the correct timezone conversion for these dates.
So formatDate(ts, fromZone, toZone) uses legacyDateFormatter and returns `0200-02-29 00:00:00`. If we don't use the legacyDateFormatter we get `0200-02-28 22:55:25`which is incorrect.
Once we have `0200-02-29 00:00:00` we need to parse it with DateTimeFormatter with ResolverStyle.LENIENT , this converts 0200-02-29 00:00:00 to the correct value `0200-03-01 00:00:00.0 ` .
Lastly, We need to format this again to return the timestamp.
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595205228
##########
common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java:
##########
@@ -529,6 +529,7 @@ public enum ErrorMsg {
PARQUET_FOOTER_ERROR(20022, "Failed to read parquet footer:"),
PARQUET_UNHANDLED_TYPE(20023, "Unhandled type {0}", true),
ORC_FOOTER_ERROR(20024, "Failed to read orc footer:"),
+ NOT_A_LEAP_YEAR(20025, "not a leap year", true),
Review Comment:
`NOT_LEAP_YEAR`
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2107033350
In the new approach, the julian day is increased by 1 for leap day and process the date.
Once we have the result, we decrement it by 1 day to get back the correct output.
This works for leap day for all century years
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995099
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
@simhadri-g , how about this
````
JulianDate jDateTime = JulianDate.of((double) julianDay);
LocalDateTime localDateTime;
int leapYearAdjustment = 0;
try {
localDateTime = jDateTime.toLocalDateTime();
} catch (DateTimeException e) {
if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
localDateTime = jDateTime.add(++leapYearDateAdjustment).toLocalDateTime();
} else {
throw e;
}
}
.....
.....
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
.minusDays(leapYearAdjustment);
````
Timestamp
````
public Timestamp minusDays(long days) {
localDateTime = localDateTime.minusDays(days);
return this;
}
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600042371
##########
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestParquetTimestampsHive2Compatibility.java:
##########
@@ -97,6 +97,21 @@ void testWriteHive2ReadHive4UsingLegacyConversionWithZone(String timestampString
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("generateJulianLeapYearTimestamps")
+ void testWriteHive2ReadHive4UsingLegacyConversionWithJulianLeapYears(String timestampString) {
+ TimeZone original = TimeZone.getDefault();
+ try {
+ String zoneId = "Asia/Singapore";
Review Comment:
please check edge cases: UTC+12 write UTC-12 read, and reversed
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995099
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
@simhadri-g , how about this
````
JulianDate jDateTime = JulianDate.of((double) julianDay);
LocalDateTime localDateTime;
int leapYearAdjustment = 0;
try {
localDateTime = jDateTime.toLocalDateTime();
} catch (DateTimeException e) {
if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
leapYearDateAdjustment = -2;
localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
} else {
throw e;
}
}
.....
.....
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
.minusDays(leapYearAdjustment);
````
Timestamp
````
public Timestamp minusDays(long days) {
localDateTime = localDateTime.minusDays(days);
return this;
}
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1601368826
##########
common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java:
##########
@@ -529,6 +529,7 @@ public enum ErrorMsg {
PARQUET_FOOTER_ERROR(20022, "Failed to read parquet footer:"),
PARQUET_UNHANDLED_TYPE(20023, "Unhandled type {0}", true),
ORC_FOOTER_ERROR(20024, "Failed to read orc footer:"),
+ NOT_LEAP_YEAR(20025, "not a leap year", true),
Review Comment:
I have the impression that this class holds messages that are targeting the end-user. I am not sure if it's appropriate for the use case here since we are not throwing this error anywhere.
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595485775
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
Review Comment:
Not exactly, we don't need to have this check for every leap year.
The Julian calendar defines a leap year as once every four years. The Gregorian calendar modified the addition of leap days, such that a century year was only counted as a leap year if it was also divisible by 400.
So according to julian calender the year 200 or 300 is a leap year but they are not a leap year according to according to Gregorian calendar. That is why we are seeing this issue, this is an edge case.
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1599717428
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -91,6 +94,7 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone) {
public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean legacyConversion) {
int julianDay = nt.getJulianDay();
long nanosOfDay = nt.getTimeOfDayNanos();
+ boolean notLeapYear = false;
Review Comment:
remove it, it's not used
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1599719186
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -100,12 +104,24 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
julianDay--;
}
- JulianDate jDateTime;
- jDateTime = JulianDate.of((double) julianDay);
+ JulianDate jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ int leapYearDateAdjustment = 0;
+
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
+ leapYearDateAdjustment = 1;
+ localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
Review Comment:
````
localDateTime = jDateTime.add(++leapYearDateAdjustment).toLocalDateTime();
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2109836266
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995099
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
@simhadri-g , how about this
````
JulianDate jDateTime = JulianDate.of((double) julianDay);
LocalDateTime localDateTime;
int leapYearAdjustment = 0;
try {
localDateTime = jDateTime.toLocalDateTime();
} catch (DateTimeException e) {
if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
leapYearDateAdjustment = 2;
localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
} else {
throw e;
}
}
Calendar calendar = getGMTCalendar();
calendar.set(Calendar.YEAR, localDateTime.getYear());
calendar.set(Calendar.MONTH, localDateTime.getMonthValue() - 1); //java calendar index starting at 1.
calendar.set(Calendar.DAY_OF_MONTH, localDateTime.getDayOfMonth());
int hour = (int) (remainder / (NANOS_PER_HOUR));
remainder = remainder % (NANOS_PER_HOUR);
int minutes = (int) (remainder / (NANOS_PER_MINUTE));
remainder = remainder % (NANOS_PER_MINUTE);
int seconds = (int) (remainder / (NANOS_PER_SECOND));
long nanos = remainder % NANOS_PER_SECOND;
calendar.set(Calendar.HOUR_OF_DAY, hour);
calendar.set(Calendar.MINUTE, minutes);
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
.minusDays(leapYearAdjustment);
````
Timestamp
````
public Timestamp minusDays(long days) {
localDateTime = localDateTime.minusDays(days);
return this;
}
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1596434754
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ localDateTime = LocalDateTime.ofEpochSecond(jDateTime.toMilliseconds() / 1000,
+ (int) jDateTime.toMilliseconds() % 1000 * 1000000, ZoneOffset.UTC);
+ if (!localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth().name().equals("FEBRUARY") &&
Review Comment:
localDateTime.getMonth() == Month.FEBRUARY
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1596434754
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ localDateTime = LocalDateTime.ofEpochSecond(jDateTime.toMilliseconds() / 1000,
+ (int) jDateTime.toMilliseconds() % 1000 * 1000000, ZoneOffset.UTC);
+ if (!localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth().name().equals("FEBRUARY") &&
Review Comment:
`localDateTime.getMonth() == Month.FEBRUARY`
or maybe `localDateTime.getDayOfMonth = 29` ?
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995099
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
@simhadri-g , how about this
````
JulianDate jDateTime = JulianDate.of((double) julianDay);
LocalDateTime localDateTime;
int leapYearAdjustment = 0;
try {
localDateTime = jDateTime.toLocalDateTime();
} catch (DateTimeException e) {
if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
leapYearDateAdjustment = OffsetDateTime.now(targetZone).getOffset()
.compareTo(ZoneOffset.UTC) > 0 ? -1 : 1;
localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
} else {
throw e;
}
}
Calendar calendar = getGMTCalendar();
calendar.set(Calendar.YEAR, localDateTime.getYear());
calendar.set(Calendar.MONTH, localDateTime.getMonthValue() - 1); //java calendar index starting at 1.
calendar.set(Calendar.DAY_OF_MONTH, localDateTime.getDayOfMonth());
int hour = (int) (remainder / (NANOS_PER_HOUR));
remainder = remainder % (NANOS_PER_HOUR);
int minutes = (int) (remainder / (NANOS_PER_MINUTE));
remainder = remainder % (NANOS_PER_MINUTE);
int seconds = (int) (remainder / (NANOS_PER_SECOND));
long nanos = remainder % NANOS_PER_SECOND;
calendar.set(Calendar.HOUR_OF_DAY, hour);
calendar.set(Calendar.MINUTE, minutes);
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
.minusDays(leapYearAdjustment);
````
Timestamp
````
public Timestamp minusDays(long days) {
localDateTime = localDateTime.minusDays(days);
return this;
}
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1602013939
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -193,7 +196,18 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
java.util.Date date = formatter.parse(ts.format(TIMESTAMP_FORMATTER));
// Set the formatter to use a different timezone
formatter.setTimeZone(TimeZone.getTimeZone(toZone));
- Timestamp result = Timestamp.valueOf(formatter.format(date));
+ Timestamp result;
+ try {
+ result = Timestamp.valueOf(formatter.format(date));
+ } catch (IllegalArgumentException ex) {
+ if (ex.getCause().getMessage().contains(NOT_LEAP_YEAR)) {
+ int leapYearDateAdjustment = ts.getMonth() == Month.MARCH.getValue() ? 2 : -2;
Review Comment:
1 and -1 here are ok
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600228031
##########
ql/src/test/queries/clientpositive/parquet_historical_timestamp.q:
##########
@@ -1,3 +1,4 @@
+--! qt:timezone:Asia/Singapore
Review Comment:
can't we move timezone setup just before your test case?
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2102939033
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2103668396
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595208801
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
Review Comment:
can we use `localDateTime.isLeapYear()` check?
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595252070
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
why do you need to parse and format twice?
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2106059859
After adding more test cases, I found some issue with this conversion for other century years. Updating PR with a different approach to address 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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1596434754
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ localDateTime = LocalDateTime.ofEpochSecond(jDateTime.toMilliseconds() / 1000,
+ (int) jDateTime.toMilliseconds() % 1000 * 1000000, ZoneOffset.UTC);
+ if (!localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth().name().equals("FEBRUARY") &&
Review Comment:
````
notLeapYear = !localDateTime.toLocalDate().isLeapYear()
&& localDateTime.getMonth() == Month.FEBRUARY
&& localDateTime.getDayOfMonth() == 29
&& legacyConversion
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1599930408
##########
ql/src/test/queries/clientpositive/timestamp_julian.q:
##########
@@ -0,0 +1,22 @@
+--! qt:timezone:Asia/Singapore
+
+-- Test 1
+-- insert into default.test_sgt700 values ('0200-03-01 00:00:00')
+CREATE EXTERNAL TABLE `TEST_SGT`(`currtime` timestamp) ROW FORMAT SERDE 'org.apache.hadoop.hive.ql.io.parquet.serde.ParquetHiveSerDe' STORED AS
Review Comment:
Done, I have added unit test that covers testing 1st march across 3000 years .
Also, can we please keep both q test and unit test ?
The datafiles for q tests are only 400 bytes each.
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600020249
##########
ql/src/test/queries/clientpositive/timestamp_julian.q:
##########
@@ -0,0 +1,22 @@
+--! qt:timezone:Asia/Singapore
+
+-- Test 1
+-- insert into default.test_sgt700 values ('0200-03-01 00:00:00')
+CREATE EXTERNAL TABLE `TEST_SGT`(`currtime` timestamp) ROW FORMAT SERDE 'org.apache.hadoop.hive.ql.io.parquet.serde.ParquetHiveSerDe' STORED AS
Review Comment:
we already have some `parquet_historical_timestamp_legacy.parq`, maybe you can set some meaningful names in new files and add your test cases to `parquet_historical_timestamp.q`
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1601860868
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -50,6 +50,8 @@
import static java.time.temporal.ChronoField.MONTH_OF_YEAR;
import static java.time.temporal.ChronoField.SECOND_OF_MINUTE;
import static java.time.temporal.ChronoField.YEAR;
+import static org.apache.commons.lang3.time.DateUtils.addDays;
Review Comment:
don't use static import here
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2111036496
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1596434754
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ localDateTime = LocalDateTime.ofEpochSecond(jDateTime.toMilliseconds() / 1000,
+ (int) jDateTime.toMilliseconds() % 1000 * 1000000, ZoneOffset.UTC);
+ if (!localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth().name().equals("FEBRUARY") &&
Review Comment:
````
notLeapYear = !localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth() == Month.FEBRUARY && localDateTime.getDayOfMonth() == 29 &&
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600140883
##########
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestParquetTimestampsHive2Compatibility.java:
##########
@@ -97,6 +97,21 @@ void testWriteHive2ReadHive4UsingLegacyConversionWithZone(String timestampString
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("generateJulianLeapYearTimestamps")
+ void testWriteHive2ReadHive4UsingLegacyConversionWithJulianLeapYears(String timestampString) {
+ TimeZone original = TimeZone.getDefault();
+ try {
+ String zoneId = "Asia/Singapore";
Review Comment:
Done
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1599819198
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
Done
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600042371
##########
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestParquetTimestampsHive2Compatibility.java:
##########
@@ -97,6 +97,21 @@ void testWriteHive2ReadHive4UsingLegacyConversionWithZone(String timestampString
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("generateJulianLeapYearTimestamps")
+ void testWriteHive2ReadHive4UsingLegacyConversionWithJulianLeapYears(String timestampString) {
+ TimeZone original = TimeZone.getDefault();
+ try {
+ String zoneId = "Asia/Singapore";
Review Comment:
please check edge cases: [UTC+13](https://en.wikipedia.org/wiki/UTC+13:00) write UTC-12 read, and reversed
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1601448040
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -119,7 +128,44 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
- ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion);
+ ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
+ .minusDays(leapYearDateAdjustment);
+
return ts;
}
+
+ private static int julianLeapYearAdjustment(JulianDate jDateTime) {
+ Set<Integer> validDaysOfMonth = new HashSet<>();
+ validDaysOfMonth.add(1);
+ validDaysOfMonth.add(2);
+ validDaysOfMonth.add(27);
+ validDaysOfMonth.add(28);
+ validDaysOfMonth.add(29);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg())) {
+ return 2;
+ } else {
+ throw e;
+ }
+ }
+
+ int year = localDateTime.getYear();
+ int dayOfMonth = localDateTime.getDayOfMonth();
+ boolean isJulianLeapYear = (year % 4 == 0 && year % 400 != 0 && year % 100 == 0);
+
+ if (year < 1582 && isJulianLeapYear && validDaysOfMonth.contains(dayOfMonth)) {
+ switch (localDateTime.getMonth()) {
+ case FEBRUARY:
+ return -2;
+ case MARCH:
+ return 2;
+ default:
+ return 0;
+ }
+ }
+ return 0;
+ }
Review Comment:
Yes, this is a jodd time issue cause by this change in jodd time: https://github.com/oblac/jodd/commit/3d9772d8be3a974bd49eef29c26c086bbb0aad1a
In the jodd change mentioned above , they have moved from jodd.datetime.JDateTime to java.time.LocalDateTime and LocalDateTime doesn't gracefully handle julian leap year dates such as 200-02-29 .
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1601448536
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -100,12 +107,14 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
julianDay--;
}
- JulianDate jDateTime;
- jDateTime = JulianDate.of((double) julianDay);
+ JulianDate jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ int leapYearDateAdjustment = legacyConversion ? julianLeapYearAdjustment(jDateTime) : 0;
+ localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
Calendar calendar = getGMTCalendar();
- calendar.set(Calendar.YEAR, jDateTime.toLocalDateTime().getYear());
Review Comment:
Hi @zabetak ,
Thanks for the review.
The entire issue starts from Removal of JDateTime here in jodd v4.3 https://github.com/oblac/jodd/commit/3d9772d8be3a974bd49eef29c26c086bbb0aad1a
In the change , they have moved from jodd.datetime.JDateTime to java.time.LocalDateTime and LocalDateTime doesn't gracefully handle julian leap year dates such as 200-02-29
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995099
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
@simhadri-g , how about this
````
JulianDate jDateTime = JulianDate.of((double) julianDay);
LocalDateTime localDateTime;
int leapYearAdjustment = 0;
try {
localDateTime = jDateTime.toLocalDateTime();
} catch (DateTimeException e) {
if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
leapYearDateAdjustment = Integer.signum(ZoneOffset.UTC.compareTo(
OffsetDateTime.now(targetZone).getOffset()));
localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
} else {
throw e;
}
}
Calendar calendar = getGMTCalendar();
calendar.set(Calendar.YEAR, localDateTime.getYear());
calendar.set(Calendar.MONTH, localDateTime.getMonthValue() - 1); //java calendar index starting at 1.
calendar.set(Calendar.DAY_OF_MONTH, localDateTime.getDayOfMonth());
int hour = (int) (remainder / (NANOS_PER_HOUR));
remainder = remainder % (NANOS_PER_HOUR);
int minutes = (int) (remainder / (NANOS_PER_MINUTE));
remainder = remainder % (NANOS_PER_MINUTE);
int seconds = (int) (remainder / (NANOS_PER_SECOND));
long nanos = remainder % NANOS_PER_SECOND;
calendar.set(Calendar.HOUR_OF_DAY, hour);
calendar.set(Calendar.MINUTE, minutes);
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
.minusDays(leapYearAdjustment);
````
Timestamp
````
public Timestamp minusDays(long days) {
localDateTime = localDateTime.minusDays(days);
return this;
}
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2101704025
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600875740
##########
ql/src/test/queries/clientpositive/parquet_historical_timestamp.q:
##########
@@ -1,3 +1,4 @@
+--! qt:timezone:Asia/Singapore
Review Comment:
No, it doesn't matter where it is. It affects the entire q file.
Also, i checked the changed output , the intent of the test remains the same.
The test was to check the reader output for the timestamp written in rome time zone .
According to legacy conversion, the timestamp is converted to the reader's timezone.
As a result , the output has moved ahead to 2019-01-01 08:30:30.111111111 which is the correct value.
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995099
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
@simhadri-g , how about this
````
JulianDate jDateTime = JulianDate.of((double) julianDay);
LocalDateTime localDateTime;
int leapYearAdjustment = 0;
try {
localDateTime = jDateTime.toLocalDateTime();
} catch (DateTimeException e) {
if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
leapYearDateAdjustment = 1;
localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
} else {
throw e;
}
}
.....
.....
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
.minusDays(leapYearAdjustment);
````
Timestamp
````
public Timestamp minusDays(long days) {
localDateTime = localDateTime.minusDays(days);
return this;
}
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1601448536
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -100,12 +107,14 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
julianDay--;
}
- JulianDate jDateTime;
- jDateTime = JulianDate.of((double) julianDay);
+ JulianDate jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ int leapYearDateAdjustment = legacyConversion ? julianLeapYearAdjustment(jDateTime) : 0;
+ localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
Calendar calendar = getGMTCalendar();
- calendar.set(Calendar.YEAR, jDateTime.toLocalDateTime().getYear());
Review Comment:
Hi @zabetak ,
Thanks for the review.
The entire issue starts from Removal of JDateTime here in jodd v4.3 https://github.com/oblac/jodd/commit/3d9772d8be3a974bd49eef29c26c086bbb0aad1a
In the change , they have moved from jodd.datetime.JDateTime to java.time.LocalDateTime and LocalDateTime doesn't support julian leap year dates such as 200-02-29
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2106662309
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595544908
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
omg, got it
should we rename the 'formatDate' to `formatLegacyDate`?
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595470239
##########
common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java:
##########
@@ -529,6 +529,7 @@ public enum ErrorMsg {
PARQUET_FOOTER_ERROR(20022, "Failed to read parquet footer:"),
PARQUET_UNHANDLED_TYPE(20023, "Unhandled type {0}", true),
ORC_FOOTER_ERROR(20024, "Failed to read orc footer:"),
+ NOT_A_LEAP_YEAR(20025, "not a leap year", true),
Review Comment:
Thanks @deniskuzZ for the review ! :)
Fixed
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -119,7 +135,11 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
- ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion);
+ if(notLeapYear){
Review Comment:
Fixed
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
+ result.setNanos(ts.getNanos());
+ return result;
+ }
+
+ private static String formatDate(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateFormat formatter = getLegacyDateFormatter();
+ formatter.setTimeZone(TimeZone.getTimeZone(fromZone));
+ java.util.Date date = null;
Review Comment:
Fixed
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2104738484
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g merged PR #5241:
URL: https://github.com/apache/hive/pull/5241
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995099
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
@simhadri-g , how about this
````
JulianDate jDateTime = JulianDate.of((double) julianDay);
LocalDateTime localDateTime;
int leapYearAdjustment = 0;
try {
localDateTime = jDateTime.toLocalDateTime();
} catch (DateTimeException e) {
if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
leapYearDateAdjustment = -2;
localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
} else {
throw e;
}
}
Calendar calendar = getGMTCalendar();
calendar.set(Calendar.YEAR, localDateTime.getYear());
calendar.set(Calendar.MONTH, localDateTime.getMonthValue() - 1); //java calendar index starting at 1.
calendar.set(Calendar.DAY_OF_MONTH, localDateTime.getDayOfMonth());
int hour = (int) (remainder / (NANOS_PER_HOUR));
remainder = remainder % (NANOS_PER_HOUR);
int minutes = (int) (remainder / (NANOS_PER_MINUTE));
remainder = remainder % (NANOS_PER_MINUTE);
int seconds = (int) (remainder / (NANOS_PER_SECOND));
long nanos = remainder % NANOS_PER_SECOND;
calendar.set(Calendar.HOUR_OF_DAY, hour);
calendar.set(Calendar.MINUTE, minutes);
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
.minusDays(leapYearAdjustment);
````
Timestamp
````
public Timestamp minusDays(long days) {
localDateTime = localDateTime.minusDays(days);
return this;
}
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600042371
##########
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestParquetTimestampsHive2Compatibility.java:
##########
@@ -97,6 +97,21 @@ void testWriteHive2ReadHive4UsingLegacyConversionWithZone(String timestampString
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("generateJulianLeapYearTimestamps")
+ void testWriteHive2ReadHive4UsingLegacyConversionWithJulianLeapYears(String timestampString) {
+ TimeZone original = TimeZone.getDefault();
+ try {
+ String zoneId = "Asia/Singapore";
Review Comment:
please check edge cases: [UTC+14](https://en.wikipedia.org/wiki/UTC+14:00) write UTC-12 read, and reversed
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995583
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
everything else could be reverted
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2112003254
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1598995099
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
Review Comment:
@simhadri-g , how about this
````
JulianDate jDateTime = JulianDate.of((double) julianDay);
LocalDateTime localDateTime;
int leapYearAdjustment = 0;
try {
localDateTime = jDateTime.toLocalDateTime();
} catch (DateTimeException e) {
if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
leapYearDateAdjustment = OffsetDateTime.now(targetZone).getOffset()
.compareTo(ZoneOffset.UTC) > 0 ? -1 : 1;
localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
} else {
throw e;
}
}
Calendar calendar = getGMTCalendar();
calendar.set(Calendar.YEAR, localDateTime.getYear());
calendar.set(Calendar.MONTH, localDateTime.getMonthValue() - 1); //java calendar index starting at 1.
calendar.set(Calendar.DAY_OF_MONTH, localDateTime.getDayOfMonth());
int hour = (int) (remainder / (NANOS_PER_HOUR));
remainder = remainder % (NANOS_PER_HOUR);
int minutes = (int) (remainder / (NANOS_PER_MINUTE));
remainder = remainder % (NANOS_PER_MINUTE);
int seconds = (int) (remainder / (NANOS_PER_SECOND));
long nanos = remainder % NANOS_PER_SECOND;
calendar.set(Calendar.HOUR_OF_DAY, hour);
calendar.set(Calendar.MINUTE, minutes);
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
.minusDays(leapYearAdjustment);
````
Timestamp
````
public Timestamp minusDays(long days) {
localDateTime = localDateTime.minusDays(days);
return this;
}
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595231241
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -119,7 +135,11 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
- ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion);
+ if(notLeapYear){
Review Comment:
nit: space
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595252070
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
why do you need to `parse` and `format` twice?
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595544908
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
omg, got it
should we rename the 'formatDate' to `formatLegacyDate`?
also could we move `parse` to the next line
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1602718666
##########
common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java:
##########
@@ -529,6 +529,7 @@ public enum ErrorMsg {
PARQUET_FOOTER_ERROR(20022, "Failed to read parquet footer:"),
PARQUET_UNHANDLED_TYPE(20023, "Unhandled type {0}", true),
ORC_FOOTER_ERROR(20024, "Failed to read orc footer:"),
+ NOT_LEAP_YEAR(20025, "not a leap year", true),
Review Comment:
moved the message out of ErrorMsg. Fixed.
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2098151997
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595261315
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
+ result.setNanos(ts.getNanos());
+ return result;
+ }
+
+ private static String formatDate(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateFormat formatter = getLegacyDateFormatter();
+ formatter.setTimeZone(TimeZone.getTimeZone(fromZone));
+ java.util.Date date = null;
Review Comment:
null initialization is redundant
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595232892
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
+ result.setNanos(ts.getNanos());
+ return result;
+ }
+
+ private static String formatDate(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
Review Comment:
what's the purpose of this method if it's used just in `convertTimestampToZone`
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595232892
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
+ result.setNanos(ts.getNanos());
+ return result;
+ }
+
+ private static String formatDate(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
Review Comment:
what's the purpose of this method if it's used just in `convertTimestampToZone`
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595464852
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
We need to use the legacyDateFormatter because of HIVE-24074: Incorrect handling of timestamp in Parquet/Avro when written in certain time zones in versions before Hive 3.x . Only with this can we get the correct timezone conversion for these dates.
So formatDate(ts, fromZone, toZone) uses legacyDateFormatter and returns `0200-02-29 00:00:00`. If we don't use the legacyDateFormatter we get `0200-02-28 22:55:25`which is incorrect.
Once we have `0200-02-29 00:00:00` we need to parse it with DateTimeFormatter with ResolverStyle.LENIENT , this converts 0200-02-29 00:00:00 to the correct value `0200-03-01 00:00:00.0 ` which was the value .
We need to format this again to return the timestamp.
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600042371
##########
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestParquetTimestampsHive2Compatibility.java:
##########
@@ -97,6 +97,21 @@ void testWriteHive2ReadHive4UsingLegacyConversionWithZone(String timestampString
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("generateJulianLeapYearTimestamps")
+ void testWriteHive2ReadHive4UsingLegacyConversionWithJulianLeapYears(String timestampString) {
+ TimeZone original = TimeZone.getDefault();
+ try {
+ String zoneId = "Asia/Singapore";
Review Comment:
please check edge cases: [UTC+14](https://en.wikipedia.org/wiki/UTC+14:00) write & UTC-12 read, and reversed
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1601339480
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -119,7 +128,44 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
- ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion);
+ ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
+ .minusDays(leapYearDateAdjustment);
+
return ts;
}
+
+ private static int julianLeapYearAdjustment(JulianDate jDateTime) {
+ Set<Integer> validDaysOfMonth = new HashSet<>();
+ validDaysOfMonth.add(1);
+ validDaysOfMonth.add(2);
+ validDaysOfMonth.add(27);
+ validDaysOfMonth.add(28);
+ validDaysOfMonth.add(29);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg())) {
+ return 2;
+ } else {
+ throw e;
+ }
+ }
+
+ int year = localDateTime.getYear();
+ int dayOfMonth = localDateTime.getDayOfMonth();
+ boolean isJulianLeapYear = (year % 4 == 0 && year % 400 != 0 && year % 100 == 0);
+
+ if (year < 1582 && isJulianLeapYear && validDaysOfMonth.contains(dayOfMonth)) {
+ switch (localDateTime.getMonth()) {
+ case FEBRUARY:
+ return -2;
+ case MARCH:
+ return 2;
+ default:
+ return 0;
+ }
+ }
+ return 0;
+ }
Review Comment:
This logic is complex and pretty expensive in terms of performance for something that will be done for every single timestamp value (especially the try catch block).
According to the simple test below the problematic Julian days seem to be rather few and they are known in advance:
```java
int start = JulianDate.of(LocalDate.of(0, 1, 1)).getInteger();
int end = JulianDate.of(LocalDate.of(9999, 12, 31)).getInteger();
System.out.println("Start Julian day " + start);
System.out.println("End Julian day " + end);
for (int i = start; i <= end; i++) {
JulianDate date = new JulianDate(i, 0.0);
try {
date.toLocalDateTime();
} catch (Exception e) {
System.out.println("Problem converting Julian day " + i);
}
}
```
```
Start Julian day 1721057
End Julian day 5373483
Problem converting Julian day 1757642
Problem converting Julian day 1794167
Problem converting Julian day 1830692
Problem converting Julian day 1903742
Problem converting Julian day 1940267
Problem converting Julian day 1976792
Problem converting Julian day 2049842
Problem converting Julian day 2086367
Problem converting Julian day 2122892
Problem converting Julian day 2195942
Problem converting Julian day 2232467
Problem converting Julian day 2268992
```
Wouldn't be easier and more efficient to do the shifting based on the 12 dates printed above? Am I missing more problematic dates?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -100,12 +107,14 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
julianDay--;
}
- JulianDate jDateTime;
- jDateTime = JulianDate.of((double) julianDay);
+ JulianDate jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ int leapYearDateAdjustment = legacyConversion ? julianLeapYearAdjustment(jDateTime) : 0;
+ localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
Calendar calendar = getGMTCalendar();
- calendar.set(Calendar.YEAR, jDateTime.toLocalDateTime().getYear());
Review Comment:
The fact that the exception originates from `JulianDate` class makes me believe that this is a bug/problem to be addressed in the `jodd` library. I also think that this exception is mainly a result of HIVE-25054 which upgraded the jodd library.
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -100,12 +107,14 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
julianDay--;
}
- JulianDate jDateTime;
- jDateTime = JulianDate.of((double) julianDay);
+ JulianDate jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ int leapYearDateAdjustment = legacyConversion ? julianLeapYearAdjustment(jDateTime) : 0;
Review Comment:
Are we sure that this leap year problem affects only the `legacyConversion`? What is preventing us from getting a problematic julian day when `legacyConversion` is `false`?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -119,7 +128,44 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
calendar.set(Calendar.SECOND, seconds);
Timestamp ts = Timestamp.ofEpochMilli(calendar.getTimeInMillis(), (int) nanos);
- ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion);
+ ts = TimestampTZUtil.convertTimestampToZone(ts, ZoneOffset.UTC, targetZone, legacyConversion)
+ .minusDays(leapYearDateAdjustment);
+
return ts;
}
+
+ private static int julianLeapYearAdjustment(JulianDate jDateTime) {
+ Set<Integer> validDaysOfMonth = new HashSet<>();
+ validDaysOfMonth.add(1);
+ validDaysOfMonth.add(2);
+ validDaysOfMonth.add(27);
+ validDaysOfMonth.add(28);
+ validDaysOfMonth.add(29);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg())) {
+ return 2;
+ } else {
+ throw e;
+ }
+ }
+
+ int year = localDateTime.getYear();
+ int dayOfMonth = localDateTime.getDayOfMonth();
+ boolean isJulianLeapYear = (year % 4 == 0 && year % 400 != 0 && year % 100 == 0);
+
+ if (year < 1582 && isJulianLeapYear && validDaysOfMonth.contains(dayOfMonth)) {
+ switch (localDateTime.getMonth()) {
+ case FEBRUARY:
+ return -2;
+ case MARCH:
+ return 2;
+ default:
+ return 0;
+ }
+ }
+ return 0;
+ }
Review Comment:
Moreover it kinda feels like this kind of shifting should be done inside the `JulianDate` class. I haven't fully formulated an opinion yet but I raised https://github.com/oblac/jodd-util/issues/21 to see what the jodd authors have to say about this use case.
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1601862248
##########
common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java:
##########
@@ -529,7 +529,6 @@ public enum ErrorMsg {
PARQUET_FOOTER_ERROR(20022, "Failed to read parquet footer:"),
PARQUET_UNHANDLED_TYPE(20023, "Unhandled type {0}", true),
ORC_FOOTER_ERROR(20024, "Failed to read orc footer:"),
-
Review Comment:
revert this class
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600020249
##########
ql/src/test/queries/clientpositive/timestamp_julian.q:
##########
@@ -0,0 +1,22 @@
+--! qt:timezone:Asia/Singapore
+
+-- Test 1
+-- insert into default.test_sgt700 values ('0200-03-01 00:00:00')
+CREATE EXTERNAL TABLE `TEST_SGT`(`currtime` timestamp) ROW FORMAT SERDE 'org.apache.hadoop.hive.ql.io.parquet.serde.ParquetHiveSerDe' STORED AS
Review Comment:
we already have some `parquet_historical_timestamp_legacy.parq`, maybe you can set some meaningful names for the new files and add your test cases to `parquet_historical_timestamp.q`
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1601865707
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -100,12 +103,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
julianDay--;
}
- JulianDate jDateTime;
- jDateTime = JulianDate.of((double) julianDay);
+ JulianDate jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ int leapYearDateAdjustment = 0;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ if (e.getMessage().contains(TimestampTZUtil.NOT_LEAP_YEAR) && legacyConversion) {
+ leapYearDateAdjustment = 1;
Review Comment:
better use `2` to avoid retry after the timezone conversion
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1599815313
##########
ql/src/test/queries/clientpositive/timestamp_julian.q:
##########
@@ -0,0 +1,22 @@
+--! qt:timezone:Asia/Singapore
+
+-- Test 1
+-- insert into default.test_sgt700 values ('0200-03-01 00:00:00')
+CREATE EXTERNAL TABLE `TEST_SGT`(`currtime` timestamp) ROW FORMAT SERDE 'org.apache.hadoop.hive.ql.io.parquet.serde.ParquetHiveSerDe' STORED AS
Review Comment:
instead of q test, do it in TestParquetTimestampsHive2Compatibility
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595252070
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
why do you need to `parse` and `format` twice, first with `legacyDateFormatter` and then with `FORMATTER`?
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595464852
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
We need to use the legacyDateFormatter because of HIVE-24074: Incorrect handling of timestamp in Parquet/Avro when written in certain time zones in versions before Hive 3.x . Only with this can we get the correct timezone conversion for these dates.
So formatDate(ts, fromZone, toZone) uses legacyDateFormatter and returns `0200-02-29 00:00:00`. If we don't use the legacyDateFormatter we get `0200-02-28 22:55:25`which is incorrect.
Once we have `0200-02-29 00:00:00` we need to parse it with DateTimeFormatter with ResolverStyle.LENIENT , this converts 0200-02-29 00:00:00 to the correct value `0200-03-01 00:00:00.0 ` .
Finally, We need to format this again to return the timestamp.
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1595566438
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
Review Comment:
relying on DateTimeException exception is not a good practice, what if the error message 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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1596118138
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,22 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
Review Comment:
Agreed , I have replaced it with a conditional check .
##########
common/src/java/org/apache/hadoop/hive/common/type/TimestampTZUtil.java:
##########
@@ -213,4 +205,26 @@ public static Timestamp convertTimestampToZone(Timestamp ts, ZoneId fromZone, Zo
public static double convertTimestampTZToDouble(TimestampTZ timestampTZ) {
return timestampTZ.getEpochSecond() + timestampTZ.getNanos() / DateUtils.NANOS_PER_SEC;
}
+
+ public static Timestamp legacyLeapYearConversions(Timestamp ts, ZoneId fromZone, ZoneId toZone) {
+ DateTimeFormatter dtf = FORMATTER;
+ Timestamp result = Timestamp.valueOf(
+ dtf.format(dtf.withResolverStyle(ResolverStyle.LENIENT).parse(formatDate(ts, fromZone, toZone))));
Review Comment:
Sure
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1596434754
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ localDateTime = LocalDateTime.ofEpochSecond(jDateTime.toMilliseconds() / 1000,
+ (int) jDateTime.toMilliseconds() % 1000 * 1000000, ZoneOffset.UTC);
+ if (!localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth().name().equals("FEBRUARY") &&
Review Comment:
````
notLeapYear = !localDateTime.toLocalDate().isLeapYear()
&& localDateTime.getMonth() == Month.FEBRUARY
&& legacyConversion
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1596434754
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ localDateTime = LocalDateTime.ofEpochSecond(jDateTime.toMilliseconds() / 1000,
+ (int) jDateTime.toMilliseconds() % 1000 * 1000000, ZoneOffset.UTC);
+ if (!localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth().name().equals("FEBRUARY") &&
Review Comment:
`localDateTime.getMonth() == Month.FEBRUARY`
+ `localDateTime.getDayOfMonth = 29` ?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ localDateTime = LocalDateTime.ofEpochSecond(jDateTime.toMilliseconds() / 1000,
+ (int) jDateTime.toMilliseconds() % 1000 * 1000000, ZoneOffset.UTC);
+ if (!localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth().name().equals("FEBRUARY") &&
Review Comment:
`localDateTime.getMonth() == Month.FEBRUARY`
\+ `localDateTime.getDayOfMonth = 29` ?
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1596434754
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -102,10 +106,23 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
JulianDate jDateTime;
jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ localDateTime = LocalDateTime.ofEpochSecond(jDateTime.toMilliseconds() / 1000,
+ (int) jDateTime.toMilliseconds() % 1000 * 1000000, ZoneOffset.UTC);
+ if (!localDateTime.toLocalDate().isLeapYear() && localDateTime.getMonth().name().equals("FEBRUARY") &&
Review Comment:
````
notLeapYear = !localDateTime.toLocalDate().isLeapYear()
&& localDateTime.getMonth() == Month.FEBRUARY
&& localDateTime.getDayOfMonth() == 29
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1599719186
##########
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/timestamp/NanoTimeUtils.java:
##########
@@ -100,12 +104,24 @@ public static Timestamp getTimestamp(NanoTime nt, ZoneId targetZone, boolean leg
julianDay--;
}
- JulianDate jDateTime;
- jDateTime = JulianDate.of((double) julianDay);
+ JulianDate jDateTime = JulianDate.of((double) julianDay);
+ LocalDateTime localDateTime;
+ int leapYearDateAdjustment = 0;
+
+ try {
+ localDateTime = jDateTime.toLocalDateTime();
+ } catch (DateTimeException e) {
+ if (e.getMessage().contains(ErrorMsg.NOT_LEAP_YEAR.getMsg()) && legacyConversion) {
+ leapYearDateAdjustment = 1;
+ localDateTime = jDateTime.add(leapYearDateAdjustment).toLocalDateTime();
Review Comment:
````
localDateTime = jDateTime.add(++leapYearDateAdjustment).toLocalDateTime();
````
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2113470967
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "simhadri-g (via GitHub)" <gi...@apache.org>.
simhadri-g commented on code in PR #5241:
URL: https://github.com/apache/hive/pull/5241#discussion_r1600141321
##########
ql/src/test/queries/clientpositive/timestamp_julian.q:
##########
@@ -0,0 +1,22 @@
+--! qt:timezone:Asia/Singapore
+
+-- Test 1
+-- insert into default.test_sgt700 values ('0200-03-01 00:00:00')
+CREATE EXTERNAL TABLE `TEST_SGT`(`currtime` timestamp) ROW FORMAT SERDE 'org.apache.hadoop.hive.ql.io.parquet.serde.ParquetHiveSerDe' STORED AS
Review Comment:
Done
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28249: Parquet legacy timezone conversion converts march 1st to … [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5241:
URL: https://github.com/apache/hive/pull/5241#issuecomment-2106375724
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5241&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5241&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5241)
--
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-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org