apilloud commented on a change in pull request #12054:
URL: https://github.com/apache/beam/pull/12054#discussion_r445244602
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/SqlTypes.java
##########
@@ -27,4 +28,7 @@ private SqlTypes() {}
/** Beam LogicalType corresponding to ZetaSQL/CalciteSQL DATE type. */
public static final LogicalType<LocalDate, Long> DATE = new Date();
+
+ /** Beam LogicalType corresponding to ZetaSQL/CalciteSQL TIME type. */
+ public static final LogicalType<LocalTime, Long> TIME = new Time();
Review comment:
This is the most important line of this CL for correctness, wanted to
get attention on it early.
ZetaSQL uses a very specific format for this, which roughly matches the
struct in LocalTime:
https://github.com/google/zetasql/blob/79adcd0fe227173e68ed7aa88f580a691ebe82c2/zetasql/public/civil_time.h#L85
It appears the Long base type is an offset in Nanoseconds. Is that
conversion lossless? Is it expensive?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
