rdblue commented on code in PR #9008:
URL: https://github.com/apache/iceberg/pull/9008#discussion_r1520403209
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -426,23 +427,55 @@ protected Type.TypeID typeId() {
}
static class TimestampLiteral extends ComparableLiteral<Long> {
- TimestampLiteral(Long value) {
+ private final TimestampType.Unit unit;
+
+ TimestampLiteral(TimestampType.Unit unit, Long value) {
super(value);
+ this.unit = unit;
}
@Override
@SuppressWarnings("unchecked")
public <T> Literal<T> to(Type type) {
switch (type.typeId()) {
case TIMESTAMP:
- return (Literal<T>) this;
+ TimestampType.Unit toUnit = ((TimestampType) type).unit();
Review Comment:
The decision to use the same `TimestampType` class and use `unit` makes this
logic more complicated than necessary.
The idea behind having a Java enum, `Type.TypeID`, was that in most cases
you can use a switch statement and very simple logic within that switch. The
reason why `TimestampType` was parameterized by a boolean for `adjustToUTC` is
that the logic in most places is identical for `timestamp` and `timestamptz`
because they have the same internal representation.
For `timestamp_ns` and `timestamptz_ns`, I don't think that the same logic
applies. The internal representation is different between nanos and micros, so
you end up with this complexity, where you have to add a switch on a different
enum. I think it would be simpler to have a `TimestampNanosType` class instead
of `Unit`.
--
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
