Hi all, I opened a PR adding a portable Time logical type, apache/beam#39804, and two reviewers there raised points that are better settled on the list, so I am moving the design question here.
(No links below - a mail client rewrites bare URLs into tracking redirects, which would then sit in the archive. Everything is referenced by PR or issue number instead.) Where things stand today ------------------------ The Java SDK already has a Time logical type, and its identifier is the hardcoded string "beam:logical_type:time:v1". Because getLogicalTypeUrn returns any identifier beginning with "beam:logical_type:" unchanged, that URN goes out on the wire as-is, with no entry in LogicalTypes.Enum backing it. Measured on master at ca6065508a3, and the files involved are unchanged on current master: urn on the wire = beam:logical_type:time:v1 argument = STRING, empty string representation = INT64 read back as = UnknownLogicalType So the Java SDK occupies the portable namespace for this URN, cannot read its own output back, and along the way emits a vestigial empty-string argument. The Time class declares getArgumentType as STRING and getArgument as "", both marked "// unused" in the source, and those are serialised. Any SDK receiving such a schema sees a URN that looks standard, is documented nowhere, and carries nothing to reconstruct from. That is the problem I would like to close. There are two coherent ways. Option A: make it properly portable ----------------------------------- Add TIME to LogicalTypes.Enum and register it in SchemaTranslation. Following the Timestamp logical type rather than the Date one, since a time of day has a precision dimension that a date does not: argument type: INT32, the precision, the number of decimal digits of the sub-second field, 0 through 9 helpers: Time.of(int), plus MILLIS / MICROS / NANOS constants The part where I think Time should not follow Timestamp is the representation. Timestamp uses a Row of seconds INT64 plus subseconds INT16 or INT32. Whatever the full reasoning was there, one constraint is plain: a single INT64 of nanoseconds since the epoch spans only about 1678 to 2262, so it cannot carry the Instant range. A time of day has no such problem. It is bounded: the largest value is 86400 x 10^9 = 8.64 x 10^13 nanoseconds, five orders of magnitude inside Long.MAX_VALUE. A single INT64 of sub-units since midnight is therefore exact at every precision from 0 to 9, and the negative-modulo hazard that the Timestamp javadoc has to warn about does not arise, because the domain has no negative values. The tradeoff is symmetry against simplicity. A Row would let an SDK reuse whatever it already wrote for Timestamp. A plain INT64 is less for each SDK to implement and needs no per-precision branching on the field width. I lean toward the INT64 but do not hold that strongly, and it is the main thing I would like opinions on. Worth being precise about compatibility, since I got this wrong in my first draft of the PR description. With INT64 sub-units since midnight, Time.of(9) produces exactly the values the Java SDK produces today - same INT64 representation, same nanoseconds since midnight - so data and the SqlTypes TIME constant are unaffected. What does change in the serialised schema is the argument: today's meaningless STRING "" becomes an INT32 precision. So the encoded schema is not byte-identical; the values are, and the field that changes is one that carries no information today. Option B: stop squatting on the namespace ----------------------------------------- If the preference is to keep the model small - and I take the point that every addition is a spec obligation for every SDK and runner - then the consistent alternative is to change the Time identifier to a non-portable one, so it serialises as beam:logical_type:javasdk_time:v1 like other Java-only types. I would not propose this as the default, because it changes the URN that is on the wire today and so is the more disruptive of the two. But it does resolve the same inconsistency, and if that is the direction the list prefers I am happy to write it instead. What I am not proposing is leaving it as it is, with a portable-looking URN that nothing, including the SDK that emits it, can interpret. References ---------- This proposal: apache/beam#39804 Related issue: apache/beam#25946 Timestamp type: apache/beam#36705 (Java), apache/beam#39537 (Python). The timestamp strategy design doc is linked from the first of those. Happy to take this either way, and to do the Python side once the shape is settled. Thanks, Zihan Dai
