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 19 commits into
base: main
Choose a base branch
from
Open

feat: paging changes for bigquery #1661

wants to merge 19 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
  • adds new baselines for bigquery v2

@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

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

sofisl
sofisl reviewed Oct 29, 2024
View reviewed changes
typescript/src/schema/proto.ts
// this is used to determine factors about pagination fields and to allow users to pass a "number" instead of
// having to convert to a protobuf wrapper type to determine page size
const wrappersAllowed =
ENABLE_WRAPPER_TYPES_FOR_PAGE_SIZE[parameters.service.packageName];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you don't want pubsub to change, you'd make this:
ENABLE_WRAPPER_TYPES_FOR_PAGE_SIZE[parameters.service.packageName] ? true : undefined
This would preserve current behavior but it would also make it less clear in the future. However, do you envision this list being actually a full dict of opt-in or opt-outs? I could also see ENABLE_WRAPPER_TYPES_FOR_PAGE_SIZE just being an array where we assume opt-in. I'm fine either way, perhaps it's best to talk to pubsub since it seems they're the only other ones potentially affected by this change (i.e., do they want to explicitly opt-out or just continue functionality?)

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

@sofisl sofisl sofisl requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@sofisl sofisl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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