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 2020/06/19 01:01:17 UTC
[GitHub] [iceberg] edgarRd opened a new pull request #1127: ORC metrics: disable date column filter pushdown due to ORC incorrect filtering
edgarRd opened a new pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127
This PR attempts to solve issues #1116 and #1113 when running in different Time Zones.
I've added a test to check for local scan on different timezones which surfaced an issue with the pushdown predicates with DATE columns, which seem to use incorrect incorrect TZ to read the metrics within the ORC reader. Due to this, disabling ORC lower/upper bound metrics for Date in Iceberg as well.
I've tested by changing system TZ to other timezones and running the tests and they pass.
PTAL @shardulm94 @chenjunjiedada if you have a chance, please pull this branch and run the test suite locally just to double check the build. Thanks!
@rdblue
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] edgarRd commented on pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
edgarRd commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-650302395
@rdblue @shardulm94 PTAL whenever you have a chance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] edgarRd commented on pull request #1127: ORC metrics: disable date column filter pushdown due to ORC incorrect filtering
Posted by GitBox <gi...@apache.org>.
edgarRd commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-647850642
@rdblue I've simplified this PR and avoided disabling date column filter pushdown as that's not really the problem.
There're actually 2 problems, which are related:
1. Getting the min/max stats for Date types in [ORC are returned with milliseconds adjusted to the local timezone](https://github.com/apache/hive/blob/204d8525bf48bf85d15feb427e6f5a31ec192bac/storage-api/src/java/org/apache/hadoop/hive/serde2/io/DateWritable.java#L145). This means that timezones ahead of epoch would have less milliseconds to the date, therefore when converting to days from epoch they would be 1 day less if `EPOCH.plus` is used for the conversion as this one is UTC based.
2. In ORC the timezone used for adjusting the min/max metrics milliseconds seems to be stored at the [Thread level](https://github.com/apache/hive/blob/204d8525bf48bf85d15feb427e6f5a31ec192bac/storage-api/src/java/org/apache/hadoop/hive/serde2/io/DateWritable.java#L48) and looks like it's not reset, hence [tests switching multiple timezones fail](https://github.com/apache/iceberg/commit/e61cc6187c6a13b7913f66f548f4be9d05d2a717) since it appears that the TZ is not updated in ORC - https://github.com/apache/iceberg/issues/1116#issuecomment-645643279.
This PR is solving:
https://github.com/apache/iceberg/pull/1127/commits/a6fc6885f4ee7f0b00b224510191e4f5a7c3a547 Problem 1 - By using the returned `Date` object from ORC and calling `toLocalDate` which performs normalization to the local timezone.
I've added a comment to mention problem 2 in https://github.com/apache/iceberg/pull/1127/commits/b1fa8e6b3a636d434a46da360556fca100219ba0 and fixed some tests that changed the default TZ and did not reset it to the current default one https://github.com/apache/iceberg/pull/1127/commits/82d46d3c733914627ef89ae921788a60f2655f0a.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on pull request #1127: ORC metrics: disable date column filter pushdown due to ORC incorrect filtering
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-646902633
@edgarRd, I ran your branch on my local machine and it passed the build, thanks for fixing!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-650416548
Thanks for the summary, @edgarRd. I find this behavior concerning. I think it will help to cover some background, just to cover the context for specific comments.
As a storage layer, Iceberg should always read and return the exact data value that was written. For example, with floating point values, there is no "close enough" and Iceberg should read the same bits that were written. That's why we use `assertEquals` instead of assertions that a value is within some floating point error.
For date/time types, Iceberg should never modify a value. Concerns like translating from some time zone into the UTC value to store for `timestamptz` must be handled by processing engines, which handle calendars time zone offsets, and other concerns. Iceberg stores the UTC value without modification, and the data type tells the engine how to produce a value to write and consume a value that's read. This is why the internal representation is microseconds from the Unix epoch in UTC. That's unambiguous.
It looks like ORC uses representations that are timezone-dependent in some cases, like returning, and possibly calculating, stats. I think that it is good that we don't use these in the Iceberg readers or writers, and my first approach to fixing this would be to avoid having data passed using those objects.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1127: ORC metrics: disable date column filter pushdown due to ORC incorrect filtering
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-647680078
@edgarRd, can you summarize what the problem was and what this does to fix it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-650462092
@edgarRd, I created https://github.com/edgarRd/iceberg/pull/1 against your branch with the reflection fix. It passes all of the tests in this PR and the stats tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] chenjunjiedada commented on pull request #1127: ORC metrics: disable date column filter pushdown due to ORC incorrect filtering
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-646465830
@edgarRd , could you please fix CI build? Looks like it doesn't pass the build.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] edgarRd edited a comment on pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
edgarRd edited a comment on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-650467924
@rdblue great! I've merged the change. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1127: ORC metrics: disable date column filter pushdown due to ORC incorrect filtering
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-646722330
The CI failures are style checks:
```
[ant:checkstyle] [ERROR] /home/travis/build/apache/iceberg/orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java:24:8: Unused import - java.sql.Date. [UnusedImports]
[ant:checkstyle] [ERROR] /home/travis/build/apache/iceberg/orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java:46:8: Unused import - org.apache.iceberg.util.DateTimeUtil. [UnusedImports]
[ant:checkstyle] [ERROR] /home/travis/build/apache/iceberg/orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java:49:8: Unused import - org.apache.orc.DateColumnStatistics. [UnusedImports]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#discussion_r446455590
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -158,8 +158,7 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
.orElse(null);
} else if (columnStats instanceof DateColumnStatistics) {
min = Optional.ofNullable(((DateColumnStatistics) columnStats).getMinimum())
- .map(minStats -> DateTimeUtil.daysFromDate(
- DateTimeUtil.EPOCH.plus(minStats.getTime(), ChronoUnit.MILLIS).toLocalDate()))
+ .map(minStats -> DateTimeUtil.daysFromDate(((Date) minStats).toLocalDate()))
Review comment:
I'm okay with either of two options:
1. Don't collect date metrics until this is fixed in ORC
2. Use reflection to pull the `minimum` and `maximum` private int fields out of the stats
Option #2 seems reasonable enough to me, since the stats actually store the int min and max values that we're looking for.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#discussion_r446455590
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -158,8 +158,7 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
.orElse(null);
} else if (columnStats instanceof DateColumnStatistics) {
min = Optional.ofNullable(((DateColumnStatistics) columnStats).getMinimum())
- .map(minStats -> DateTimeUtil.daysFromDate(
- DateTimeUtil.EPOCH.plus(minStats.getTime(), ChronoUnit.MILLIS).toLocalDate()))
+ .map(minStats -> DateTimeUtil.daysFromDate(((Date) minStats).toLocalDate()))
Review comment:
I'm okay with either of two options:
1. Don't collect date metrics until this is fixed in ORC
2. Use reflection to pull the `minimum` and `maximum` private int fields out of the stats
Option 2 seems reasonable enough to me, since the stats actually store the int min and max values that we're looking for.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] edgarRd commented on a change in pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
edgarRd commented on a change in pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#discussion_r446447805
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -158,8 +158,7 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
.orElse(null);
} else if (columnStats instanceof DateColumnStatistics) {
min = Optional.ofNullable(((DateColumnStatistics) columnStats).getMinimum())
- .map(minStats -> DateTimeUtil.daysFromDate(
- DateTimeUtil.EPOCH.plus(minStats.getTime(), ChronoUnit.MILLIS).toLocalDate()))
+ .map(minStats -> DateTimeUtil.daysFromDate(((Date) minStats).toLocalDate()))
Review comment:
> I think your intent was to account for the adjustment made by ORC that you referenced by saying "Getting the min/max stats for Date types in ORC are returned with milliseconds adjusted to the local timezone."
Yes, ORC modifies the internally stored stats and returns the values dependent on the timezone. These methods adjust for that.
> I think the main problem is that this uses a path where the value is modified by ORC based on the JVM environment, which breaks a fundamental rule in Iceberg. I don't think that we should use a path where the value is modified. It's just too hard to guarantee that although the value was modified, we were able to correctly undo it.
I agree, ORC in this respect violates the rule and modifies the stored value. I'm okay with not using this path, and therefore removing `Date` metrics collection if that's okay. Does that sound good?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-650623060
Merged to master. Thanks @edgarRd!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-650456019
I just had a chat with @omalley about this issue and he's going to add a getter for the underlying values, `getMinimumDays` and `getMaximumDays`. That way, we won't need to go through a type with behavior dependent on the time zone and we will avoid the adjustment problems.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#discussion_r446425293
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -158,8 +158,7 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
.orElse(null);
} else if (columnStats instanceof DateColumnStatistics) {
min = Optional.ofNullable(((DateColumnStatistics) columnStats).getMinimum())
- .map(minStats -> DateTimeUtil.daysFromDate(
- DateTimeUtil.EPOCH.plus(minStats.getTime(), ChronoUnit.MILLIS).toLocalDate()))
+ .map(minStats -> DateTimeUtil.daysFromDate(((Date) minStats).toLocalDate()))
Review comment:
Javadoc for `Date.toLocalDate` says that the LocalDate returned is "in local time zone". It is implemented like this:
```java
return LocalDate.of(getYear() + 1900, getMonth() + 1, getDate());
```
And, those getter methods use the current zone. So this cannot be correct because the value is dependent on the local zone. I think your intent was to account for the adjustment made by ORC that you referenced by saying "Getting the min/max stats for Date types in ORC are returned with milliseconds adjusted to the local timezone."
I think the main problem is that this uses a path where the value is modified by ORC based on the JVM environment, which breaks a fundamental rule in Iceberg. I don't think that we should use a path where the value is modified.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#discussion_r446425293
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -158,8 +158,7 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
.orElse(null);
} else if (columnStats instanceof DateColumnStatistics) {
min = Optional.ofNullable(((DateColumnStatistics) columnStats).getMinimum())
- .map(minStats -> DateTimeUtil.daysFromDate(
- DateTimeUtil.EPOCH.plus(minStats.getTime(), ChronoUnit.MILLIS).toLocalDate()))
+ .map(minStats -> DateTimeUtil.daysFromDate(((Date) minStats).toLocalDate()))
Review comment:
Javadoc for `Date.toLocalDate` says that the LocalDate returned is "in local time zone". It is implemented like this:
```java
return LocalDate.of(getYear() + 1900, getMonth() + 1, getDate());
```
And, those getter methods use the current zone. So this cannot be correct because the value is dependent on the local zone. I think your intent was to account for the adjustment made by ORC that you referenced by saying "Getting the min/max stats for Date types in ORC are returned with milliseconds adjusted to the local timezone."
I think the main problem is that this uses a path where the value is modified by ORC based on the JVM environment, which breaks a fundamental rule in Iceberg. I don't think that we should use a path where the value is modified. It's just too hard to guarantee that although the value was modified, we were able to correctly undo it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] edgarRd commented on pull request #1127: ORC metrics: disable date column filter pushdown due to ORC incorrect filtering
Posted by GitBox <gi...@apache.org>.
edgarRd commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-646812638
Sorry about that, checkstyle errors after last change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] edgarRd commented on a change in pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
edgarRd commented on a change in pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#discussion_r446447805
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -158,8 +158,7 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
.orElse(null);
} else if (columnStats instanceof DateColumnStatistics) {
min = Optional.ofNullable(((DateColumnStatistics) columnStats).getMinimum())
- .map(minStats -> DateTimeUtil.daysFromDate(
- DateTimeUtil.EPOCH.plus(minStats.getTime(), ChronoUnit.MILLIS).toLocalDate()))
+ .map(minStats -> DateTimeUtil.daysFromDate(((Date) minStats).toLocalDate()))
Review comment:
> I think your intent was to account for the adjustment made by ORC that you referenced by saying "Getting the min/max stats for Date types in ORC are returned with milliseconds adjusted to the local timezone."
Yes, ORC modifies the internally stored stats and returns the values dependent on the timezone. These methods adjust for that.
> I think the main problem is that this uses a path where the value is modified by ORC based on the JVM environment, which breaks a fundamental rule in Iceberg. I don't think that we should use a path where the value is modified. It's just too hard to guarantee that although the value was modified, we were able to correctly undo it.
I agree, ORC in this respect violates the rule and modifies the stored value. I'm okay with not using this path, and therefore removing Date metrics collection if that's okay. Does that sound good?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] edgarRd commented on pull request #1127: ORC: fix date metrics to adjust milliseconds to local timezone
Posted by GitBox <gi...@apache.org>.
edgarRd commented on pull request #1127:
URL: https://github.com/apache/iceberg/pull/1127#issuecomment-650467924
@rdblue great! I've merge the change. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org