jacobmarble commented on code in PR #8658:
URL: https://github.com/apache/iceberg/pull/8658#discussion_r1343083398
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -48,6 +48,8 @@ private Types() {}
.put(TimeType.get().toString(), TimeType.get())
.put(TimestampType.withZone().toString(), TimestampType.withZone())
.put(TimestampType.withoutZone().toString(),
TimestampType.withoutZone())
+ .put(TimestampnsType.withZone().toString(),
TimestampnsType.withZone())
Review Comment:
> I don't see a discussion that discusses whether a new type should be used
vs. a new field in the timestamp type
> There is only one timestamp type with an adjust-to-utc flag. Instead,
there could have been two types, timestamp and timestamptz.
> clumsy backward compatible naming of timestamp (without any suffix) and
now timestamp_ns, where one type has a suffix while the other one hasn't.
> maybe there is a disadvantage of the field approach I don't see here.
We may be talking past each other wrt the Iceberg types vs the Java
implementation of those types.
- The design considers **existing Iceberg types** `timestamp` and
`timestamptz` (microsecond units), then **adds new types** `timestamp_ns` and
`timestamptz_ns` (nanosecond units).
- @rdblue guided the design decision away from (1) parameterized type
names `timestamp[MILLIS|MICROS|NANOS]` and (2) adding milliseconds at the same
time we add nanoseconds.
- This proposal for Java implementation of the design considers **existing
Java type** `TimestampType` (including field `private final boolean
adjustToUTC`), then **adds new type** `TimestampnsType` (including field
`private final boolean adjustToUTC`).
- The concern @JFinis raises is that Java `TimestampType` already
represents two Iceberg types, so it would make sense that the
- Do I understand you correctly?
- If so: I did start this change by extending Java type `TimestampType`
with a new field, but backed out and started over with a new Java type. I was
overwhelmed by the variety of cases where the micro/nano branch would be
needed. In contrast, adding a new type made it easier for me to self-audit and
test, and removed any confusion to a user of the API. The approach could
certainly be revisited.
> With a new field, everything would just stay timestamp and the field
would implicitly be Millis, if it's not set.
nit: Currently, `timestamp` and `timestamptz` represent microseconds, not
milliseconds. And yes, I had proposed that `timestamp` continue to refer to
microseconds for backward compatibility.
--
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-unsubscr...@iceberg.apache.org
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
