-
Notifications
You must be signed in to change notification settings - Fork 16
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.
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
feat: support flexible columns on adapt package #474
feat: support flexible columns on adapt package #474
Conversation
There was a problem hiding this 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.
src/adapt/proto.ts
Outdated
} | ||
const isNameCompatible = isProtoCompatible(name); | ||
if (!isNameCompatible) { | ||
name = generatePlaceholderFieldName(name, fNumber); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Towards internal b/362388647