chamikaramj commented on code in PR #17608:
URL: https://github.com/apache/beam/pull/17608#discussion_r871611650
##########
model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto:
##########
@@ -1094,6 +1094,13 @@ message StandardCoders {
}
}
+message LogicalTypes {
Review Comment:
Let's add a top level comment as well.
##########
model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto:
##########
@@ -1094,6 +1094,13 @@ message StandardCoders {
}
}
+message LogicalTypes {
Review Comment:
Seems like we defined other logical types in comments:
https://github.com/apache/beam/blob/fd61a90057011270dbf9a36c73b5baaf120100e2/model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto#L1049
For consistency, should we move all definitions here ?
cc: @TheNeuralBit
##########
sdks/java/extensions/python/src/main/java/org/apache/beam/sdk/extensions/python/PythonExternalTransform.java:
##########
@@ -179,16 +204,20 @@ Row buildOrGetKwargsRow() {
// Types that are not one of following are considered custom types.
// * Java primitives
// * Type String
+ // * Type PythonCallableSource
+ // * Any Type explicitly annotated by withTypeHint()
// * Type Row
- private static boolean isCustomType(java.lang.Class<?> type) {
+ private boolean isCustomType(java.lang.Class<?> type) {
boolean val =
!(ClassUtils.isPrimitiveOrWrapper(type)
|| type == String.class
+ || type == PythonCallableSource.class
Review Comment:
Can this be generalized to all logical types somehow (instead of special
casing PythonCallableSource) ?
##########
sdks/java/extensions/python/src/main/java/org/apache/beam/sdk/extensions/python/PythonExternalTransform.java:
##########
@@ -162,6 +167,26 @@ public PythonExternalTransform<InputT, OutputT>
withKwargs(Row kwargs) {
return this;
}
+ /**
+ * Specifies the field type of arguments.
+ *
+ * <p>Type hints are especially useful for logical types since type
inference does not work well
+ * for logical types.
+ *
+ * @param argType A class object for the argument type.
+ * @param fieldType A schema field type for the argument.
+ * @return updated wrapper for the cross-language transform.
+ */
+ public PythonExternalTransform<InputT, OutputT> withTypeHint(
Review Comment:
Should this be per arg instead of per type ? In other words, can the same
class map to different schema types ?
--
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]
