-
Notifications
You must be signed in to change notification settings - Fork 211
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
feat: integrate jobs.query and stateless query for faster queries #1337
feat: integrate jobs.query and stateless query for faster queries #1337
Conversation
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.
Someone with more BQ experience should give this a look too, but I don't see major Node issues :)
src/bigquery.ts
Outdated
params: Query['params'], | ||
types: Query['types'] | ||
): { | ||
parameterMode: 'positional' | 'named' | undefined; |
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.
Might be good to pull this type out into an interface
, but YMMV.
src/bigquery.ts
Outdated
const queryParameters: bigquery.IQueryParameter[] = []; | ||
if (parameterMode === 'named') { | ||
const namedParams = params as {[param: string]: any}; | ||
for (const namedParameter in namedParams) { |
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.
Might want to use for (const namedParameter of Object.getOwnPropertyNames(namedParams))
or check each namedParameter
against namedParams.hasOwnProperty()
.
src/bigquery.ts
Outdated
let rows: any = []; | ||
if (res.schema && res.rows) { | ||
rows = BigQuery.mergeSchemaWithRows_(res.schema, res.rows, { | ||
wrapIntegers: options.wrapIntegers!, // TODO: fix default value |
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.
Same as above (re: not sure if this was moved from elsewhere or new code) but I wanted to ping in case the TODO isn't intentional.
src/bigquery.ts
Outdated
// The Job is important for the `queryAsStream_` method, so a new query | ||
// isn't created each time results are polled for. | ||
options = extend({job}, queryOpts, options); | ||
if (options.timeoutMs) { |
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.
You can just delete, no need for the if
.
src/bigquery.ts
Outdated
requestId: uuid.v4(), | ||
jobCreationMode: 'JOB_CREATION_OPTIONAL', | ||
}; | ||
if (req.maxResults) { |
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.
Temporary workaround?
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.
gonna remove that
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.
Looks like reasonable coverage for the expansion here, thanks for putting it together.
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.
All still looks fine. I wonder if we have a code style standard for foo_
vs _foo
. Not really something specific to this PR.
Hey @alvarowolfx , thanks for contributing this! I tried using this out but noticed it seems like the types might be incorrect. In particular, when I use |
Also do you know if there is a reason jobTimeoutMs cannot be set? Seems like a nice safety net for runaway queries that is unavailable here (unless timeoutMs implicitly stops the query, but it doesn't seem like it) edit: Also timeoutMs doesn't seem to behave as I would expect. It seems like its a timeout for the initial request only, but then the option is deleted and it will wait for the results again still which may take much longer? I want to ensure my requests don't exceed 1s, but it seems like for a slow query it will still wait 10+s. The "job not complete" log fires fairly fast, but the results still don't resolve until the query actually finishes |
Integrate with
jobs.query
endpoint for faster small queries. Keep usingjobs.getQueryResults
when pagination is needed. Fallback to current method of creating a job +jobs.getQueryResults
when not possible to usejobs.query
.