Portable Time type (Java + model changes)

[PDGGK]

Adds the portable Time logical type, so a LocalTime field can cross an SDK boundary the way Date can since #38077.

Time already exists in the Java SDK but is not portable: its identifier is a hardcoded string that no entry in LogicalTypes.Enum backs, and it is absent from SchemaTranslation.STANDARD_LOGICAL_TYPES. A schema arriving from another SDK carrying beam:logical_type:time:v1 therefore comes back as UnknownLogicalType rather than as Time.

This is a direct mirror of #38077:

• schema.proto — TIME = 10, representation INT64, nanoseconds since midnight, matching what Time.toBaseType already produces;
• SchemaTranslation — register Time.IDENTIFIER -> Time.class;
• Time.java — take IDENTIFIER from the enum, and return null from getArgumentType() instead of the STRING/"" pair that was marked // unused.

Following the same split @Abacn suggested on #37830, the Python changes go in a separate PR so the new URN is in a snapshot container before the cross-language tests need it. JdbcUtil is deliberately left out too: the Date PR added a Date.valueOf(LocalDate) setter, but the Time equivalent, Time.valueOf(LocalTime), silently truncates sub-second precision, and this type's whole base representation is nanoseconds. That deserves its own change rather than a copy of the Date line.

Testing

SchemaTranslationTest — 89 tests, 0 failures. spotlessJavaCheck, checkstyleMain and checkstyleTest are clean.

The two entries added to the existing parameterised lists are not on their own enough, and it is worth saying why. LogicalTypesTest.testLogicalTypeFromToProtoCorrectly branches on the registry it is meant to be exercising:

if (STANDARD_LOGICAL_TYPES.containsKey(translated.getLogicalType().getIdentifier())) {
  assertThat(translated.getLogicalType().getClass(), equalTo(fieldType.getLogicalType().getClass()));
} else {
  assertThat(translated.getLogicalType().getClass(), equalTo(UnknownLogicalType.class));
}

Drop the SchemaTranslation registration and the else branch simply takes over — the suite stays green. I checked: with that one line removed, all 87 pre-existing tests still passed.

So this adds PortableLogicalTypeFromUrnTest, which asserts the recovery directly and consults nothing:

SchemaApi.FieldType proto = SchemaTranslation.fieldTypeToProto(fieldType, false, false);
assertThat(proto.getLogicalType().getUrn(), equalTo("beam:logical_type:time:v1"));
assertThat(proto.getLogicalType().getPayload().size(), equalTo(0));

Schema.FieldType translated = SchemaTranslation.fieldTypeFromProto(proto);
assertThat(translated.getLogicalType().getClass(), equalTo(Time.class));

With the registration removed this fails as it should —

timeIsRecoveredFromItsUrnAlone
Expected: <class org.apache.beam.sdk.schemas.logicaltypes.Time>
     but: was <class org.apache.beam.sdk.schemas.logicaltypes.UnknownLogicalType>

— while its Date sibling stays green, so the failure is the missing registration and not the test class itself.

Addresses part of #25946. @ahmedabu98 you offered to review this one back in March — PTAL.

[Abacn]

In practice a "portable" type effectively defined an "abstract" type asking each SDK to implement. In general /model change is conservative as each addition introduces additional spec/responsibility asking each SDK / runner to implement it

[PDGGK]

That is a fair bar, and I would rather the model stay small too. Let me put the specific numbers here, because I think this case is narrower than adding a new abstract type.

beam:logical_type:time:v1 is already on the wire from the Java SDK today, with no entry in the model. Time.IDENTIFIER is a hardcoded string starting with beam:logical_type:, and getLogicalTypeUrn passes such identifiers through unchanged, so it never gets the javasdk_ treatment that other unregistered Java types get. Measured on master at ca6065508a3:

urn on the wire   = beam:logical_type:time:v1
payload bytes     = 0
read back as      = UnknownLogicalType
read back urn     = beam:logical_type:time:v1

So today Java occupies the portable namespace for this URN, emits it with an empty payload — and cannot read its own output back. Any SDK receiving it has a URN that looks standard, is documented nowhere, and carries nothing to reconstruct from.

That is what makes me read this as documenting an obligation the Java SDK already imposes, rather than creating one. The enum entry costs other SDKs nothing they are not already paying: an SDK that does not implement TIME behaves exactly as it does now.

If the preference is the other direction, the consistent alternative is to stop squatting on the portable namespace — change Time.IDENTIFIER to a non-portable identifier so it serialises as beam:logical_type:javasdk_time:v1 like other Java-only types. I would rather not propose that as the default, since it changes the URN currently on the wire and so is the more disruptive of the two, but it is a coherent resolution and I am happy to write it instead.

Either way the present state seems worth not leaving alone. Whichever you prefer, I will follow.

[github-actions]

Assigning reviewers:

R: @chamikaramj for label java.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

[ahmedabu98]

This type may not be a straightforward redo of the Date type, since conceptually it can be described by different precisions. Using nanosecond precision makes sense today, but one could argue it's arbitrary, and future applications may require a higher precision.

Check out the timestamp logical type (design doc + Java implementation + Python implementation), where we include a "precision" parameter. I think it makes sense to do something similar here.

Python already has a custom Timestamp type. I think we can do with a new custom Time type that behaves similarly, but others may have opinions.

I think at the very least, this topic deserves a discussion in the mailing dev list. Feel free to send a message there so others can pitch in

[PDGGK]

That is a better framing than the one I brought, and I am happy to redo it that way. Having read through Timestamp, one thing about time-of-day does fall out differently, so let me put the concrete shape here before taking it to dev@.

Precision as an argument, same as Timestamp. getArgumentType() = INT32, getArgument() = precision, MIN_PRECISION = 0 / MAX_PRECISION = 9, plus Time.of(int) and MILLIS / MICROS / NANOS constants.

Representation is where it need not follow. Timestamp uses ROW<seconds: INT64, subseconds: INT16|INT32> because epoch-seconds at nanosecond precision does not fit a single INT64 — that overflows around 2262. Time-of-day is bounded: the largest value is 86400 × 10⁹ = 8.64 × 10¹³ nanoseconds, four orders of magnitude inside Long.MAX_VALUE. So a single INT64 of sub-units since midnight is exact at every precision from 0 to 9, and the negative-modulo footgun that Timestamp's javadoc has to warn about does not arise, because the domain has no negatives.

The tradeoff is symmetry against simplicity: a ROW would let an SDK reuse whatever it wrote for Timestamp, while a plain INT64 is less for each SDK to implement and needs no per-precision branching on the field width. My inclination is the INT64, but that is exactly the sort of thing worth other opinions.

One useful property either way. With INT64 sub-units since midnight, Time.of(9) is byte-identical to what the Java SDK produces today, so SqlTypes.TIME keeps its current representation and existing pipelines are unaffected — the precision parameter is purely additive.

On Python: agreed that a custom type mirroring the Timestamp one is the way, rather than the Date-style approach in the follow-up I had planned.

I will start the dev@ thread with the above and link back here. Meanwhile I will hold this PR rather than push a half-converted version — happy to close and reopen against whatever the list settles on, if you would prefer that to leaving it open.