Skip to content

Conversation

@alvarowolfx
Copy link
Contributor

@alvarowolfx alvarowolfx commented May 22, 2024

Fixes #1210
Fixes #1362

@alvarowolfx alvarowolfx requested review from a team as code owners May 22, 2024 21:09
@alvarowolfx alvarowolfx requested a review from obada-ab May 22, 2024 21:09
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/nodejs-bigquery API. labels May 22, 2024
@alvarowolfx
Copy link
Contributor Author

needs fix from googleapis/nodejs-paginator#365

@alvarowolfx alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label May 23, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 23, 2024
@leahecole leahecole self-assigned this May 23, 2024
@shollyman shollyman requested review from leahecole and shollyman and removed request for obada-ab May 23, 2024 18:10
@alvarowolfx alvarowolfx requested a review from obada-ab May 23, 2024 18:10
src/bigquery.ts Outdated
Comment on lines 83 to 94
export type QueryResultsResponse = bigquery.IGetQueryResultsResponse &
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.

@alvarowolfx alvarowolfx removed the request for review from obada-ab May 23, 2024 18:15
Copy link
Contributor

@leahecole leahecole left a comment

Choose a reason for hiding this comment

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

I think this is okay from a TS perspective - left one comment, but would love a BQ review too

src/bigquery.ts Outdated
Comment on lines 83 to 94
export type QueryResultsResponse = bigquery.IGetQueryResultsResponse &
bigquery.IQueryResponse;

export type QueryRowsResponse = PagedResponse<
RowMetadata,
Query,
bigquery.IGetQueryResultsResponse
QueryResultsResponse
>;
export type QueryRowsCallback = PagedCallback<
RowMetadata,
Query,
bigquery.IGetQueryResultsResponse
QueryResultsResponse
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.

src/bigquery.ts Outdated
Comment on lines 2119 to 2122
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.

src/bigquery.ts Outdated
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

@alvarowolfx alvarowolfx added owlbot:run Add this label to trigger the Owlbot post processor. labels May 24, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 24, 2024
@alvarowolfx alvarowolfx added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. labels May 31, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 31, 2024
@gcf-merge-on-green gcf-merge-on-green bot merged commit 9d77cd8 into googleapis:main May 31, 2024
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/nodejs-bigquery API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

query result metadata problem getQueryResults does not provide response object if not specifying maxResults

3 participants