JFinis commented on code in PR #8658:
URL: https://github.com/apache/iceberg/pull/8658#discussion_r1342463104
##########
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:
Thanks, now I see the discussions! That being said, I don't see a discussion
that discusses whether a new type should be used vs. a new field in the
timestamp type. Can you point me to that discussion? Maybe I'm missing the
forest for the trees here. Or the discussion was removed due to the text to
which it refers to being removed?
I see that it was a suggestion in your old versions. Therefore, I would like
to understand the rationale of the person who was against it. Can you maybe tag
them here? Then they can chime in with their reasoning.
> > the design was to separate different timestamps through fields, not new
types
> Help me out, where do you read that?
I read that in the code itself: There is only one `timestamp` type with an
`adjust-to-utc` flag. Instead, there could have been two types, `timestamp` and
`timestamptz`. Thus, the person doing the initial design decided for a flag
instead of two different types.
And now you add another dimension in which timestamps can be different.
Thus, the design closest to the initial design would be to have yet another
(enum) field on the timestamp type instead of a new type. This would also help
with the 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.
With a new field, everything would just stay `timestamp` and the field would
implicitly be Millis, if it's not set.
So at least for me, this points towards having the field. Though, maybe
there is a disadvantage of the field approach I don't see here.
--
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
