[GitHub] [beam] amaliujia commented on a change in pull request #12481: [BEAM-10571] Use schemas in ExternalConfigurationPayload

[GitBox <gi...@apache.org>]

amaliujia commented on a change in pull request #12481:
URL: https://github.com/apache/beam/pull/12481#discussion_r466586877



##########
File path: sdks/java/expansion-service/build.gradle
##########
@@ -44,6 +44,8 @@ dependencies {
   compile library.java.slf4j_api
   runtimeOnly library.java.slf4j_jdk14
   testCompile library.java.junit
+  // TODO(BEAM-10632): Remove this. Currently Schema inference (used in ExpansionServiceTest) hits an NPE when checker is enabled, and checkerframework is not in the classpath.

Review comment:
       Split this comment to multiple lines?

##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -222,14 +228,21 @@ public void partitionTest() {
   }
 
   private byte[] toStringPayloadBytes(String data) throws IOException {
+    Row configRow =
+        Row.withSchema(Schema.of(Field.of("data", FieldType.STRING)))
+            .withFieldValue("data", data)
+            .build();
+
+    ByteString.Output outputStream = ByteString.newOutput();
+    try {
+      RowCoder.of(configRow.getSchema()).encode(configRow, outputStream);
+    } catch (IOException e) {
+      throw new RuntimeException(e);
+    }
     ExternalTransforms.ExternalConfigurationPayload payload =
         ExternalTransforms.ExternalConfigurationPayload.newBuilder()
-            .putConfiguration(
-                "data",
-                ExternalTransforms.ConfigValue.newBuilder()
-                    .addCoderUrn("beam:coder:string_utf8:v1")
-                    .setPayload(ByteString.copyFrom(encodeString(data)))
-                    .build())
+            .setSchema(SchemaTranslation.schemaToProto(configRow.getSchema(), false))
+            .setPayload(outputStream.toByteString())

Review comment:
       So this is a design decision?
   
   this configRow is a Row with a String field and the payload is already a string/byte array. At least in this case it seems that without a scheme that payload can still be constructed back to a Row. 
   
   
   The schema looks right in general when payload is not a string/byte array




----------------------------------------------------------------
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:
users@infra.apache.org