Skip to content
This repository was archived by the owner on Mar 5, 2026. It is now read-only.

feat: support flexible columns on adapt package#474

Merged
alvarowolfx merged 4 commits intogoogleapis:mainfrom
alvarowolfx:feat-adapt-flexible-column
Sep 10, 2024
Merged

feat: support flexible columns on adapt package#474
alvarowolfx merged 4 commits intogoogleapis:mainfrom
alvarowolfx:feat-adapt-flexible-column

Conversation

@alvarowolfx
Copy link
Copy Markdown
Contributor

Towards internal b/362388647

@alvarowolfx alvarowolfx requested review from a team and shollyman September 6, 2024 15:41
@product-auto-label product-auto-label Bot added the size: m Pull request size is medium. label Sep 6, 2024
@alvarowolfx alvarowolfx added owlbot:run Add this label to trigger the Owlbot post processor. and removed size: m Pull request size is medium. labels Sep 6, 2024
@product-auto-label product-auto-label Bot added the api: bigquerystorage Issues related to the googleapis/nodejs-bigquery-storage API. label Sep 6, 2024
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 6, 2024
@product-auto-label product-auto-label Bot added the size: m Pull request size is medium. label Sep 6, 2024
@alvarowolfx alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 6, 2024
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 6, 2024
@product-auto-label product-auto-label Bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 6, 2024
Copy link
Copy Markdown
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

One question about corner case behavior, but otherwise LGTM.

Comment thread src/adapt/proto.ts Outdated
}
const isNameCompatible = isProtoCompatible(name);
if (!isNameCompatible) {
name = generatePlaceholderFieldName(name, fNumber);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we think name collisions after sanitizing is a significant-enough concern at this point? e.g. foo_👍 and foo_👎 as an example, since both would end up with foo__ if I'm reading correctly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, I was concerned about the field collision, but not sure the best approach here. The approach that the Java side took to always convert a non proto compatible field to a format like col_{b64(fieldName), but that generates a field that is not easy to guess or use (like col_a5Bmvz).

And I meant to transform foo_👍 into just foo_, but I'll check again if that is true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, it comes down to the consumer model. I believe the expectation in java is they're the ones consuming the schema so they know to check the annotation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you build up a reserved field list while you're walking the list of fields you can add basic conflict handling, but it does highlight that field name resolution necessitates awareness of the annotation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I sent some changes to actually work more closely on how the Java client lib going, which is to allow users to write JSON data using flexible column field names and under the hood use the placeholders to convert data.

@alvarowolfx alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 6, 2024
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 6, 2024
@alvarowolfx alvarowolfx merged commit c521cc5 into googleapis:main Sep 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: bigquerystorage Issues related to the googleapis/nodejs-bigquery-storage API. size: l Pull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants