fix: Move ratio calculation for whether to use read API to avoid NPE with setUseReadAPI(false)

[jonathanswenson]

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:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #2508 ☕️

[conventional-commit-lint-gcf]

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

[Neenu1995]

@prash-mi PTAL. Thank you .

[prash-mi]

Thanks for raising the PR.
NPE shouldn't ideally come as we had made getQueryResultFirstPage(...) a blocking call(Ref: https://github.com/googleapis/java-bigquery/blob/main/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/ConnectionImpl.java#L243), such that it should not return unless the job is complete, which implies that we must be getting totalRows and pageRows back.
Still we can use a null check IMO
Do we want to replace

if ((totalRows == null || pageRows == null)
        && Boolean.TRUE.equals(
            connectionSettings
                .getUseReadAPI())) { // totalRows and pageRows are returned null when the job is not
      // complete
      return true;
    }

with

if (totalRows == null || pageRows == null) { 
      return false;
    }

as, we can't check the ration if any one of these attributes are null . WDYT?

[jonathanswenson]

@prash-mi That sounds reasonable to me, I have made that additional change. Let me know what you think.

[prash-mi]

thanks for making the changes, LGTM

[gcf-merge-on-green]

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

[obada-ab]

@jonathanswenson thanks for making the changes, it's better to add unit tests to the changes to ensure that the NPE would be avoided

[obada-ab]

@prash-mi there's a behaviour change as a side effect of this change that I'm worried about. Specifically, when either totalRows or pageRows are null (due to the job taking a long time) and setUseReadAPI(true) is used in the connection settings.

The current behaviour would be to use the Read API, but after introducing this fix such queries would always use tabledata.list.

If you believe that this change makes sense, then we can unit test and merge it. Otherwise maybe the following change might be reasonable:

    // 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;
    }

    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 {
      return false;
    }

This would avoid the NPE while maintaining the current behaviour.

[obada-ab]

Added mentioned change alongside unit testing. I don't think new integration tests are useful, but it might be worth it to run the benchmark queries to ensure long-running jobs are working fine, and that their execution time hasn't gotten worse. Will work on that.

[prash-mi]

@obada-ab I think returning connectionSettings.getUseReadAPI() if totalRows or pageRows is null is the right thing to do.
Thanks for making changes, LGTM!

[obada-ab]

I ran the benchmark queries locally against my test project. The third run of the queries in main gave these results:

query "SELECT * FROM `nyc-tlc.yellow.trips` LIMIT 10000": read 10000 rows, 19 cols, first byte 3.907000 sec, total 3.910000 sec
query "SELECT * FROM `nyc-tlc.yellow.trips` LIMIT 100000": read 100000 rows, 19 cols, first byte 5.695000 sec, total 25.436000 sec
query "SELECT * FROM `nyc-tlc.yellow.trips` LIMIT 1000000": read 1000000 rows, 19 cols, first byte 6.617000 sec, total 239.012000 sec
query "SELECT title FROM `bigquery-public-data.samples.wikipedia` ORDER BY title LIMIT 1000": read 1000 rows, 1 cols, first byte 0.664000 sec, total 0.664000 sec
query "SELECT title, id, timestamp, contributor_ip FROM `bigquery-public-data.samples.wikipedia` WHERE title like 'Blo%' ORDER BY id": read 193029 rows, 4 cols, first byte 7.252000 sec, total 13.599000 sec
query "SELECT * FROM `bigquery-public-data.baseball.games_post_wide` ORDER BY gameId": read 8676 rows, 145 cols, first byte 4.839000 sec, total 11.641000 sec
query "SELECT * FROM `bigquery-public-data.samples.github_nested` WHERE repository.has_downloads ORDER BY repository.created_at LIMIT 10000": read 10000 rows, 8 cols, first byte 7.865000 sec, total 12.966000 sec
query "SELECT repo_name, path FROM `bigquery-public-data.github_repos.files` WHERE path LIKE '%.java' ORDER BY id LIMIT 1000000": read 1000000 rows, 2 cols, first byte 12.123000 sec, total 110.380000 sec

The third run in fix-read-query-npe gave these results:

query "SELECT * FROM `nyc-tlc.yellow.trips` LIMIT 10000": read 10000 rows, 19 cols, first byte 4.089000 sec, total 4.094000 sec
query "SELECT * FROM `nyc-tlc.yellow.trips` LIMIT 100000": read 100000 rows, 19 cols, first byte 5.584000 sec, total 25.566000 sec
query "SELECT * FROM `nyc-tlc.yellow.trips` LIMIT 1000000": read 1000000 rows, 19 cols, first byte 5.847000 sec, total 238.485000 sec
query "SELECT title FROM `bigquery-public-data.samples.wikipedia` ORDER BY title LIMIT 1000": read 1000 rows, 1 cols, first byte 0.602000 sec, total 0.603000 sec
query "SELECT title, id, timestamp, contributor_ip FROM `bigquery-public-data.samples.wikipedia` WHERE title like 'Blo%' ORDER BY id": read 193029 rows, 4 cols, first byte 6.885000 sec, total 13.119000 sec
query "SELECT * FROM `bigquery-public-data.baseball.games_post_wide` ORDER BY gameId": read 8676 rows, 145 cols, first byte 4.057000 sec, total 10.648000 sec
query "SELECT * FROM `bigquery-public-data.samples.github_nested` WHERE repository.has_downloads ORDER BY repository.created_at LIMIT 10000": read 10000 rows, 8 cols, first byte 8.350000 sec, total 13.840000 sec
query "SELECT repo_name, path FROM `bigquery-public-data.github_repos.files` WHERE path LIKE '%.java' ORDER BY id LIMIT 1000000": read 1000000 rows, 2 cols, first byte 11.803000 sec, total 107.445000 sec

Nothing concerning IMO, the differences seem minor and are probably just due to variance between runs.