chamikaramj commented on a change in pull request #16923:
URL: https://github.com/apache/beam/pull/16923#discussion_r828584669
##########
File path:
runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/CoderTranslators.java
##########
@@ -204,6 +205,20 @@ public RowCoder fromComponents(
};
}
+ static CoderTranslator<NullableCoder<?>> nullable() {
+ return new SimpleStructuredCoderTranslator<NullableCoder<?>>() {
+ @Override
+ protected NullableCoder<?> fromComponents(List<Coder<?>> components) {
Review comment:
Raise an error if there's more than one component coder ?
##########
File path: sdks/python/apache_beam/typehints/typehints.py
##########
@@ -601,6 +604,13 @@ def __getitem__(self, py_type):
return Union[py_type, type(None)]
+def is_optional(typehint):
Review comment:
May be call this "is_nullable" to be more specific ?
##########
File path: sdks/python/apache_beam/coders/standard_coders_test.py
##########
@@ -193,7 +193,9 @@ class StandardCodersTest(unittest.TestCase):
value_parser: ShardedKey(
key=value_parser(x['key']), shard_id=x['shardId'].encode('utf-8')),
'beam:coder:custom_window:v1': lambda x,
- window_parser: window_parser(x['window'])
+ window_parser: window_parser(x['window']),
Review comment:
Hmm, why did we have to update this existing test ? (just to make sure
that there's no backwards compatibility issue here).
##########
File path: sdks/python/apache_beam/coders/coders.py
##########
@@ -584,6 +594,9 @@ def __hash__(self):
return hash(type(self)) + hash(self._value_coder)
+Coder.register_structured_urn(common_urns.coders.NULLABLE.urn, NullableCoder)
Review comment:
Do we have tests to make sure that coders of the three languages are
compatible ? For example, that you can encode a value of type
"NullableCoder(Other StandardCoder)" from SDK1 and decode from SDK2 and vice
versa for all SDK combinations ?
@TheNeuralBit might know the answer.
##########
File path:
runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/wire/CommonCoderTest.java
##########
@@ -382,6 +384,17 @@ private static Object convertValue(Object value,
CommonCoder coderSpec, Coder co
Map<String, Object> kvMap = (Map<String, Object>) value;
Coder windowCoder = ((TimestampPrefixingWindowCoder)
coder).getWindowCoder();
return convertValue(kvMap.get("window"),
coderSpec.getComponents().get(0), windowCoder);
+ } else if (s.equals(getUrn(StandardCoders.Enum.NULLABLE))) {
Review comment:
Are we generating individual tests for coders here or are we lumping
everything into a single test ? If it's latter probably we should consider
adding tests specifically for the new case.
##########
File path: sdks/python/apache_beam/coders/coders.py
##########
@@ -572,6 +572,16 @@ def _create_impl(self):
def to_type_hint(self):
return typehints.Optional[self._value_coder.to_type_hint()]
+ def _get_component_coders(self):
+ # type: () -> List[Coder]
+ return [self._value_coder]
+
+ @classmethod
+ def from_type_hint(cls, typehint, registry):
+ value_type = list(
Review comment:
You just expect one inner type here as well ?
If so we should probably validate that and simplify this code.
##########
File path: sdks/python/apache_beam/typehints/typehints_test.py
##########
@@ -320,6 +320,14 @@ def test_getitem_proxy_to_union(self):
hint = typehints.Optional[int]
self.assertTrue(isinstance(hint, typehints.UnionHint.UnionConstraint))
+ def test_is_optional(self):
Review comment:
Add more tests to check encoding/decoding with NullableCoder ? (we may
already have such tests)
##########
File path: sdks/go/pkg/beam/core/graph/coder/coder.go
##########
@@ -394,6 +395,19 @@ func NewKV(components []*Coder) *Coder {
}
}
+func NewN(component *Coder) *Coder {
+ checkCoderNotNil(component, "Nullable")
+ return &Coder{
+ Kind: Nullable,
+ T: typex.New(typex.NullableType, component.T),
+ Components: []*Coder{component},
+ }
+}
Review comment:
Pls address this.
--
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]
