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

[BEAM-7274] Add DynamicMessage Schema support #10502

Merged
merged 1 commit into from
Feb 25, 2020
Merged

[BEAM-7274] Add DynamicMessage Schema support #10502

merged 1 commit into from
Feb 25, 2020

Conversation

alexvanboxel
Copy link
Contributor

@alexvanboxel alexvanboxel commented Jan 4, 2020 •
edited
Loading

Add DynamicMessage schema support. This is different from
generated classes as it uses the proto descriptors. It uses
the ProtoDomain as an index for searching embedded messages.

R: @reuvenlax

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status		 Build Status Build Status Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status		 --- Build Status
Build Status		 Build Status
Build Status		 --- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status		 Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch from e4a6ec6 to 3a6ca71 Compare January 4, 2020 18:39
@alexvanboxel
Copy link
Contributor Author

Run Java PreCommit

@alexvanboxel alexvanboxel marked this pull request as ready for review January 5, 2020 11:41
@alexvanboxel
Copy link
Contributor Author

R: @reuvenlax this is an implementation for DynamicMessages. It takes into account the new Logical Types and also use RowWithStorage instead of going with the getters.

@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch 2 times, most recently from 04a8580 to 058a636 Compare January 8, 2020 08:00
@alexvanboxel alexvanboxel mentioned this pull request Jan 8, 2020
Closed
@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch from 058a636 to 97414c6 Compare January 8, 2020 09:33
@alexvanboxel
Copy link
Contributor Author

@reuvenlax this is ready for review. It has feature parity with the static compiled proto, including all logical types.

@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch from 97414c6 to e352ff4 Compare January 10, 2020 16:06
@alexvanboxel
Copy link
Contributor Author

Run Java PreCommit

@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch 3 times, most recently from 51f9c9d to a83d803 Compare February 8, 2020 21:42
@alexvanboxel
Copy link
Contributor Author

This branch has been updated to take the new Logical Types into account.

@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch 2 times, most recently from 602869a to 7e57a39 Compare February 12, 2020 16:36
@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch from 7e57a39 to 9a13256 Compare February 18, 2020 13:49
@alexvanboxel
Copy link
Contributor Author

Run Java PreCommit

@alexvanboxel
Copy link
Contributor Author

alexvanboxel commented Feb 18, 2020 •
edited
Loading

@reuvenlax @iemejia @TheNeuralBit I've tested this PR extensively on Dataflow. This is way more stable than the original DynamicMessage schema implementation.

Can I get a LGTM, that I can focus on the options. Thanks.

(master is failing, it was green till rebase)

@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch from 9a13256 to 0edacbb Compare February 18, 2020 23:48
@alexvanboxel
Copy link
Contributor Author

Run Java PreCommit

@alexvanboxel alexvanboxel mentioned this pull request Feb 19, 2020
Closed
3 tasks
@alexvanboxel
Copy link
Contributor Author

Run Java PreCommit

@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch from 0edacbb to 012c45a Compare February 20, 2020 12:28
@reuvenlax
Copy link
Contributor

A few comments.

reuvenlax
reuvenlax approved these changes Feb 25, 2020
View reviewed changes
Copy link
Contributor

@reuvenlax reuvenlax left a comment

Choose a reason for hiding this comment

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

One small comment, but otherwise LGTM for merge.

...rotobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoDynamicMessageSchema.java Outdated
TypeDescriptor.of(context.getBaseClass()),
toRowFunction,
fromRowFunction);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this function being used anywhere. Remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used it in a production pipeline, but as it's not available in the other providers I removed it, (squashed and rebased).

@alexvanboxel
[BEAM-7274] Add DynamicMessage Schema support
7a0fe93 
Add DynamicMessage schema support. This is different from
generated classes as it uses the proto descriptors. It uses
the ProtoDomain as an index for searching embedded messages.
@alexvanboxel alexvanboxel force-pushed the feature/BEAM-7274-dynamic-schema branch from 036b831 to 7a0fe93 Compare February 25, 2020 23:25
@alexvanboxel alexvanboxel merged commit 7990bc7 into apache:master Feb 25, 2020
@alexvanboxel alexvanboxel deleted the feature/BEAM-7274-dynamic-schema branch February 25, 2020 23:41
@veblush veblush mentioned this pull request Oct 27, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reuvenlax reuvenlax reuvenlax approved these changes

Assignees
No one assigned
Labels
extensions java protobuf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alexvanboxel @reuvenlax