TheNeuralBit commented on a change in pull request #14591:
URL: https://github.com/apache/beam/pull/14591#discussion_r618817939
##########
File path:
runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/CommonCoderTest.java
##########
@@ -332,6 +332,8 @@ private static Object convertValue(Object value,
CommonCoder coderSpec, Coder co
} else if (s.equals(getUrn(StandardCoders.Enum.ROW))) {
Schema schema;
try {
+ // SchemaApi.Schema.Builder schemaBuilder =
SchemaApi.Schema.newBuilder();
+ // JsonFormat.parser().merge(coderSpec.getPayload(), schemaBuilder);
Review comment:
nit: this looks like it was accidentally added
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaTranslation.java
##########
@@ -215,7 +215,19 @@ public static Schema schemaFromProto(SchemaApi.Schema
protoSchema) {
}
builder.setOptions(optionsFromProto(protoSchema.getOptionsList()));
Schema schema = builder.build();
- schema.setEncodingPositions(encodingLocationMap);
+
+ Preconditions.checkState(encodingLocationMap.size() ==
schema.getFieldCount());
+ long dinstictEncodingPositions =
encodingLocationMap.values().stream().distinct().count();
+ Preconditions.checkState(dinstictEncodingPositions <=
schema.getFieldCount());
+ if (dinstictEncodingPositions < schema.getFieldCount() &&
schema.getFieldCount() > 0) {
+ // This means that encoding positions were not specified in the proto.
Generally, we don't
+ // expect this to happen,
+ // but if it does happen, we expect none to be specified - in which case
the should all be
+ // zero.
Review comment:
nit
```suggestion
// but if it does happen, we expect none to be specified - in which
case they should all be
// zero.
```
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaTranslation.java
##########
@@ -215,7 +215,19 @@ public static Schema schemaFromProto(SchemaApi.Schema
protoSchema) {
}
builder.setOptions(optionsFromProto(protoSchema.getOptionsList()));
Schema schema = builder.build();
- schema.setEncodingPositions(encodingLocationMap);
+
+ Preconditions.checkState(encodingLocationMap.size() ==
schema.getFieldCount());
+ long dinstictEncodingPositions =
encodingLocationMap.values().stream().distinct().count();
+ Preconditions.checkState(dinstictEncodingPositions <=
schema.getFieldCount());
+ if (dinstictEncodingPositions < schema.getFieldCount() &&
schema.getFieldCount() > 0) {
+ // This means that encoding positions were not specified in the proto.
Generally, we don't
+ // expect this to happen,
Review comment:
Right now Python's row coder never sets encoding position, so Python
pipelines that use SqlTransform will trigger this path
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
