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
Changes from 17 commits
51d5cf0
d57d800
a55f5f7
bc917b2
ae1690c
40cf355
3381d55
3fc5369
5b24883
fe50f03
6298efd
283071a
b83a84e
eb6f5cd
3df601a
5358056
7244427
80490b5
0d1a53a
623a5b7
6fc30ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1022,7 +1022,6 @@ export class BigQuery extends common.Service { | |
if ((!options || !options.query) && !options.pageToken) { | ||
throw new Error('A SQL query string is required.'); | ||
} | ||
|
||
// tslint:disable-next-line no-any | ||
const query: any = extend( | ||
true, | ||
|
@@ -1463,8 +1462,7 @@ export class BigQuery extends common.Service { | |
return new Job(this, id, options); | ||
} | ||
|
||
query(query: string, options?: QueryOptions): Promise<QueryRowsResponse>; | ||
query(query: Query, options?: QueryOptions): Promise<SimpleQueryRowsResponse>; | ||
query(query: string, options?: QueryOptions): Promise<RowsResponse>; | ||
query( | ||
query: string, | ||
options: QueryOptions, | ||
|
@@ -1571,9 +1569,10 @@ export class BigQuery extends common.Service { | |
optionsOrCallback?: | ||
| QueryOptions | ||
| SimpleQueryRowsCallback | ||
| RowsCallback | ||
| QueryRowsCallback, | ||
cb?: SimpleQueryRowsCallback | QueryRowsCallback | ||
): void | Promise<SimpleQueryRowsResponse> | Promise<QueryRowsResponse> { | ||
cb?: SimpleQueryRowsCallback | RowsCallback | ||
): void | Promise<RowsResponse> { | ||
let options = | ||
typeof optionsOrCallback === 'object' ? optionsOrCallback : {}; | ||
const callback = | ||
|
@@ -1590,7 +1589,27 @@ export class BigQuery extends common.Service { | |
// The Job is important for the `queryAsStream_` method, so a new query | ||
// isn't created each time results are polled for. | ||
options = extend({job}, options); | ||
job!.getQueryResults(options, callback as QueryRowsCallback); | ||
|
||
// 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 commentThe 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.,
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
|
||
if (typeof query === 'string' && query.match(modelQueryRegex)) { | ||
job!.getQueryResults(options, callback as QueryRowsCallback); | ||
return; | ||
} | ||
|
||
job! | ||
.on('error', err => callback!(err)) | ||
.on('complete', () => { | ||
const datasetId = job!.metadata.configuration.query.destinationTable | ||
.datasetId; | ||
const tableId = job!.metadata.configuration.query.destinationTable | ||
.tableId; | ||
const dataset = this.dataset(datasetId); | ||
const table = dataset.table(tableId); | ||
table.getRows(options, callback as 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 changeRowsCallback
, since in the case of model queries we still return the original response:☝️ this is the original slow path right? which we returned as a
QueryRowsCallback
?Mainly just making sure code-paths return the same shaped object.