PDGGK opened a new pull request, #39804:
URL: https://github.com/apache/beam/pull/39804
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:
```java
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:
```java
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.
--
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]
