[GitHub] [beam] chamikaramj commented on a change in pull request #14238: [BEAM-7195] BQ BatchLoads doesn't always create new tables

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

chamikaramj commented on a change in pull request #14238:
URL: https://github.com/apache/beam/pull/14238#discussion_r602943526



##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/WriteTables.java
##########
@@ -88,8 +98,35 @@
 })
 class WriteTables<DestinationT>
     extends PTransform<
-        PCollection<KV<ShardedKey<DestinationT>, List<String>>>,
-        PCollection<KV<TableDestination, String>>> {
+        PCollection<KV<ShardedKey<DestinationT>, WritePartition.Result>>,
+        PCollection<KV<TableDestination, WriteTables.Result>>> {
+  @AutoValue
+  abstract static class Result {
+    abstract String getTableName();
+
+    abstract Boolean isFirstPane();
+  }
+
+  static class ResultCoder extends AtomicCoder<WriteTables.Result> {

Review comment:
       Did you mean to redefine Result and ResultCoder classes here ? They are Already defined in the WritePartition class and definitions seems to be identical. Can we move these to new files and re-use ?

##########
File path: sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOWriteTest.java
##########
@@ -65,19 +65,24 @@
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;

Review comment:
       Is it possible to add a new unit test that would break for the previous implementation (due to pane numbers being incorrect) but would pass for the new implementation ? 




-- 
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