-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Pulsar SQL]support protobuf_native decoder #9841
Conversation
@gaoran10 @congbobo184 Please help review this PR. |
/pulsarbot run-failure-checks |
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.
Great work, LGTM, left some trivial suggestions.
Could you add an integration test for the SchemaType.PROTOBUF_NATIVE
, please refer to the method testForSchema
of the class TestBasicPresto
.
...java/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeRowDecoder.java
Outdated
Show resolved
Hide resolved
...a/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
Show resolved
Hide resolved
...a/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
Outdated
Show resolved
Hide resolved
/pulsarbot run-failure-checks |
...a/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
Outdated
Show resolved
Hide resolved
...a/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
Outdated
Show resolved
Hide resolved
...g/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeRowDecoderFactory.java
Outdated
Show resolved
Hide resolved
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.
All over look good!
...g/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeRowDecoderFactory.java
Show resolved
Hide resolved
...a/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
Outdated
Show resolved
Hide resolved
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.
@hnail The change looks good to me, could you please help add an integration test for using Pulsar SQL to queue data with protobuf_native schema?
…pport_protobuf_decoder
/pulsarbot run-failure-checks |
Fixes apache#3554 ### Motivation Pulsar SQL support for Protobuf.
Fixes #3554
Motivation
Pulsar SQL support for Protobuf.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changes