fix: Move ratio calculation for whether to use read API to avoid NPE …

…with setUseReadAPI(false) (#2509) Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [x] Make sure to open an issue as a [bug/issue](https://togithub.com/googleapis/java-bigquery/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [x] Ensure the tests and linter pass - [x] Code coverage does not decrease (if any source code was changed) - [x] Appropriate docs were updated (if necessary) Fixes #2508 ☕️
googleapis · May 4, 2023 · e1326c8 · e1326c8
1 parent 909a574
commit e1326c8
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 10 deletions.
diff --git a/README.md b/README.md
@@ -53,7 +53,7 @@ If you are using Maven without the BOM, add this to your dependencies:
 If you are using Gradle 5.x or later, add this to your dependencies:
 
 ```Groovy
-implementation platform('com.google.cloud:libraries-bom:26.13.0')
+implementation platform('com.google.cloud:libraries-bom:26.14.0')
 
 implementation 'com.google.cloud:google-cloud-bigquery'
 ```

diff --git a/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/ConnectionImpl.java b/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/ConnectionImpl.java
@@ -1203,22 +1203,18 @@ boolean isFastQuerySupported() {
 
   @VisibleForTesting
   boolean useReadAPI(Long totalRows, Long pageRows, Schema schema, Boolean hasQueryParameters) {
-    if ((totalRows == null || pageRows == null)
-        && Boolean.TRUE.equals(
-            connectionSettings
-                .getUseReadAPI())) { // totalRows and pageRows are returned null when the job is not
-      // complete
-      return true;
-    }
-
     // Read API does not yet support Interval Type or QueryParameters
     if (containsIntervalType(schema) || hasQueryParameters) {
       logger.log(Level.INFO, "\n Schema has IntervalType, or QueryParameters. Disabling ReadAPI");
       return false;
     }
 
-    long resultRatio = totalRows / pageRows;
+    if (totalRows == null || pageRows == null) {
+      return connectionSettings.getUseReadAPI();
+    }
+
     if (Boolean.TRUE.equals(connectionSettings.getUseReadAPI())) {
+      long resultRatio = totalRows / pageRows;
       return resultRatio >= connectionSettings.getTotalToPageRowCountRatio()
           && totalRows > connectionSettings.getMinResultSize();
     } else {

diff --git a/google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/ConnectionImplTest.java b/google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/ConnectionImplTest.java
@@ -76,6 +76,12 @@ public class ConnectionImplTest {
           Field.newBuilder("state_name", StandardSQLTypeName.STRING)
               .setMode(Field.Mode.NULLABLE)
               .build());
+
+  private static final Schema QUERY_SCHEMA_WITH_INTERVAL_FIELD =
+      Schema.of(
+          Field.newBuilder("interval", StandardSQLTypeName.INTERVAL)
+              .setMode(Field.Mode.NULLABLE)
+              .build());
   private static final TableSchema FAST_QUERY_TABLESCHEMA = QUERY_SCHEMA.toPb();
   private static final BigQueryResult BQ_RS_MOCK_RES =
       new BigQueryResultImpl(QUERY_SCHEMA, 2, null, null);
@@ -661,6 +667,32 @@ public void testGetSubsequentQueryResultsWithJob() {
         .getSubsequentQueryResultsWithJob(10000L, 100L, jobId, GET_QUERY_RESULTS_RESPONSE, false);
   }
 
+  @Test
+  public void testUseReadApi() {
+    ConnectionSettings connectionSettingsSpy = Mockito.spy(ConnectionSettings.class);
+    doReturn(true).when(connectionSettingsSpy).getUseReadAPI();
+    doReturn(2).when(connectionSettingsSpy).getTotalToPageRowCountRatio();
+    doReturn(100).when(connectionSettingsSpy).getMinResultSize();
+
+    connection = (ConnectionImpl) bigquery.createConnection(connectionSettingsSpy);
+
+    // defaults to connectionSettings.getUseReadAPI() when total/page rows are null (job is still
+    // running)
+    assertTrue(connection.useReadAPI(null, null, QUERY_SCHEMA, false));
+
+    assertFalse(connection.useReadAPI(10000L, 10000L, QUERY_SCHEMA, false));
+    assertFalse(connection.useReadAPI(50L, 10L, QUERY_SCHEMA, false));
+    assertTrue(connection.useReadAPI(10000L, 10L, QUERY_SCHEMA, false));
+
+    // interval and query parameters not supported
+    assertFalse(connection.useReadAPI(10000L, 10L, QUERY_SCHEMA_WITH_INTERVAL_FIELD, false));
+    assertFalse(connection.useReadAPI(10000L, 10L, QUERY_SCHEMA, true));
+
+    doReturn(false).when(connectionSettingsSpy).getUseReadAPI();
+    assertFalse(connection.useReadAPI(null, null, QUERY_SCHEMA, false));
+    assertFalse(connection.useReadAPI(10000L, 10L, QUERY_SCHEMA, false));
+  }
+
   @Test
   public void testGetPageCacheSize() {
     ConnectionImpl connectionSpy = Mockito.spy(connection);