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

Jump to bottom

feat: paging changes for bigquery #1661

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

feat: paging changes for bigquery #1661

wants to merge 35 commits into from

Conversation

leahecole
Copy link
Contributor

@leahecole leahecole commented Oct 18, 2024 •
edited
Loading

This PR resolves b/360115234 - see that bug and its parent b/352331075 for initial proposal info. Things this PR does include:

  • the addition of a boolean field in the MethodDescriptorProto interface called maxResultsParameter - this is true if integer wrapper types are allowed and a method has a maxResults parameter
  • logic in the templates that when a method has maxResultsParameter set to true, if it is a paginated method (but not async or stream), convert a number type that the user passes in and reformat it to be in the proper wrapped integer type the underlying API expects
  • logic in the templates that when a method has maxResultsParameter set to true, if it is a paginated method (but not an async or stream one), and if the parameter is maxResults, update the param comments, allow either the wrapper type or the number type
    -logic in the samples templates that when a method has maxResultsParameter set to true, if it is a paginated method (but not an async or stream one), and if the parameter is maxResults, update the TODO comments to have the example comment be a number type instead of the protobuf one
  • adds new baselines for bigquery v2
    • adds protos in test-fixtures
    • updates ci.yaml to call baselines
    • updates baseline-esm.ts and cjs to run

cc @shollyman @alvarowolfx

@leahecole leahecole changed the title Paging changes WIP Paging changes Oct 18, 2024
@sofisl
Copy link
Contributor

sofisl commented Oct 23, 2024 •
edited by leahecole
Loading

Edit - adding the original questions because I (leahecole/coleleah) deleted them from the description:

  1. Do I need to add a baseline?
  2. How do we feel about the param comment modifications?

Ok great! Answering a few questions/comments:

  1. Yes, I do think you should add a baseline to reflect these quirks. From reading the code, I think you'll want to generate bigquery and rename it according to the pattern in baselines and add it there, and also generate it in esm (just add it as an option to the generator), and then the baseline test should pick it up (and you should expect to see your changes). You might have to play around with this though, the important script is in typescript/tools/update-baselines.ts.
  2. I think it's good to modify parameter comments!
  3. I think ending the nunjucks brackets in -%} will keep it from adding those extra lines to baselines which we don't want.
  4. Given this is a brand-new feature, I'd expect we only want to see baseline changes in the bigquery library and not in any other baseline.

@leahecole
Copy link
Contributor Author

Noted! Re point 3 I think that I redid the baselines before I took out a comment I had in a nunjucks file and so that makes sense why it hit so many. I will work on a few outstanding changes, make those baselines, and get this ready for real review

@leahecole leahecole changed the title WIP Paging changes feat: paging changes for bigquery Oct 23, 2024
@leahecole leahecole marked this pull request as ready for review October 23, 2024 21:00
@leahecole leahecole requested review from a team as code owners October 23, 2024 21:00
@leahecole
Copy link
Contributor Author

I added baselines @sofisl! I am honestly not sure why the pubsub baseline regenerates. However, it regenerated with the correct behavior - that maxResultsParameter is set to false because pubsub isn't an allowlisted service, so I am less concerned than if it regenerated with maxResultsParameter set to true.

@snippet-bot Snippet Bot
Copy link

snippet-bot bot commented Oct 23, 2024 •
edited
Loading

Here is the summary of changes.

You are about to add 64 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

sofisl
sofisl requested changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@sofisl sofisl 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 we'll also want to add cjs+ esm tests here and make sure they pass

@leahecole
Copy link
Contributor Author

@googleapis/jsteam-handwritten-libraries FYI, this is part of some modernization work being done for BQ - if your library is also impacted by this, feel free to ping me and I can share more

@leahecole leahecole requested a review from sofisl November 7, 2024 02:12
sofisl
sofisl approved these changes Nov 8, 2024
View reviewed changes
templates/cjs/typescript_gapic/src/$version/$service_client.ts.njk
@@ -893,10 +894,23 @@ export class {{ service.name }}Client {
{{ util.toInterface(method.outputInterface) }}
]>|void {
request = request || {};
{%- if method.maxResultsParameter %}
// the underlying API expects a UInt32Value or Int32Value object
Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually get rid of this comment or perhaps reword it, as technically this is what goes in the client library (and thus what users actually use). Maybe just say:

Converts number to Unit32 or Int32 value for non-compliant APIs.

typescript/src/schema/proto.ts
// services that are allowed to use Int32Value and UInt32Value protobuf wrapper types
// instead of "number" for pageSize/maxResults
// keyed by proto package name, e.g. "google.cloud.foo.v1".
const ENABLE_WRAPPER_TYPES_FOR_PAGE_SIZE = {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, this could be simplified if we just assume this list is opt-in, and remove the trueflag, but up to you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feywind feywind feywind left review comments

@sofisl sofisl sofisl approved these changes

Assignees

@sofisl sofisl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@leahecole @sofisl @feywind