-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BEAM-5504] Add Avro support to Pubsub table provider #12780
Conversation
4db126e
to
0374a98
Compare
3a7bbf7
to
9213387
Compare
@TheNeuralBit Could I ask you for review? I wonder whether payloadFormat shouldn't be specified in TBLPROPERTIES instead of creating a separate class for every payload format? I'm planning to write PubsubProtoTableProvider next so there will be another and then probably yet another class. |
7c0fe8d
to
7b14d30
Compare
fa617a4
to
c40eeb2
Compare
Yep I can review this!
Yeah I think that's the way to go. Let's add a payloadFormat option in TBLPROPERTIES that defaults to JSON if not specified. |
87141c5
to
ae2fbbc
Compare
3af8a1c
to
d2933bf
Compare
It's already in TBLPROPERTIES |
4283eca
to
b3a71ef
Compare
Run SQL PostCommit |
Done. |
aecc317
to
66756ff
Compare
Run SQL PostCommit |
Run Java PreCommit |
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.
Thanks @piotr-szuberski! I have a few suggestions
.../sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubAvroIT.java
Outdated
Show resolved
Hide resolved
.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/pubsub/PayloadFormat.java
Outdated
Show resolved
Hide resolved
...oogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/pubsub/PubsubMessageToRow.java
Outdated
Show resolved
Hide resolved
Run SQL PostCommit |
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.
Took a closer look at the implementation this time and I have a few more suggestions there. This looks good otherwise.
...test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
Outdated
Show resolved
Hide resolved
.../sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubAvroIT.java
Outdated
Show resolved
Hide resolved
...oogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/pubsub/PubsubMessageToRow.java
Outdated
Show resolved
Hide resolved
...oogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/pubsub/RowToPubsubMessage.java
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/dsls/sql/extensions/create-external-table.md
Show resolved
Hide resolved
77da35a
to
e3efcb4
Compare
e3efcb4
to
582ef45
Compare
Run SQL PostCommit |
Run Java PreCommit |
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.
LGTM, thanks!
Add pubsub avro table provider.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.