Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Long term fix & redesign of executeSelect #2240

Closed
prash-mi opened this issue Aug 19, 2022 · 2 comments · Fixed by #2294
Closed

Long term fix & redesign of executeSelect #2240

prash-mi opened this issue Aug 19, 2022 · 2 comments · Fixed by #2294
Assignees
Labels
api: bigquery Issues related to the googleapis/java-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@prash-mi
Copy link
Contributor

prash-mi commented Aug 19, 2022

b/241134681 's short term fix #2222 uses polling to fix the table_not_found issue.
As a long term cleaner solution & redesign we can explore:

  1. Providing an API Future based async implementation which can offer more control to the users (in terms of cancelling the operation and checking the status)
  2. Removing patch of logic related with queryDryRun which wont be necessary after this fix
  3. See if we can optimise the fast and slow query paths. The current flow is a bit convoluted . Refactor the overloaded executeSelect methods to avoid code duplicity.
  4. A similar error can probably occur in the fast query path if we get an incomplete job id from the backend and Read API is initialised on the basis of the response from useReadAPI
  5. Exploring a JDBCish overloaded method [if we redesign executeSelect to return APIFuture, then we may want to have a synchronous counterpart like executeQuery as well to comply with JDBC standards]
  6. Add user configurable timeout for getQueryResults RPC (it can be in the range 1 - 200 secs . Ref https://cloud.google.com/bigquery/docs/reference/rest/v2/jobs/getQueryResults)
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/java-bigquery API. label Aug 19, 2022
@prash-mi prash-mi self-assigned this Aug 19, 2022
@prash-mi prash-mi added priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed api: bigquery Issues related to the googleapis/java-bigquery API. labels Aug 19, 2022
@shollyman
Copy link
Contributor

These seem reasonable. The async design provides for some interesting permutations:

  • return a future that returns the tableresult on completion, and optionally make it easy to wire job cancellation in.
  • refactor TableResult so that it becomes the future

The thing to keep in mind when designing is what you want for queries that return no results, and for queries (scripts) that may return multiple result sets.

RE: 5 - The original design was for there to be three components: executeSelect, executeUpdate, and execute. My expectation is that JDBC would map to the third method, as it represents the query method where the content is ambiguous.

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/java-bigquery API. label Aug 20, 2022
@prash-mi
Copy link
Contributor Author

These seem reasonable. The async design provides for some interesting permutations:

  • return a future that returns the tableresult on completion, and optionally make it easy to wire job cancellation in.
  • refactor TableResult so that it becomes the future

The thing to keep in mind when designing is what you want for queries that return no results, and for queries (scripts) that may return multiple result sets.

RE: 5 - The original design was for there to be three components: executeSelect, executeUpdate, and execute. My expectation is that JDBC would map to the third method, as it represents the query method where the content is ambiguous.

Thanks for taking a look @shollyman . I will put together the async design while trying to accommodate the mentioned permutations and get back.
Regarding point#5 my initial thought was to have one synchronous method executeQuery which will return ResultSet as our current executeSelect does. We can then modify executeSelect to return a Future. This reason for this is executeQuery is a commonly used method in JDBC. WDYT?

@prash-mi prash-mi changed the title Long term fix for executeSelect's table_not_found issue Long term fix & redesign of executeSelect Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/java-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants