Re: [PR] Add Cloud Spanner Directed Reads support to SpannerIO [beam]

[via GitHub]

gemini-code-assist[bot] commented on code in PR #39241:
URL: https://github.com/apache/beam/pull/39241#discussion_r3541883705


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -638,6 +639,24 @@ public ReadAll withExperimentalHost(String 
experimentalHost) {
       return 
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
     }
 
+    /** Specifies the directed read options for Cloud Spanner. */
+    public ReadAll withDirectedReadOptions(DirectedReadOptions 
directedReadOptions) {
+      SpannerConfig config = getSpannerConfig();
+      return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
+    }
+
+    /** Specifies the directed read options for Cloud Spanner. */
+    public ReadAll withDirectedReadOptions(ValueProvider<DirectedReadOptions> 
directedReadOptions) {
+      SpannerConfig config = getSpannerConfig();
+      return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
+    }
+
+    /** Specifies the directed read options for Cloud Spanner from a string 
representation. */
+    public ReadAll withDirectedReadOptions(String directedReadOptions) {
+      SpannerConfig config = getSpannerConfig();
+      return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
+    }

Review Comment:
   ![high](https://www.gstatic.com/codereviewagent/high-priority.svg)
   
   If `getSpannerConfig()` returns `null` (which is the default when a pipeline 
developer initializes `SpannerIO.readAll()` without configuring it first), 
calling `config.withDirectedReadOptions(...)` will throw a 
`NullPointerException`. We should initialize `config` with 
`SpannerConfig.create()` if it is `null` to prevent this.
   
   ```java
       /** Specifies the directed read options for Cloud Spanner. */
       public ReadAll withDirectedReadOptions(DirectedReadOptions 
directedReadOptions) {
         SpannerConfig config = getSpannerConfig();
         if (config == null) {
           config = SpannerConfig.create();
         }
         return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
       }
   
       /** Specifies the directed read options for Cloud Spanner. */
       public ReadAll 
withDirectedReadOptions(ValueProvider<DirectedReadOptions> directedReadOptions) 
{
         SpannerConfig config = getSpannerConfig();
         if (config == null) {
           config = SpannerConfig.create();
         }
         return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
       }
   
       /** Specifies the directed read options for Cloud Spanner from a string 
representation. */
       public ReadAll withDirectedReadOptions(String directedReadOptions) {
         SpannerConfig config = getSpannerConfig();
         if (config == null) {
           config = SpannerConfig.create();
         }
         return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
       }
   ```



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -927,6 +946,24 @@ public Read withExperimentalHost(String experimentalHost) {
       return 
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
     }
 
+    /** Specifies the directed read options for Cloud Spanner. */
+    public Read withDirectedReadOptions(DirectedReadOptions 
directedReadOptions) {
+      SpannerConfig config = getSpannerConfig();
+      return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
+    }
+
+    /** Specifies the directed read options for Cloud Spanner. */
+    public Read withDirectedReadOptions(ValueProvider<DirectedReadOptions> 
directedReadOptions) {
+      SpannerConfig config = getSpannerConfig();
+      return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
+    }
+
+    /** Specifies the directed read options for Cloud Spanner from a string 
representation. */
+    public Read withDirectedReadOptions(String directedReadOptions) {
+      SpannerConfig config = getSpannerConfig();
+      return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
+    }

Review Comment:
   ![high](https://www.gstatic.com/codereviewagent/high-priority.svg)
   
   If `getSpannerConfig()` returns `null` (which is the default when a pipeline 
developer initializes `SpannerIO.read()` without configuring it first), calling 
`config.withDirectedReadOptions(...)` will throw a `NullPointerException`. We 
should initialize `config` with `SpannerConfig.create()` if it is `null` to 
prevent this.
   
   ```java
       /** Specifies the directed read options for Cloud Spanner. */
       public Read withDirectedReadOptions(DirectedReadOptions 
directedReadOptions) {
         SpannerConfig config = getSpannerConfig();
         if (config == null) {
           config = SpannerConfig.create();
         }
         return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
       }
   
       /** Specifies the directed read options for Cloud Spanner. */
       public Read withDirectedReadOptions(ValueProvider<DirectedReadOptions> 
directedReadOptions) {
         SpannerConfig config = getSpannerConfig();
         if (config == null) {
           config = SpannerConfig.create();
         }
         return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
       }
   
       /** Specifies the directed read options for Cloud Spanner from a string 
representation. */
       public Read withDirectedReadOptions(String directedReadOptions) {
         SpannerConfig config = getSpannerConfig();
         if (config == null) {
           config = SpannerConfig.create();
         }
         return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
       }
   ```



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -2052,6 +2089,28 @@ public ReadChangeStream withExperimentalHost(String 
experimentalHost) {
       return 
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
     }
 
+    /** Specifies the directed read options for change stream queries. */
+    public ReadChangeStream withDirectedReadOptions(DirectedReadOptions 
directedReadOptions) {
+      SpannerConfig config = getSpannerConfig();
+      return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
+    }
+
+    /** Specifies the directed read options for change stream queries. */
+    public ReadChangeStream withDirectedReadOptions(
+        ValueProvider<DirectedReadOptions> directedReadOptions) {
+      SpannerConfig config = getSpannerConfig();
+      return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
+    }
+
+    /**
+     * Specifies the directed read options for change stream queries from a 
string representation
+     * (e.g., JSON string or "us-central1:READ_ONLY").
+     */
+    public ReadChangeStream withDirectedReadOptions(String 
directedReadOptions) {
+      SpannerConfig config = getSpannerConfig();
+      return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
+    }

Review Comment:
   ![high](https://www.gstatic.com/codereviewagent/high-priority.svg)
   
   If `getSpannerConfig()` returns `null` (which is the default when a pipeline 
developer initializes `SpannerIO.readChangeStream()` without configuring it 
first), calling `config.withDirectedReadOptions(...)` will throw a 
`NullPointerException`. We should initialize `config` with 
`SpannerConfig.create()` if it is `null` to prevent this.
   
   ```java
       /** Specifies the directed read options for change stream queries. */
       public ReadChangeStream withDirectedReadOptions(DirectedReadOptions 
directedReadOptions) {
         SpannerConfig config = getSpannerConfig();
         if (config == null) {
           config = SpannerConfig.create();
         }
         return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
       }
   
       /** Specifies the directed read options for change stream queries. */
       public ReadChangeStream withDirectedReadOptions(
           ValueProvider<DirectedReadOptions> directedReadOptions) {
         SpannerConfig config = getSpannerConfig();
         if (config == null) {
           config = SpannerConfig.create();
         }
         return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
       }
   
       /**
        * Specifies the directed read options for change stream queries from a 
string representation
        * (e.g., JSON string or "us-central1:READ_ONLY").
        */
       public ReadChangeStream withDirectedReadOptions(String 
directedReadOptions) {
         SpannerConfig config = getSpannerConfig();
         if (config == null) {
           config = SpannerConfig.create();
         }
         return 
withSpannerConfig(config.withDirectedReadOptions(directedReadOptions));
       }
   ```



-- 
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-unsubscr...@beam.apache.org

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