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.

Sign up for GitHub

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

fix: return extra args when autoPaginate is on #1365

Merged
merged 5 commits into from
May 31, 2024
Merged

fix: return extra args when autoPaginate is on #1365

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f0db3ec
fix: return extra args when autoPaginate is on
alvarowolfx May 22, 2024
d4f6723
fix: remove tpc test
alvarowolfx May 22, 2024
add2823
fix: use latest version of paginator
alvarowolfx May 23, 2024
bef5262
fix: adjust type changes to be more accurate
alvarowolfx May 24, 2024
efc3636
Merge branch 'main' into fix-pagination-extra-args
alvarowolfx May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
},
"dependencies": {
"@google-cloud/common": "^5.0.0",
"@google-cloud/paginator": "^5.0.0",
"@google-cloud/paginator": "^5.0.2",
"@google-cloud/precise-date": "^4.0.0",
"@google-cloud/promisify": "^4.0.0",
"arrify": "^2.0.1",
Expand Down
17 changes: 12 additions & 5 deletions src/bigquery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,18 @@
maxApiCalls?: number;
};

export type QueryResultsResponse = bigquery.IGetQueryResultsResponse &
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this syntax feels absolutely wild to me, a concrete type made from ANDing two interfaces? Should it be IQueryResultsResponse?
Does this blow up if you have fields with equal names but varying types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to be an or, so it's more accurate. When using the .query method, the return can be either a IQueryResultsResponse or IQueryResponse, this way all common fields are gonna be in that type by default and if you need an specific type, you can create a type coercion like I show in the system-tests:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"this syntax feels absolutely wild to me" that's because it is - you can do some wild and wacky things with typescript 😆

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re Alvaro, I think the OR is probably a better choice

bigquery.IQueryResponse;

export type QueryRowsResponse = PagedResponse<
RowMetadata,
Query,
bigquery.IGetQueryResultsResponse
QueryResultsResponse
>;
export type QueryRowsCallback = PagedCallback<
RowMetadata,
Query,
bigquery.IGetQueryResultsResponse
QueryResultsResponse
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leahecole do you think this is a breaking change ? I feel like is fine because it still compatible with the previous type, I'm just enhancing to cover other response types when getting query results, which can come as IGetQueryResultsResponse (normal path with jobs.getQueryResults) or IQueryResponse (jobs.query "fast query path).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo this is such a good question. I thought about it for a bit and I think you're right - it is backwards compatible - I think it does merit a minor release and not just a patch release though because you are adding functionality - you're just doing so in a way that is backwards compatible.

>;

export type SimpleQueryRowsResponse = [RowMetadata[], bigquery.IJob];
Expand Down Expand Up @@ -165,8 +168,8 @@
bigquery.IJobList
>;

export type JobsQueryResponse = [Job, bigquery.IQueryResponse];
export type JobsQueryCallback = ResourceCallback<Job, bigquery.IQueryResponse>;
export type JobsQueryResponse = [Job, QueryResultsResponse];
export type JobsQueryCallback = ResourceCallback<Job, QueryResultsResponse>;

export interface BigQueryTimeOptions {
hours?: number | string;
Expand Down Expand Up @@ -336,17 +339,17 @@
private _universeDomain: string;
private _enableQueryPreview: boolean;

createQueryStream(options?: Query | string): ResourceStream<RowMetadata> {

Check warning on line 342 in src/bigquery.ts

View workflow job for this annotation

GitHub Actions / lint 

'options' is defined but never used
// placeholder body, overwritten in constructor
return new ResourceStream<RowMetadata>({}, () => {});
}

getDatasetsStream(options?: GetDatasetsOptions): ResourceStream<Dataset> {

Check warning on line 347 in src/bigquery.ts

View workflow job for this annotation

GitHub Actions / lint 

'options' is defined but never used
// placeholder body, overwritten in constructor
return new ResourceStream<Dataset>({}, () => {});
}

getJobsStream(options?: GetJobsOptions): ResourceStream<Job> {

Check warning on line 352 in src/bigquery.ts

View workflow job for this annotation

GitHub Actions / lint 

'options' is defined but never used
// placeholder body, overwritten in constructor
return new ResourceStream<Job>({}, () => {});
}
Expand Down Expand Up @@ -1533,7 +1536,7 @@
const parameterMode = is.array(params) ? 'positional' : 'named';
const queryParameters: bigquery.IQueryParameter[] = [];
if (parameterMode === 'named') {
const namedParams = params as {[param: string]: any};

Check warning on line 1539 in src/bigquery.ts

View workflow job for this annotation

GitHub Actions / lint 

Unexpected any. Specify a different type
for (const namedParameter of Object.getOwnPropertyNames(namedParams)) {
const value = namedParams[namedParameter];
let queryParameter;
Expand Down Expand Up @@ -2113,7 +2116,10 @@
* ```
*/
query(query: string, options?: QueryOptions): Promise<QueryRowsResponse>;
query(query: Query, options?: QueryOptions): Promise<SimpleQueryRowsResponse>;
query(
query: Query,
options?: QueryOptions
): Promise<SimpleQueryRowsResponse> | Promise<QueryRowsResponse>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to also update the docstring to have an example of this? Or is it going to be the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this signature change as was not needed. So I think we don't need updates to the docstring, this is just to fix a behavior that users were already expecting from that method.

query(
query: string,
options: QueryOptions,
Expand Down Expand Up @@ -2176,7 +2182,7 @@

options = extend({job}, queryOpts, options);
if (res && res.jobComplete) {
let rows: any = [];

Check warning on line 2185 in src/bigquery.ts

View workflow job for this annotation

GitHub Actions / lint 

Unexpected any. Specify a different type
if (res.schema && res.rows) {
rows = BigQuery.mergeSchemaWithRows_(res.schema, res.rows, {
wrapIntegers: options.wrapIntegers || false,
Expand All @@ -2185,6 +2191,7 @@
}
this.trace_('[runJobsQuery] job complete');
options._cachedRows = rows;
options._cachedResponse = res;
if (res.pageToken) {
this.trace_('[runJobsQuery] has more pages');
options.pageToken = res.pageToken;
Expand Down
4 changes: 3 additions & 1 deletion src/job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@
/**
* internal properties
*/
_cachedRows?: any[];

Check warning on line 58 in src/job.ts

View workflow job for this annotation

GitHub Actions / lint 

Unexpected any. Specify a different type
_cachedResponse?: bigquery.IQueryResponse;
};

/**
Expand Down Expand Up @@ -133,7 +134,7 @@
location?: string;
projectId?: string;
getQueryResultsStream(
options?: QueryResultsOptions

Check warning on line 137 in src/job.ts

View workflow job for this annotation

GitHub Actions / lint 

'options' is defined but never used
): ResourceStream<RowMetadata> {
// placeholder body, overwritten in constructor
return new ResourceStream<RowMetadata>({}, () => {});
Expand Down Expand Up @@ -571,8 +572,9 @@
pageToken: options.pageToken,
});
delete nextQuery._cachedRows;
delete nextQuery._cachedResponse;
}
callback!(null, options._cachedRows, nextQuery);
callback!(null, options._cachedRows, nextQuery, options._cachedResponse);
return;
}

Expand Down
28 changes: 28 additions & 0 deletions system-test/bigquery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ import {
Routine,
Table,
InsertRowsStreamResponse,
Query,
} from '../src';
import bq from '../src/types';

const bigquery = new BigQuery();
const storage = new Storage();
Expand Down Expand Up @@ -341,6 +343,32 @@ describe('BigQuery', () => {
});
});

it('should query with jobs.query and return all PagedResponse as positional parameters', async () => {
const [rows, q, response] = await bigquery.query(query);
const res: bq.IQueryResponse = response!;
assert.strictEqual(rows!.length, 100);
assert.notEqual(q?.job?.id, undefined);
assert.notEqual(res, undefined);
assert.strictEqual(res.kind, 'bigquery#queryResponse');
assert.notEqual(res.queryId, undefined);
assert.strictEqual(res.totalRows, '100');
});

it('should query without jobs.query and return all PagedResponse as positional parameters', async () => {
// force getQueryResult instead of fast query path
const jobId = generateName('job');
const [rows, q, response] = await bigquery.query({
query,
jobId,
});
const res: bq.IGetQueryResultsResponse = response!;
assert.strictEqual(rows!.length, 100);
assert.strictEqual((q as Query).job?.id, jobId);
assert.notEqual(res, undefined);
assert.strictEqual(res.kind, 'bigquery#getQueryResultsResponse');
assert.strictEqual(res.totalRows, '100');
});

it('should allow querying in series', done => {
bigquery.query(
query,
Expand Down
24 changes: 13 additions & 11 deletions test/bigquery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3152,24 +3152,26 @@ describe('BigQuery', () => {
});
});

it('should call job#getQueryResults with cached rows from jobs.query', done => {
it('should call job#getQueryResults with cached rows and response from jobs.query', done => {
const fakeJob = {
getQueryResults: (options: QueryResultsOptions, callback: Function) => {
callback(null, options._cachedRows, FAKE_RESPONSE);
callback(null, options._cachedRows, null, options._cachedResponse);
},
};

const fakeResponse = {
jobComplete: true,
schema: {
fields: [{name: 'value', type: 'INT64'}],
},
rows: [{f: [{v: 1}]}, {f: [{v: 2}]}, {f: [{v: 3}]}],
};

bq.runJobsQuery = (query: {}, callback: Function) => {
callback(null, fakeJob, {
jobComplete: true,
schema: {
fields: [{name: 'value', type: 'INT64'}],
},
rows: [{f: [{v: 1}]}, {f: [{v: 2}]}, {f: [{v: 3}]}],
});
callback(null, fakeJob, fakeResponse);
};

bq.query(QUERY_STRING, (err: Error, rows: {}, resp: {}) => {
bq.query(QUERY_STRING, (err: Error, rows: {}, query: {}, resp: {}) => {
assert.ifError(err);
assert.deepStrictEqual(rows, [
{
Expand All @@ -3182,7 +3184,7 @@ describe('BigQuery', () => {
value: 3,
},
]);
assert.strictEqual(resp, FAKE_RESPONSE);
assert.strictEqual(resp, fakeResponse);
done();
});
});
Expand Down
Loading