Skip to content

Conversation

@ahmedabu98
Copy link
Contributor

Bringing back the changes reverted in #31109

More details in #31061 and #31353

@github-actions
Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@ahmedabu98
Copy link
Contributor Author

R: @robertwb
R: @chamikaramj

CC: @lostluck (if this affects Go at all)
CC: @Polber (if any more changes are needed for YAML)

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@ahmedabu98
Copy link
Contributor Author

Fixes #31353

@lostluck
Copy link
Contributor

Probably does affect Go, but IIRC it just means non idiomatic go field names in generated structs. The existing schema handling code should handle making sure the field is Exported (upper case first character) with a manual field name override of the given snake case name.

@ahmedabu98
Copy link
Contributor Author

Thanks for the insight @lostluck! From a cursory search I see xlang/bigtableio/bigtableio.go is currently the only SchemaTransform use-case in the Go SDK, is that right?

@github-actions github-actions bot added the go label May 28, 2024
@ahmedabu98
Copy link
Contributor Author

R: @lostluck

manual field name override of the given snake case name.

let me know if the last commit does what you meant here

@chamikaramj chamikaramj added this to the 2.57.0 Release milestone May 28, 2024
@lostluck
Copy link
Contributor

Thanks for the insight @lostluck! From a cursory search I see xlang/bigtableio/bigtableio.go is currently the only SchemaTransform use-case in the Go SDK, is that right?

I have no idea. I do not keep track of cross language implementations and details about specific IOs. Your search would be as good as mine.

Copy link
Contributor

@Polber Polber left a comment

Choose a reason for hiding this comment

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

Left some comments

@ahmedabu98
Copy link
Contributor Author

Thanks @Polber for catching those, can you PTAL again?

@Polber
Copy link
Contributor

Polber commented Jun 3, 2024

@ahmedabu98 LGTM, but there is an open PR #31455 that would require a couple changes as well if it gets in before this PR is merged

@robertwb
Copy link
Contributor

robertwb commented Jun 3, 2024

@chamikaramj would this impact using the transform service cross-version?

@chamikaramj
Copy link
Contributor

So transform service just wrap expansion services. So change of behavior should be similar to the change of behavior of the Java expansion service.

Regarding, cross-version usage, I think what could break is use-cases that assume a schema from a old Beam version while trying to use the new Beam version for expansion. For example, hand-coded wrappers in customer's side. We are updating the affected Beam provided wrappers so I think the chances of someone running into issues due to this should be rare.

@ahmedabu98 ahmedabu98 merged commit a7f5898 into apache:master Jun 4, 2024
@Polber Polber mentioned this pull request Nov 13, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants