You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/11/16 03:57:31 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request, #6199: API, Core: Move micros and days conversions to DateTimeUtil
aokolnychyi opened a new pull request, #6199:
URL: https://github.com/apache/iceberg/pull/6199
While working on time-related functions in the Spark function catalog, I wanted to use `Transform`. However, calling transforms on primitive values would require unnecessary boxing. That's why I decided to move relevant conversions to `DateTimeUtil` and leverage the utility directly. It is in line with what we did for bucketing in PR #5513.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#discussion_r1024596919
##########
api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java:
##########
@@ -133,4 +134,56 @@ public static long isoTimestampToMicros(String timestampString) {
return microsFromTimestamp(
LocalDateTime.parse(timestampString, DateTimeFormatter.ISO_LOCAL_DATE_TIME));
}
+
+ public static int daysToYears(int days) {
+ return convertDays(days, ChronoUnit.YEARS);
+ }
+
+ public static int daysToMonths(int days) {
+ return convertDays(days, ChronoUnit.MONTHS);
+ }
+
+ private static int convertDays(int days, ChronoUnit granularity) {
+ if (days >= 0) {
+ LocalDate date = EPOCH_DAY.plusDays(days);
+ return (int) granularity.between(EPOCH_DAY, date);
+ } else {
+ // add 1 day to the value to account for the case where there is exactly 1 unit between the
+ // date and epoch because the result will always be decremented.
+ LocalDate date = EPOCH_DAY.plusDays(days + 1);
+ return (int) granularity.between(EPOCH_DAY, date) - 1;
+ }
+ }
+
+ public static int microsToYears(long micros) {
+ return convertMicros(micros, ChronoUnit.YEARS);
+ }
+
+ public static int microsToMonths(long micros) {
+ return convertMicros(micros, ChronoUnit.MONTHS);
+ }
+
+ public static int microsToDays(long micros) {
+ return convertMicros(micros, ChronoUnit.DAYS);
+ }
+
+ public static int microsToHours(long micros) {
+ return convertMicros(micros, ChronoUnit.HOURS);
+ }
+
+ private static int convertMicros(long micros, ChronoUnit granularity) {
+ if (micros >= 0) {
+ long epochSecond = Math.floorDiv(micros, MICROS_PER_SECOND);
Review Comment:
I think we can pull only `epochSecond` so I wasn't sure it was worth it. Let me do that.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#discussion_r1024845392
##########
api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java:
##########
@@ -133,4 +134,56 @@ public static long isoTimestampToMicros(String timestampString) {
return microsFromTimestamp(
LocalDateTime.parse(timestampString, DateTimeFormatter.ISO_LOCAL_DATE_TIME));
}
+
+ public static int daysToYears(int days) {
+ return convertDays(days, ChronoUnit.YEARS);
+ }
+
+ public static int daysToMonths(int days) {
+ return convertDays(days, ChronoUnit.MONTHS);
+ }
+
+ private static int convertDays(int days, ChronoUnit granularity) {
+ if (days >= 0) {
+ LocalDate date = EPOCH_DAY.plusDays(days);
+ return (int) granularity.between(EPOCH_DAY, date);
+ } else {
+ // add 1 day to the value to account for the case where there is exactly 1 unit between the
+ // date and epoch because the result will always be decremented.
+ LocalDate date = EPOCH_DAY.plusDays(days + 1);
+ return (int) granularity.between(EPOCH_DAY, date) - 1;
+ }
+ }
+
+ public static int microsToYears(long micros) {
+ return convertMicros(micros, ChronoUnit.YEARS);
+ }
+
+ public static int microsToMonths(long micros) {
+ return convertMicros(micros, ChronoUnit.MONTHS);
+ }
+
+ public static int microsToDays(long micros) {
+ return convertMicros(micros, ChronoUnit.DAYS);
+ }
+
+ public static int microsToHours(long micros) {
+ return convertMicros(micros, ChronoUnit.HOURS);
+ }
+
+ private static int convertMicros(long micros, ChronoUnit granularity) {
+ if (micros >= 0) {
+ long epochSecond = Math.floorDiv(micros, MICROS_PER_SECOND);
Review Comment:
I moved that var but it became somehow disconnected from the rest of the logic in each branch. I kept it separate like in the original snippet.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#discussion_r1024595729
##########
api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java:
##########
@@ -133,4 +134,56 @@ public static long isoTimestampToMicros(String timestampString) {
return microsFromTimestamp(
LocalDateTime.parse(timestampString, DateTimeFormatter.ISO_LOCAL_DATE_TIME));
}
+
+ public static int daysToYears(int days) {
Review Comment:
Good point, will do.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#discussion_r1023681076
##########
api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java:
##########
@@ -133,4 +134,56 @@ public static long isoTimestampToMicros(String timestampString) {
return microsFromTimestamp(
LocalDateTime.parse(timestampString, DateTimeFormatter.ISO_LOCAL_DATE_TIME));
}
+
+ public static int daysToYears(int days) {
Review Comment:
should we also move `TestDateTimeUtil` to `api`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#discussion_r1023728546
##########
api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java:
##########
@@ -133,4 +134,56 @@ public static long isoTimestampToMicros(String timestampString) {
return microsFromTimestamp(
LocalDateTime.parse(timestampString, DateTimeFormatter.ISO_LOCAL_DATE_TIME));
}
+
+ public static int daysToYears(int days) {
+ return convertDays(days, ChronoUnit.YEARS);
+ }
+
+ public static int daysToMonths(int days) {
+ return convertDays(days, ChronoUnit.MONTHS);
+ }
+
+ private static int convertDays(int days, ChronoUnit granularity) {
+ if (days >= 0) {
+ LocalDate date = EPOCH_DAY.plusDays(days);
+ return (int) granularity.between(EPOCH_DAY, date);
+ } else {
+ // add 1 day to the value to account for the case where there is exactly 1 unit between the
+ // date and epoch because the result will always be decremented.
+ LocalDate date = EPOCH_DAY.plusDays(days + 1);
+ return (int) granularity.between(EPOCH_DAY, date) - 1;
+ }
+ }
+
+ public static int microsToYears(long micros) {
+ return convertMicros(micros, ChronoUnit.YEARS);
+ }
+
+ public static int microsToMonths(long micros) {
+ return convertMicros(micros, ChronoUnit.MONTHS);
+ }
+
+ public static int microsToDays(long micros) {
+ return convertMicros(micros, ChronoUnit.DAYS);
+ }
+
+ public static int microsToHours(long micros) {
+ return convertMicros(micros, ChronoUnit.HOURS);
+ }
+
+ private static int convertMicros(long micros, ChronoUnit granularity) {
+ if (micros >= 0) {
+ long epochSecond = Math.floorDiv(micros, MICROS_PER_SECOND);
Review Comment:
Nit: am I missing something or can we just pull these out of individual blocks?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#issuecomment-1318216016
Thanks for reviewing, @nastra @szehon-ho @gaborkaszab!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#discussion_r1023481066
##########
api/src/main/java/org/apache/iceberg/transforms/Dates.java:
##########
@@ -50,24 +48,19 @@ public Integer apply(Integer days) {
return null;
}
- if (granularity == ChronoUnit.DAYS) {
- return days;
- }
-
- if (days >= 0) {
- LocalDate date = EPOCH.plusDays(days);
- return (int) granularity.between(EPOCH, date);
- } else {
- // add 1 day to the value to account for the case where there is exactly 1 unit between the
- // date and epoch because the result will always be decremented.
- LocalDate date = EPOCH.plusDays(days + 1);
- return (int) granularity.between(EPOCH, date) - 1;
+ switch (granularity) {
+ case YEARS:
+ return DateTimeUtil.daysToYears(days);
+ case MONTHS:
+ return DateTimeUtil.daysToMonths(days);
+ case DAYS:
+ return days;
+ default:
+ throw new UnsupportedOperationException("Unsupported time unit: " + granularity);
Review Comment:
This block is now similar to `toHumanString` below.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#discussion_r1024786018
##########
api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java:
##########
@@ -133,4 +134,56 @@ public static long isoTimestampToMicros(String timestampString) {
return microsFromTimestamp(
LocalDateTime.parse(timestampString, DateTimeFormatter.ISO_LOCAL_DATE_TIME));
}
+
+ public static int daysToYears(int days) {
Review Comment:
Moved.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#discussion_r1023482889
##########
api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java:
##########
@@ -133,4 +134,56 @@ public static long isoTimestampToMicros(String timestampString) {
return microsFromTimestamp(
LocalDateTime.parse(timestampString, DateTimeFormatter.ISO_LOCAL_DATE_TIME));
}
+
+ public static int daysToYears(int days) {
+ return convertDays(days, ChronoUnit.YEARS);
+ }
+
+ public static int daysToMonths(int days) {
+ return convertDays(days, ChronoUnit.MONTHS);
+ }
+
+ private static int convertDays(int days, ChronoUnit granularity) {
+ if (days >= 0) {
+ LocalDate date = EPOCH_DAY.plusDays(days);
+ return (int) granularity.between(EPOCH_DAY, date);
+ } else {
+ // add 1 day to the value to account for the case where there is exactly 1 unit between the
+ // date and epoch because the result will always be decremented.
+ LocalDate date = EPOCH_DAY.plusDays(days + 1);
+ return (int) granularity.between(EPOCH_DAY, date) - 1;
+ }
+ }
+
+ public static int microsToYears(long micros) {
+ return convertMicros(micros, ChronoUnit.YEARS);
+ }
+
+ public static int microsToMonths(long micros) {
+ return convertMicros(micros, ChronoUnit.MONTHS);
+ }
+
+ public static int microsToDays(long micros) {
+ return convertMicros(micros, ChronoUnit.DAYS);
+ }
+
+ public static int microsToHours(long micros) {
+ return convertMicros(micros, ChronoUnit.HOURS);
+ }
+
+ private static int convertMicros(long micros, ChronoUnit granularity) {
Review Comment:
These conversions are covered by `TestDates` and `TestTimestamps` suites.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi merged pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi merged PR #6199:
URL: https://github.com/apache/iceberg/pull/6199
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#discussion_r1023481446
##########
api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java:
##########
@@ -133,4 +134,56 @@ public static long isoTimestampToMicros(String timestampString) {
return microsFromTimestamp(
LocalDateTime.parse(timestampString, DateTimeFormatter.ISO_LOCAL_DATE_TIME));
}
+
+ public static int daysToYears(int days) {
Review Comment:
I had to move this utility to `api` as transforms are part of `api`. I feel that's okay since `BucketUtil` and a few others are already defined there.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #6199: API, Core: Move micros and days conversions to DateTimeUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on PR #6199:
URL: https://github.com/apache/iceberg/pull/6199#issuecomment-1316294974
@kbendick @rdblue @RussellSpitzer @szehon-ho @flyrain, could you take a look at this PR? I need to use these conversions in our Spark function catalog.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org