zhangdove commented on a change in pull request #1368:
URL: https://github.com/apache/iceberg/pull/1368#discussion_r477143269
##########
File path: api/src/main/java/org/apache/iceberg/transforms/Timestamps.java
##########
@@ -54,12 +55,17 @@ public Integer apply(Long timestampMicros) {
// discards fractional seconds, not needed for calculation
OffsetDateTime timestamp = Instant
- .ofEpochSecond(timestampMicros / 1_000_000)
+ .ofEpochSecond(timestampMicros / 1_000_000 +
zoneOffset.getTotalSeconds())
.atOffset(ZoneOffset.UTC);
return (int) granularity.between(EPOCH, timestamp);
}
+ public Timestamps zoneOffset(ZoneOffset newZoneOffset) {
+ this.zoneOffset = newZoneOffset;
Review comment:
>This changes the zone offset for all uses of the given granularity in
this JVM and can unexpectedly change the behavior of another table. I think it
would be a serious correctness bug.
The variable zoneOffset is not a global static variable,I'm not sure why it
affects the other tables.
But I may have realized that enumerating classes is not really a good way to
use 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
