RussellSpitzer commented on code in PR #11775:
URL: https://github.com/apache/iceberg/pull/11775#discussion_r2154963152
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -300,8 +300,7 @@ public <T> Literal<T> to(Type type) {
case TIMESTAMP:
return (Literal<T>) new TimestampLiteral(value());
case TIMESTAMP_NANO:
- // assume micros and convert to nanos to match the behavior in the
timestamp case above
- return new TimestampLiteral(value()).to(type);
+ return (Literal<T>) new TimestampNanoLiteral(value());
Review Comment:
I think the current behavior is pretty surprising since you can imagine that
if I come to Iceberg and I never use non-nano timestamps and I pass in a Long
as a literal I probably would assume I'm writing a nano-precision long.
I think it may make sense to just *not* support long -> nanoTimestamp in
this transform at all and instead throw an error saying to use a
TimestampNanoLiteral (which we would make.) Unfortunately we are past the time
when we could make that change painlessly so it's probably not possible.
What we can do is catch the overflow and throw a smarter message like
"Literal.of(Long) assumes timestamp is given in microseconds and the value
given overflows the timestamp container. Use Literals.timestampNanos(Long
nanoseconds) to create a literal with nanosecond resolution"
--
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]
