ccciudatu commented on a change in pull request #13572:
URL: https://github.com/apache/beam/pull/13572#discussion_r546387329



##########
File path: 
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/kafka/BeamKafkaThriftTable.java
##########
@@ -74,55 +92,81 @@ private static Schema thriftSchema(
 
   @Override
   protected PTransform<PCollection<KV<byte[], byte[]>>, PCollection<Row>> 
getPTransformForInput() {
-    final @NonNull SchemaProvider schemaProvider = ThriftSchema.provider();
-    return new PTransform<PCollection<KV<byte[], byte[]>>, PCollection<Row>>() 
{
-      @Override
-      @SuppressWarnings("nullness")
-      public PCollection<Row> expand(PCollection<KV<byte[], byte[]>> input) {
-        return input
-            .apply(Values.create())
-            
.apply(MapElements.into(typeDescriptor).via(BeamKafkaThriftTable.this::decode))
-            .setSchema(
-                schema,
-                typeDescriptor,
-                schemaProvider.toRowFunction(typeDescriptor),
-                schemaProvider.fromRowFunction(typeDescriptor))
-            .apply(Convert.toRows());
-      }
-    };
+    return new InputTransformer(typeDescriptor, coder, schema);
   }
 
-  private T decode(byte[] bytes) {
-    try {
-      return thriftCoder.decode(new ByteArrayInputStream(bytes));
-    } catch (IOException e) {
-      throw new IllegalStateException(e);
+  private static class InputTransformer<T extends TBase<?, ?>>

Review comment:
       Got it, makes sense. I tried to solve it locally for Kafka, as I think 
making this truly reusable/composable requires a larger refactoring that should 
follow up. But those functions do seem to rather belong in the `ThriftSchema` 
anyway, so I'll fix 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.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to