epgif commented on code in PR #9008:
URL: https://github.com/apache/iceberg/pull/9008#discussion_r1490137388
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -205,38 +208,74 @@ public String toString() {
}
public static class TimestampType extends PrimitiveType {
- private static final TimestampType INSTANCE_WITH_ZONE = new
TimestampType(true);
- private static final TimestampType INSTANCE_WITHOUT_ZONE = new
TimestampType(false);
+ private static final TimestampType INSTANCE_MICROS_WITH_ZONE =
+ new TimestampType(true, ChronoUnit.MICROS);
+ private static final TimestampType INSTANCE_MICROS_WITHOUT_ZONE =
+ new TimestampType(false, ChronoUnit.MICROS);
+ private static final TimestampType INSTANCE_NANOS_WITH_ZONE =
+ new TimestampType(true, ChronoUnit.NANOS);
+ private static final TimestampType INSTANCE_NANOS_WITHOUT_ZONE =
+ new TimestampType(false, ChronoUnit.NANOS);
+
+ /** @deprecated use {@link #microsWithZone()} instead. */
+ @Deprecated
Review Comment:
Done.
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -564,14 +569,26 @@ private static String sanitizeDate(int days, int today) {
return "(date)";
}
- private static String sanitizeTimestamp(long micros, long now) {
- String isPast = now > micros ? "ago" : "from-now";
- long diff = Math.abs(now - micros);
- if (diff < FIVE_MINUTES_IN_MICROS) {
+ private static String sanitizeTimestamp(ChronoUnit unit, long timeUnits,
long nowMillis) {
+ long timeMillis;
+ switch (unit) {
+ case MICROS:
+ timeMillis = DateTimeUtil.microsToMillis(timeUnits);
+ break;
+ case NANOS:
+ timeMillis = DateTimeUtil.nanosToMillis(timeUnits);
Review Comment:
I see what you mean now. How does it look now?
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -46,8 +47,10 @@ private Types() {}
.put(DoubleType.get().toString(), DoubleType.get())
.put(DateType.get().toString(), DateType.get())
.put(TimeType.get().toString(), TimeType.get())
- .put(TimestampType.withZone().toString(), TimestampType.withZone())
- .put(TimestampType.withoutZone().toString(),
TimestampType.withoutZone())
+ .put(TimestampType.microsWithZone().toString(),
TimestampType.microsWithZone())
+ .put(TimestampType.microsWithoutZone().toString(),
TimestampType.microsWithoutZone())
Review Comment:
Done.
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -501,18 +531,22 @@ public <T> Literal<T> to(Type type) {
return (Literal<T>) new TimeLiteral(timeMicros);
case TIMESTAMP:
- if (((Types.TimestampType) type).shouldAdjustToUTC()) {
- long timestampMicros =
- ChronoUnit.MICROS.between(
- EPOCH, OffsetDateTime.parse(value(),
DateTimeFormatter.ISO_DATE_TIME));
- return (Literal<T>) new TimestampLiteral(timestampMicros);
+ TimestampType tsType = (TimestampType) type;
+ if (tsType.shouldAdjustToUTC()) {
+ long timestampUnits =
+ tsType
+ .unit()
+ .between(EPOCH, OffsetDateTime.parse(value(),
DateTimeFormatter.ISO_DATE_TIME));
Review Comment:
My bad; I thought I knew what you meant when I replied two weeks ago, but I
do not! Can you clarify exactly what you'd like to see moved to DateTimeUtil?
At first I thought you might have wanted this whole case moved to
DateTimeUtil, but then I realized that would create a circular dependency
between Literals and DateTimeUtil.
I reorganized this a bit and maybe it helps, but I still don't know what
ought to move out.
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
