-
Notifications
You must be signed in to change notification settings - Fork 213
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
refactor: replaces job#getQueryResults with table#getRows in query method #454
Conversation
Codecov Report
@@ Coverage Diff @@
## master #454 +/- ##
==========================================
- Coverage 99.42% 97.74% -1.69%
==========================================
Files 5 5
Lines 699 708 +9
Branches 194 196 +2
==========================================
- Hits 695 692 -3
- Misses 2 12 +10
- Partials 2 4 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #454 +/- ##
==========================================
+ Coverage 99.42% 99.57% +0.14%
==========================================
Files 5 5
Lines 698 708 +10
Branches 192 193 +1
==========================================
+ Hits 694 705 +11
+ Misses 3 1 -2
- Partials 1 2 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @steffnay, just came across this while performing triage (I'm not a subject expert in bigquery, so please take my comments with a grain of salt 😛).
This is looking reasonable to me.
src/index.ts
Outdated
// table#getRows uses listTableData endpoint, which is a faster method | ||
// to read rows of the results. However, it won't work for model queries, | ||
// so use the original job#getQueryResults for model queries. | ||
const modelQueryRegex = new RegExp('\\b((create|replace) model)\\b', 'i'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would pull this regex into a constant outside of the method, similar to the approach used in tests, e.g.,
const MODEL_QUERY_STRING = '\\b((create|replace) model)\\b', 'i'
I don't know this problem space too well, but I wonder if it might be worth tightening up the regex a bit. What would happen if someone performed a query asking with a variable create
rather than a statement, e.g.,
SELECT * FROM foo WHERE operation = 'create'
| QueryRowsCallback, | ||
cb?: SimpleQueryRowsCallback | QueryRowsCallback | ||
): void | Promise<SimpleQueryRowsResponse> | Promise<QueryRowsResponse> { | ||
cb?: SimpleQueryRowsCallback | RowsCallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It surprised me that QueryRowsCallback
had to change RowsCallback
, since in the case of model queries we still return the original response:
job!.getQueryResults(options, callback as QueryRowsCallback);
☝️ this is the original slow path right? which we returned as a QueryRowsCallback
?
Mainly just making sure code-paths return the same shaped object.
// table#getRows uses listTableData endpoint, which is a faster method | ||
// to read rows of results. | ||
|
||
job!.getQueryResults(options, (err, rows) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So getQueryResults
is going to get called continuously until the job is done and all the rows have been fetched. That doesn't sound like something we want here, so we'll probably need to set autoPaginate
to false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I pieced this together, I have a feeling we are going to need to manually poll this method until we see the jobComplete
value set to true
. If there are 0 rows, we execute the users callback with an empty array and the response object. If a page was returned, then we should call listTableData
.
@shollyman does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Typical flow for query processing should look something like this in terms of backend interactions:
-
Call bigquery.jobs.insert() to insert the query job into the system,
which returns a job resource (and job reference) for future polling
operations. -
Call bigquery.jobs.getQueryResults to poll (possibly repeatedly) for
job completion. Typically you do this with a nonzero timeoutMs param (which
moves polling to the server side as a hanging get), and zero maxRows
param (as we're not consuming row data here). -
When job is complete, you can consume schema from the getQueryResults,
and then setup the node equivalent of a row iterator if the getQueryResults
response indicates rows were returned. Many queries don't return results,
such as DML or DDL operations (UPDATE TABLE, ALTER TABLE, etc). -
Row iteration should use bigquery.tabledata.list to fetch pages of row data.
Each page optionally returns a next page token, which is passed to the next
invocation to advance the cursor (don't use explicit row offsets) until no further
page token is provided.
|
||
job!.getQueryResults(options, (err, rows) => { | ||
if (!err) { | ||
if (rows!.length !== 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This throws me off a little bit, does this imply that the best way to go is to poll the getQueryResults
API until we get a single page of results and only then should we call listTableData
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what Seth told me they do in the other languages and how I should handle it to be consistent.
const tableId = job!.metadata.configuration.query.destinationTable | ||
.tableId; | ||
const dataset = this.dataset(datasetId); | ||
const table = dataset.table(tableId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're probably going to need to cache the destinationTable
in case users don't want to autoPaginate
the results
googleapis/synthtool@6a17abc commit 6a17abc7652e2fe563e1288c6e8c23fc260dda97 Author: Benjamin E. Coe <bencoe@google.com> Date: Mon Mar 23 19:22:34 2020 -0700 docs: document the release schedule we follow (#454)
* Change triggered by none of the following: This git repo (https://github.com/googleapis/nodejs-bigquery.git) Git repo https://github.com/googleapis/synthtool.git * feat!: drop Node 8 from engines field (#662) 712b029 commit 712b029 Author: Steffany Brown <30247553+steffnay@users.noreply.github.com> Date: Mon Mar 30 12:59:52 2020 -0700 feat!: drop Node 8 from engines field (#662) Drops Node 8 from the engines field. * refactor!: don't return Stream from createLoadJob (#647) 8e26fb5 commit 8e26fb5 Author: Andrew Zammit <zammit.andrew@gmail.com> Date: Mon Mar 30 21:58:06 2020 -0700 refactor!: don't return Stream from createLoadJob (#647) * fix!(table): createLoadJobStream sync returns a stream, createLoadJob always returns a job #640 * chore(table): remove createLoadJobStream, createLoadJob test refactor for promises #640 * chore(table): remove never encountered callback noop in createLoadJob given promisifyAll * test(biqquery): add tests to increase codecov as a result of #647 refactor Co-authored-by: Benjamin E. Coe <bencoe@google.com> Co-authored-by: Steffany Brown <30247553+steffnay@users.noreply.github.com> * chore: update dependency @google-cloud/common to v3 (#661) c61407e commit c61407e Author: WhiteSource Renovate <bot@renovateapp.com> Date: Tue Mar 31 19:27:51 2020 +0200 chore: update dependency @google-cloud/common to v3 (#661) * fix(deps): update dependency @google-cloud/paginator to v3 (#658) a09c493 commit a09c493 Author: WhiteSource Renovate <bot@renovateapp.com> Date: Tue Mar 31 19:38:07 2020 +0200 fix(deps): update dependency @google-cloud/paginator to v3 (#658) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@google-cloud/paginator](https://github.com/googleapis/nodejs-paginator) | dependencies | major | [`^2.0.0` -> `^3.0.0`](https://renovatebot.com/diffs/npm/@google-cloud%2fpaginator/2.0.3/3.0.0) | --- ### Release Notes <details> <summary>googleapis/nodejs-paginator</summary> ### [`v3.0.0`](https://github.com/googleapis/nodejs-paginator/blob/master/CHANGELOG.md#​300-httpswwwgithubcomgoogleapisnodejs-paginatorcomparev203v300-2020-03-25) [Compare Source](https://github.com/googleapis/nodejs-paginator/compare/v2.0.3...v3.0.0) ##### ⚠ BREAKING CHANGES - **dep:** upgrade gts 2.0.0 ([#​194](https://github.com/googleapis/nodejs-paginator/issues/194)) - **deps:** deprecated node 8 to 10; upgrade typescript ##### Miscellaneous Chores - **dep:** upgrade gts 2.0.0 ([#​194](https://www.github.com/googleapis/nodejs-paginator/issues/194)) ([4eaf9be](https://www.github.com/googleapis/nodejs-paginator/commit/4eaf9bed1fcfd0f10e877ff15c1d0e968e3356c8)) - **deps:** deprecated node 8 to 10; upgrade typescript ([f6434ab](https://www.github.com/googleapis/nodejs-paginator/commit/f6434ab9cacb6ab804c070f19c38b6072ca326b5)) ##### [2.0.3](https://www.github.com/googleapis/nodejs-paginator/compare/v2.0.2...v2.0.3) (2019-12-05) ##### Bug Fixes - **deps:** pin TypeScript below 3.7.0 ([e06e1b0](https://www.github.com/googleapis/nodejs-paginator/commit/e06e1b0a2e2bb1cf56fc806c1703b8b5e468b954)) ##### [2.0.2](https://www.github.com/googleapis/nodejs-paginator/compare/v2.0.1...v2.0.2) (2019-11-13) ##### Bug Fixes - **docs:** add jsdoc-region-tag plugin ([#​155](https://www.github.com/googleapis/nodejs-paginator/issues/155)) ([b983799](https://www.github.com/googleapis/nodejs-paginator/commit/b98379905848fd179c6268aff3e1cfaf2bf76663)) ##### [2.0.1](https://www.github.com/googleapis/nodejs-paginator/compare/v2.0.0...v2.0.1) (2019-08-25) ##### Bug Fixes - **deps:** use the latest extend ([#​141](https://www.github.com/googleapis/nodejs-paginator/issues/141)) ([61b383e](https://www.github.com/googleapis/nodejs-paginator/commit/61b383e)) </details> --- ### Renovate configuration 📅 **Schedule**: "after 9am and before 3pm" (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻️ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-bigquery). * fix(deps): update dependency @google-cloud/promisify to v2 (#657) 5d8112c commit 5d8112c Author: WhiteSource Renovate <bot@renovateapp.com> Date: Tue Mar 31 19:48:07 2020 +0200 fix(deps): update dependency @google-cloud/promisify to v2 (#657) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@google-cloud/promisify](https://github.com/googleapis/nodejs-promisify) | dependencies | major | [`^1.0.0` -> `^2.0.0`](https://renovatebot.com/diffs/npm/@google-cloud%2fpromisify/1.0.4/2.0.0) | --- ### Release Notes <details> <summary>googleapis/nodejs-promisify</summary> ### [`v2.0.0`](https://github.com/googleapis/nodejs-promisify/blob/master/CHANGELOG.md#​200-httpswwwgithubcomgoogleapisnodejs-promisifycomparev104v200-2020-03-23) [Compare Source](https://github.com/googleapis/nodejs-promisify/compare/v1.0.4...v2.0.0) ##### ⚠ BREAKING CHANGES - update to latest version of gts/typescript ([#​183](https://github.com/googleapis/nodejs-promisify/issues/183)) - drop Node 8 from engines field ([#​184](https://github.com/googleapis/nodejs-promisify/issues/184)) ##### Features - drop Node 8 from engines field ([#​184](https://www.github.com/googleapis/nodejs-promisify/issues/184)) ([7e6d3c5](https://www.github.com/googleapis/nodejs-promisify/commit/7e6d3c54066d89530ed25c7f9722efd252f43fb8)) ##### Build System - update to latest version of gts/typescript ([#​183](https://www.github.com/googleapis/nodejs-promisify/issues/183)) ([9c3ed12](https://www.github.com/googleapis/nodejs-promisify/commit/9c3ed12c12f4bb1e17af7440c6371c4cefddcd59)) ##### [1.0.4](https://www.github.com/googleapis/nodejs-promisify/compare/v1.0.3...v1.0.4) (2019-12-05) ##### Bug Fixes - **deps:** pin TypeScript below 3.7.0 ([e48750e](https://www.github.com/googleapis/nodejs-promisify/commit/e48750ef96aa20eb3a2b73fe2f062d04430468a7)) ##### [1.0.3](https://www.github.com/googleapis/nodejs-promisify/compare/v1.0.2...v1.0.3) (2019-11-13) ##### Bug Fixes - **docs:** add jsdoc-region-tag plugin ([#​146](https://www.github.com/googleapis/nodejs-promisify/issues/146)) ([ff0ee74](https://www.github.com/googleapis/nodejs-promisify/commit/ff0ee7408f50e8f7147b8ccf7e10337aa5920076)) ##### [1.0.2](https://www.github.com/googleapis/nodejs-promisify/compare/v1.0.1...v1.0.2) (2019-06-26) ##### Bug Fixes - **docs:** link to reference docs section on googleapis.dev ([#​128](https://www.github.com/googleapis/nodejs-promisify/issues/128)) ([5a8bd90](https://www.github.com/googleapis/nodejs-promisify/commit/5a8bd90)) ##### [1.0.1](https://www.github.com/googleapis/nodejs-promisify/compare/v1.0.0...v1.0.1) (2019-06-14) ##### Bug Fixes - **docs:** move to new client docs URL ([#​124](https://www.github.com/googleapis/nodejs-promisify/issues/124)) ([34d18cd](https://www.github.com/googleapis/nodejs-promisify/commit/34d18cd)) </details> --- ### Renovate configuration 📅 **Schedule**: "after 9am and before 3pm" (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻️ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-bigquery). * fix(deps): update dependency google-auth-library to v6 (#660) 3ea642e commit 3ea642e Author: WhiteSource Renovate <bot@renovateapp.com> Date: Tue Mar 31 19:58:07 2020 +0200 fix(deps): update dependency google-auth-library to v6 (#660) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [google-auth-library](https://github.com/googleapis/google-auth-library-nodejs) | dependencies | major | [`^5.8.0` -> `^6.0.0`](https://renovatebot.com/diffs/npm/google-auth-library/5.10.1/6.0.0) | --- ### Release Notes <details> <summary>googleapis/google-auth-library-nodejs</summary> ### [`v6.0.0`](https://github.com/googleapis/google-auth-library-nodejs/blob/master/CHANGELOG.md#​600-httpswwwgithubcomgoogleapisgoogle-auth-library-nodejscomparev5101v600-2020-03-26) [Compare Source](https://github.com/googleapis/google-auth-library-nodejs/compare/v5.10.1...v6.0.0) ##### ⚠ BREAKING CHANGES - typescript@3.7.x introduced some breaking changes in generated code. - require node 10 in engines field ([#​926](https://github.com/googleapis/google-auth-library-nodejs/issues/926)) - remove deprecated methods ([#​906](https://github.com/googleapis/google-auth-library-nodejs/issues/906)) ##### Features - require node 10 in engines field ([#​926](https://www.github.com/googleapis/google-auth-library-nodejs/issues/926)) ([d89c59a](https://www.github.com/googleapis/google-auth-library-nodejs/commit/d89c59a316e9ca5b8c351128ee3e2d91e9729d5c)) ##### Bug Fixes - do not warn for SDK creds ([#​905](https://www.github.com/googleapis/google-auth-library-nodejs/issues/905)) ([9536840](https://www.github.com/googleapis/google-auth-library-nodejs/commit/9536840f88e77f747bbbc2c1b5b4289018fc23c9)) - use iamcredentials API to sign blobs ([#​908](https://www.github.com/googleapis/google-auth-library-nodejs/issues/908)) ([7b8e4c5](https://www.github.com/googleapis/google-auth-library-nodejs/commit/7b8e4c52e31bb3d448c3ff8c05002188900eaa04)) - **deps:** update dependency gaxios to v3 ([#​917](https://www.github.com/googleapis/google-auth-library-nodejs/issues/917)) ([1f4bf61](https://www.github.com/googleapis/google-auth-library-nodejs/commit/1f4bf6128a0dcf22cfe1ec492b2192f513836cb2)) - **deps:** update dependency gcp-metadata to v4 ([#​918](https://www.github.com/googleapis/google-auth-library-nodejs/issues/918)) ([d337131](https://www.github.com/googleapis/google-auth-library-nodejs/commit/d337131d009cc1f8182f7a1f8a9034433ee3fbf7)) - **types:** add additional fields to TokenInfo ([#​907](https://www.github.com/googleapis/google-auth-library-nodejs/issues/907)) ([5b48eb8](https://www.github.com/googleapis/google-auth-library-nodejs/commit/5b48eb86c108c47d317a0eb96b47c0cae86f98cb)) ##### Build System - update to latest gts and TypeScript ([#​927](https://www.github.com/googleapis/google-auth-library-nodejs/issues/927)) ([e11e18c](https://www.github.com/googleapis/google-auth-library-nodejs/commit/e11e18cb33eb60a666980d061c54bb8891cdd242)) ##### Miscellaneous Chores - remove deprecated methods ([#​906](https://www.github.com/googleapis/google-auth-library-nodejs/issues/906)) ([f453fb7](https://www.github.com/googleapis/google-auth-library-nodejs/commit/f453fb7d8355e6dc74800b18d6f43c4e91d4acc9)) ##### [5.10.1](https://www.github.com/googleapis/google-auth-library-nodejs/compare/v5.10.0...v5.10.1) (2020-02-25) ##### Bug Fixes - if GCF environment detected, increase library timeout ([#​899](https://www.github.com/googleapis/google-auth-library-nodejs/issues/899)) ([2577ff2](https://www.github.com/googleapis/google-auth-library-nodejs/commit/2577ff28bf22dfc58bd09e7365471c16f359f109)) </details> --- ### Renovate configuration 📅 **Schedule**: "after 9am and before 3pm" (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻️ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-bigquery). * build: set AUTOSYNTH_MULTIPLE_COMMITS=true for context aware commits (#665) 3f78914 commit 3f78914 Author: Benjamin E. Coe <bencoe@google.com> Date: Tue Mar 31 18:35:04 2020 -0700 build: set AUTOSYNTH_MULTIPLE_COMMITS=true for context aware commits (#665) * docs: document the release schedule we follow (#454) googleapis/synthtool@6a17abc commit 6a17abc7652e2fe563e1288c6e8c23fc260dda97 Author: Benjamin E. Coe <bencoe@google.com> Date: Mon Mar 23 19:22:34 2020 -0700 docs: document the release schedule we follow (#454) * fix: do not run node 8 CI (#456) googleapis/synthtool@1b4cc80 commit 1b4cc80a7aaf164f6241937dd87f3bd1f4149e0c Author: Alexander Fenster <fenster@google.com> Date: Wed Mar 25 08:01:31 2020 -0700 fix: do not run node 8 CI (#456) * fix: update template files for Node.js libraries (#463) googleapis/synthtool@9982024 commit 99820243d348191bc9c634f2b48ddf65096285ed Author: Alexander Fenster <fenster@google.com> Date: Tue Mar 31 11:56:27 2020 -0700 fix: update template files for Node.js libraries (#463)
Fixes #12