TheNeuralBit commented on code in PR #22561:
URL: https://github.com/apache/beam/pull/22561#discussion_r954384079
##########
sdks/python/apache_beam/typehints/schemas.py:
##########
@@ -647,9 +648,36 @@ def _from_typing(cls, typ):
('micros', np.int64)])
[email protected]_logical_type
+class MillisInstant(NoArgumentLogicalType[Timestamp, np.int64]):
+ """Millis instant logical type handles values consistent with that encoded by
+ InstantCoder in Java sdk.
Review Comment:
nit:
```suggestion
"""Millisecond-precision instant logical type handles values consistent
with that encoded by
``InstantCoder`` in the Java SDK.
```
##########
sdks/python/apache_beam/typehints/schemas.py:
##########
@@ -647,9 +648,36 @@ def _from_typing(cls, typ):
('micros', np.int64)])
[email protected]_logical_type
+class MillisInstant(NoArgumentLogicalType[Timestamp, np.int64]):
+ """Millis instant logical type handles values consistent with that encoded by
+ InstantCoder in Java sdk.
+
+ This class handles Timestamp language type as :class:`MicrosInstant`, but it
+ only provides millisecond precision, because it is aimed to handle data
+ encoded by Java sdk's InstantCoder which has same precision level.
+ """
+ @classmethod
+ def representation_type(cls):
+ # type: () -> type
+ return np.int64
+
+ @classmethod
+ def urn(cls):
+ return common_urns.millis_instant.urn
+
+ @classmethod
+ def language_type(cls):
+ return Timestamp
+
+
+# Make sure MicrosInstant is registered after MillisInstant so that it
+# overwrites the mapping of Timestamp language type representation choice and
+# thus does not lose microsecond precision inside python sdk.
@LogicalType.register_logical_type
class MicrosInstant(NoArgumentLogicalType[Timestamp,
MicrosInstantRepresentation]):
+ """MicroInstant logical type handles Timestamp."""
Review Comment:
nit:
```suggestion
"""Microsecond-precision instant logical type that handles
``Timestamp``."""
```
##########
sdks/python/apache_beam/coders/row_coder.py:
##########
@@ -163,10 +165,15 @@ def _nonnull_coder_from_type(field_type):
_coder_from_type(field_type.map_type.key_type),
_coder_from_type(field_type.map_type.value_type))
elif type_info == "logical_type":
- # Special case for the Any logical type. Just use the default coder for an
- # unknown Python object.
if field_type.logical_type.urn == PYTHON_ANY_URN:
+ # Special case for the Any logical type. Just use the default coder for
an
+ # unknown Python object.
return typecoders.registry.get_coder(object)
+ elif field_type.logical_type.urn == DATETIME_URN:
+ # Special case for datetime logical type.
+ # DATETIME_URN explicitly uses TimestampCoder which deals with fix length
+ # 8-bytes big-endian-long instead of varint coder.
+ return TimestampCoder()
Review Comment:
Hmm I'd really like to have this tested at the `standard_coders.yaml` level
as a unit test, it's not ideal if the only thing verifying compatibility is an
integration test in Python.
If you have time tomorrow maybe we could do a quick video call to live debug
what's going wrong here, and see if we can work around it. If it will be
painful we can leave it as future work, but I'd like to understand the level of
effort.
--
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]
