[GitHub] [beam] zhoufek commented on a diff in pull request #26311: BigTableReadSchemaTransform

["zhoufek (via GitHub)" <gi...@apache.org>]

zhoufek commented on code in PR #26311:
URL: https://github.com/apache/beam/pull/26311#discussion_r1170463886


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigTableReadSchemaTransformProvider.java:
##########
@@ -0,0 +1,226 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.bigtable;
+
+import static org.apache.beam.vendor.guava.v26_0_jre.com.google.common.base.Preconditions.checkArgument;
+
+import com.google.auto.service.AutoService;
+import com.google.auto.value.AutoValue;
+import com.google.bigtable.v2.Cell;
+import com.google.bigtable.v2.Column;
+import com.google.bigtable.v2.Family;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.beam.sdk.annotations.Experimental;
+import org.apache.beam.sdk.annotations.Experimental.Kind;
+import org.apache.beam.sdk.io.gcp.bigtable.BigTableReadSchemaTransformProvider.BigTableReadSchemaTransformConfiguration;
+import org.apache.beam.sdk.schemas.AutoValueSchema;
+import org.apache.beam.sdk.schemas.Schema;
+import org.apache.beam.sdk.schemas.annotations.DefaultSchema;
+import org.apache.beam.sdk.schemas.transforms.SchemaTransform;
+import org.apache.beam.sdk.schemas.transforms.SchemaTransformProvider;
+import org.apache.beam.sdk.schemas.transforms.TypedSchemaTransformProvider;
+import org.apache.beam.sdk.transforms.MapElements;
+import org.apache.beam.sdk.transforms.PTransform;
+import org.apache.beam.sdk.transforms.SimpleFunction;
+import org.apache.beam.sdk.values.PCollection;
+import org.apache.beam.sdk.values.PCollectionRowTuple;
+import org.apache.beam.sdk.values.Row;
+import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.base.Strings;
+
+/**
+ * An implementation of {@link TypedSchemaTransformProvider} for BigTable Read jobs configured via
+ * {@link BigTableReadSchemaTransformConfiguration}.
+ *
+ * <p><b>Internal only:</b> This class is actively being worked on, and it will likely change. We
+ * provide no backwards compatibility guarantees, and it should not be implemented outside the Beam
+ * repository.
+ */
+@Experimental(Kind.SCHEMAS)
+@AutoService(SchemaTransformProvider.class)
+public class BigTableReadSchemaTransformProvider
+    extends TypedSchemaTransformProvider<BigTableReadSchemaTransformConfiguration> {
+
+  private static final String OUTPUT_TAG = "output";
+
+  public static final Schema CELL_SCHEMA =
+      Schema.builder()
+          .addStringField("value")
+          .addInt64Field("timestamp")
+          .addArrayField("labels", Schema.FieldType.array(Schema.FieldType.STRING))
+          .build();
+
+  public static final Schema ROW_SCHEMA =
+      Schema.builder()
+          .addStringField("key")
+          .addMapField(
+              "families",
+              Schema.FieldType.STRING,
+              Schema.FieldType.map(
+                  Schema.FieldType.STRING,
+                  Schema.FieldType.array(Schema.FieldType.row(CELL_SCHEMA))))
+          .build();
+
+  @Override
+  protected Class<BigTableReadSchemaTransformConfiguration> configurationClass() {
+    return BigTableReadSchemaTransformConfiguration.class;
+  }
+
+  @Override
+  protected SchemaTransform from(BigTableReadSchemaTransformConfiguration configuration) {
+    return new BigTableReadSchemaTransform(configuration);
+  }
+
+  @Override
+  public String identifier() {
+    return "beam:schematransform:org.apache.beam:bigtable_read:v1";
+  }
+
+  @Override
+  public List<String> inputCollectionNames() {
+    return Collections.emptyList();
+  }
+
+  @Override
+  public List<String> outputCollectionNames() {
+    return Collections.singletonList(OUTPUT_TAG);
+  }
+
+  /** Configuration for reading from BigTable. */
+  @DefaultSchema(AutoValueSchema.class)
+  @AutoValue
+  public abstract static class BigTableReadSchemaTransformConfiguration {
+
+    public void validate() {

Review Comment:
   Something else you could do is validate the values in `build`: https://github.com/google/auto/blob/main/value/userguide/builders-howto.md#-validate-property-values
   
   I think this would be preferable, since it guarantees the object is in a valid state rather than having less clear ownership of validation and errors getting raised separate from where they're introduced.



##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigTableReadSchemaTransformProviderIT.java:
##########
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.bigtable;
+
+import static org.apache.beam.sdk.io.gcp.bigtable.BigTableReadSchemaTransformProvider.CELL_SCHEMA;
+import static org.apache.beam.sdk.io.gcp.bigtable.BigTableReadSchemaTransformProvider.ROW_SCHEMA;
+import static org.junit.Assert.assertThrows;
+
+import com.google.api.gax.rpc.NotFoundException;
+import com.google.cloud.bigtable.admin.v2.BigtableTableAdminClient;
+import com.google.cloud.bigtable.admin.v2.BigtableTableAdminSettings;
+import com.google.cloud.bigtable.admin.v2.models.CreateTableRequest;
+import com.google.cloud.bigtable.data.v2.BigtableDataClient;
+import com.google.cloud.bigtable.data.v2.BigtableDataSettings;
+import com.google.cloud.bigtable.data.v2.models.RowMutation;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.beam.sdk.extensions.gcp.options.GcpOptions;
+import org.apache.beam.sdk.io.gcp.bigtable.BigTableReadSchemaTransformProvider.BigTableReadSchemaTransformConfiguration;
+import org.apache.beam.sdk.schemas.transforms.SchemaTransform;
+import org.apache.beam.sdk.testing.PAssert;
+import org.apache.beam.sdk.testing.TestPipeline;
+import org.apache.beam.sdk.values.PCollection;
+import org.apache.beam.sdk.values.PCollectionRowTuple;
+import org.apache.beam.sdk.values.Row;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class BigTableReadSchemaTransformProviderIT {
+  @Rule public final transient TestPipeline p = TestPipeline.create();
+
+  private static final String COLUMN_FAMILY_NAME_1 = "test_cf_1";
+  private static final String COLUMN_FAMILY_NAME_2 = "test_cf_2";
+  private BigtableTableAdminClient adminClient;
+  private BigtableDataClient dataClient;
+  private String tableId;
+  private String projectId;
+  private String instanceId;
+
+  @Test
+  public void testInvalidConfigs() {

Review Comment:
   Is this test necessary? It looks like it's testing `@AutoValue` behavior, specifically that it doesn't allow null on fields not explicitly marked as nullable. 



##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigTableReadSchemaTransformProviderIT.java:
##########
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.bigtable;
+
+import static org.apache.beam.sdk.io.gcp.bigtable.BigTableReadSchemaTransformProvider.CELL_SCHEMA;
+import static org.apache.beam.sdk.io.gcp.bigtable.BigTableReadSchemaTransformProvider.ROW_SCHEMA;
+import static org.junit.Assert.assertThrows;
+
+import com.google.api.gax.rpc.NotFoundException;
+import com.google.cloud.bigtable.admin.v2.BigtableTableAdminClient;
+import com.google.cloud.bigtable.admin.v2.BigtableTableAdminSettings;
+import com.google.cloud.bigtable.admin.v2.models.CreateTableRequest;
+import com.google.cloud.bigtable.data.v2.BigtableDataClient;
+import com.google.cloud.bigtable.data.v2.BigtableDataSettings;
+import com.google.cloud.bigtable.data.v2.models.RowMutation;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.beam.sdk.extensions.gcp.options.GcpOptions;
+import org.apache.beam.sdk.io.gcp.bigtable.BigTableReadSchemaTransformProvider.BigTableReadSchemaTransformConfiguration;
+import org.apache.beam.sdk.schemas.transforms.SchemaTransform;
+import org.apache.beam.sdk.testing.PAssert;
+import org.apache.beam.sdk.testing.TestPipeline;
+import org.apache.beam.sdk.values.PCollection;
+import org.apache.beam.sdk.values.PCollectionRowTuple;
+import org.apache.beam.sdk.values.Row;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class BigTableReadSchemaTransformProviderIT {
+  @Rule public final transient TestPipeline p = TestPipeline.create();
+
+  private static final String COLUMN_FAMILY_NAME_1 = "test_cf_1";
+  private static final String COLUMN_FAMILY_NAME_2 = "test_cf_2";
+  private BigtableTableAdminClient adminClient;
+  private BigtableDataClient dataClient;
+  private String tableId;
+  private String projectId;
+  private String instanceId;
+
+  @Test
+  public void testInvalidConfigs() {
+    // All properties are required (project, instance, and table)
+    List<BigTableReadSchemaTransformConfiguration.Builder> invalidConfigs =
+        Arrays.asList(
+            BigTableReadSchemaTransformConfiguration.builder()
+                .setProject("project")
+                .setInstance("instance"),
+            BigTableReadSchemaTransformConfiguration.builder()
+                .setInstance("instance")
+                .setTable("table"),
+            BigTableReadSchemaTransformConfiguration.builder()
+                .setProject("project")
+                .setTable("table"));
+
+    for (BigTableReadSchemaTransformConfiguration.Builder config : invalidConfigs) {
+      assertThrows(
+          IllegalStateException.class,
+          () -> {
+            config.build();
+          });
+    }
+  }
+
+  @Before
+  public void setup() throws Exception {
+    BigtableTestOptions options =
+        TestPipeline.testingPipelineOptions().as(BigtableTestOptions.class);
+    projectId = options.as(GcpOptions.class).getProject();
+    instanceId = options.getInstanceId();
+
+    BigtableDataSettings settings =
+        BigtableDataSettings.newBuilder().setProjectId(projectId).setInstanceId(instanceId).build();
+    // Creates a bigtable data client.

Review Comment:
   nit: This comment is unnecessary. It's obvious what's happening from the code.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org