rdblue commented on a change in pull request #1368:
URL: https://github.com/apache/iceberg/pull/1368#discussion_r476858595
##########
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.
Instead, I think this PR needs to create a new `TimestampTransform` class
like the `BucketTransform` class and remove old references to this enum.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
