Skip to content

[BEAM-7274] Add DynamicMessage Schema support - #10502

Merged
alexvanboxel merged 1 commit into
apache:masterfrom
alexvanboxel:feature/BEAM-7274-dynamic-schema
Feb 25, 2020
Merged

[BEAM-7274] Add DynamicMessage Schema support#10502
alexvanboxel merged 1 commit into
apache:masterfrom
alexvanboxel:feature/BEAM-7274-dynamic-schema

Conversation

@alexvanboxel

@alexvanboxelalexvanboxel commented Jan 4, 2020

Copy link
Copy Markdown
Contributor

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


LangSDKApexDataflowFlinkGearpumpSamzaSpark
GoBuild Status------Build Status------Build Status
JavaBuild StatusBuild StatusBuild StatusBuild Status
Build Status
Build Status
Build StatusBuild StatusBuild Status
Build Status
Build Status
PythonBuild 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)

---JavaPythonGoWebsite
Non-portableBuild StatusBuild Status
Build Status
Build StatusBuild Status
Portable---Build Status------

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

@alexvanboxel
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch from e4a6ec6 to 3a6ca71CompareJanuary 4, 2020 18:39
@alexvanboxel

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

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

Copy link
Copy Markdown
ContributorAuthor

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
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch 2 times, most recently from 04a8580 to 058a636CompareJanuary 8, 2020 08:00
@alexvanboxel
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch from 058a636 to 97414c6CompareJanuary 8, 2020 09:33
@alexvanboxel

Copy link
Copy Markdown
ContributorAuthor

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

@alexvanboxel
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch from 97414c6 to e352ff4CompareJanuary 10, 2020 16:06
@alexvanboxel

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@alexvanboxel
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch 3 times, most recently from 51f9c9d to a83d803CompareFebruary 8, 2020 21:42
@alexvanboxel

Copy link
Copy Markdown
ContributorAuthor

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

@alexvanboxel
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch 2 times, most recently from 602869a to 7e57a39CompareFebruary 12, 2020 16:36
@alexvanboxel
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch from 7e57a39 to 9a13256CompareFebruary 18, 2020 13:49
@alexvanboxel

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@alexvanboxel

alexvanboxel commented Feb 18, 2020

Copy link
Copy Markdown
ContributorAuthor

@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
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch from 9a13256 to 0edacbbCompareFebruary 18, 2020 23:48
@alexvanboxel

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@alexvanboxelalexvanboxel mentioned this pull request Feb 19, 2020
3 tasks
@alexvanboxel

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@alexvanboxel
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch from 0edacbb to 012c45aCompareFebruary 20, 2020 12:28
@reuvenlax

Copy link
Copy Markdown
Contributor

A few comments.

@reuvenlaxreuvenlax left a comment

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.

One small comment, but otherwise LGTM for merge.

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.

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

Copy link
Copy Markdown
ContributorAuthor

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).

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
alexvanboxelforce-pushed the feature/BEAM-7274-dynamic-schema branch from 036b831 to 7a0fe93CompareFebruary 25, 2020 23:25
@alexvanboxel
alexvanboxel merged commit 7990bc7 into apache:masterFeb 25, 2020
@alexvanboxel
alexvanboxel deleted the feature/BEAM-7274-dynamic-schema branch February 25, 2020 23:41
Sign up for freeto 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.

2 participants

@alexvanboxel@reuvenlax