[Pulsar SQL] support protobuf/timestamp #13287

tjiuming · 2021-12-14T10:47:57Z

fix: #3554

Motivation

Support protobuf native timestamp for pulsar-sql, so that presto could display formatted timestamp.
Del conflict method testMergeDifferentCleanupSubscription in SinkConfigUtilTest, to make sure build project successfully.

Modifications

add protobuf/timestamp support in PulsarProtobufNativeRowDecoderFactory
return the seconds field of protobuf/timestamp in PulsarProtobufNativeColumnDecoder

Verifying this change

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (don't know)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (don't know)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

no-need-doc
a small feature, seems no need

remove conflict method in SinkConfigUtilsTest.java

tjiuming · 2021-12-14T10:48:19Z

TBR

tjiuming · 2021-12-14T10:50:33Z

there is a question, protobuf/timestamp contains nanos, but presto/timestamp not. so that nanos will be missed in pulsar-sql when display formatted timestamp.

github-actions · 2021-12-14T12:04:20Z

@tjiuming:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

tjiuming · 2021-12-14T16:01:43Z

@tjiuming:Thanks for your contribution. For this PR, do we need to update docs? (The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

I‘m not sure, it is a small feature, just display formatted timestamp, maybe not.

codelipenghui

Could you please add a test to cover the new fix? So that we can avoid the issue will not happening in the future.

codelipenghui · 2021-12-15T00:17:18Z

pulsar-functions/utils/src/test/java/org/apache/pulsar/functions/utils/SinkConfigUtilsTest.java

@@ -390,22 +390,6 @@ public void testMergeRuntimeFlags() {
        );
    }

-    @Test
-    public void testMergeDifferentCleanupSubscription() {


Could you please rebase the master branch? I think the test has already been removed on the master branch.

the method still exists yesterday, but I checkout just now, it removed.

codelipenghui · 2021-12-15T00:17:44Z

...g/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeRowDecoderFactory.java

-import io.prestosql.spi.type.TypeSignature;
-import io.prestosql.spi.type.TypeSignatureParameter;
-import io.prestosql.spi.type.VarbinaryType;
+import io.prestosql.spi.type.*;


Please avoid use star import.

codelipenghui · 2021-12-15T00:29:30Z

...a/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java

+            //return seconds field of protobuf/timestamp
+            if (columnType instanceof TimestampType && value instanceof DynamicMessage) {
+                DynamicMessage message = (DynamicMessage) value;
+                return (long) message.getField(message.getDescriptorForType().findFieldByName("seconds"));


What is the difference between the following pattern:

if (columnType instanceof TimestampType && value instanceof Timestamp) { return ((Timestamp) value).getSeconds(); }

I’ll check it, but I confirmed value instanceof DynamicMessage is true, and protobuf/timestamp not extends DynamicMessage, if I use value instanceof Timestamp, it seems not work

I think we should return time in the unit of milliseconds here. It seems that we need to convert it here. And do we also need to handle Timestamp.nanos here?

…to-timestamp

remove star imports

tjiuming · 2021-12-15T10:08:26Z

@codelipenghui TBR

merge master into presto-timestamp

RobertIndie

I have tested it in C# client -> pulsar SQL case. But the time shown in pulsar SQL is wrong, see lastlogintimestamp:

presto> select * from pulsar."public/default".test;
 name | age |   lastlogintimestamp    | __partition__ |     __event_time__      |    __publish_time__     | __message_id__ | __sequence_id__ | __producer_name__ | __key__ | __properties__
------+-----+-------------------------+---------------+-------------------------+-------------------------+----------------+-----------------+-------------------+---------+----------------
 abc  |  18 | 1970-01-20 07:39:05.115 |            -1 | 1970-01-01 08:00:00.000 | 2021-12-24 11:25:15.093 | (76,0,0)       |               0 | NULL              | NULL    | {}

The correct time is 2021-12-24 ... ....

I have left some comments below. Could we also add some tests to cover the above case?

RobertIndie · 2021-12-24T07:25:29Z

...a/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java

+            //return seconds field of protobuf/timestamp
+            if (columnType instanceof TimestampType && value instanceof DynamicMessage) {
+                DynamicMessage message = (DynamicMessage) value;
+                return (long) message.getField(message.getDescriptorForType().findFieldByName("seconds"));


I think we should return time in the unit of milliseconds here. It seems that we need to convert it here. And do we also need to handle Timestamp.nanos here?

RobertIndie · 2021-12-24T07:28:23Z

...test/java/org/apache/pulsar/sql/presto/decoder/protobufnative/TestProtobufNativeDecoder.java

@@ -83,6 +85,7 @@ public void testPrimitiveType() {
                .setBoolField(true)
                .setBytesField(ByteString.copyFrom("abc".getBytes()))
                .setTestEnum(TestMsg.TestEnum.FAILOVER)
+                .setTimestampField(Timestamp.newBuilder().setSeconds(System.currentTimeMillis()).build())


We should use the time in seconds here.

merge master into presto-timestamp

(cherry picked from commit 1ea4ad8) (cherry picked from commit bdf5802477a8df39a5826548d4b80ad6b3ddbab7)

(cherry picked from commit 1ea4ad8)

support protobuf/timestamp for pulsar-sql

24ce9e3

remove conflict method in SinkConfigUtilsTest.java

github-actions bot assigned tjiuming Dec 14, 2021

github-actions bot added the doc-label-missing label Dec 14, 2021

codelipenghui added doc-not-needed Your PR changes do not impact docs type/bug The PR fixed a bug or issue reported a bug area/sql Pulsar SQL related features and removed doc-label-missing labels Dec 15, 2021

codelipenghui added this to the 2.10.0 milestone Dec 15, 2021

codelipenghui added release/2.8.3 release/2.9.2 labels Dec 15, 2021

codelipenghui requested changes Dec 15, 2021

View reviewed changes

codelipenghui requested review from gaoran10, MarvinCai, 315157973, hangc0276, merlimat and Technoboy- December 15, 2021 00:18

codelipenghui reviewed Dec 15, 2021

View reviewed changes

ariestp316@gmail.com added 2 commits December 15, 2021 17:28

Merge branch 'master' of https://github.com/tjiuming/pulsar into pres…

53ba547

…to-timestamp

add tests.

9eabcf7

remove star imports

tjiuming requested a review from codelipenghui December 22, 2021 02:05

Merge pull request #1 from tjiuming/master

d0c6f72

merge master into presto-timestamp

RobertIndie reviewed Dec 24, 2021

View reviewed changes

ariestp316@gmail.com added 2 commits December 24, 2021 18:03

bug fix

8eb821b

bug fix

3cac235

Merge pull request #2 from tjiuming/master

b6a9e00

merge master into presto-timestamp

RobertIndie approved these changes Dec 27, 2021

View reviewed changes

codelipenghui approved these changes Dec 27, 2021

View reviewed changes

Merge branch 'apache:master' into presto-timestamp

aaac951

codelipenghui merged commit 1ea4ad8 into apache:master Dec 29, 2021

codelipenghui changed the title ~~support protobuf/timestamp for pulsar-sql~~ [Pulsar SQL] support protobuf/timestamp Dec 29, 2021

codelipenghui pushed a commit that referenced this pull request Dec 30, 2021

[Pulsar SQL] support protobuf/timestamp (#13287)

c846c5f

(cherry picked from commit 1ea4ad8) (cherry picked from commit bdf5802477a8df39a5826548d4b80ad6b3ddbab7)

codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 30, 2021

codelipenghui pushed a commit that referenced this pull request Dec 30, 2021

[Pulsar SQL] support protobuf/timestamp (#13287)

078c8db

(cherry picked from commit 1ea4ad8)

codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Dec 30, 2021

wuzhanpeng pushed a commit to wuzhanpeng/pulsar that referenced this pull request Jan 5, 2022

[Pulsar SQL] support protobuf/timestamp (apache#13287)

ea60449

tjiuming deleted the presto-timestamp branch May 10, 2022 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pulsar SQL] support protobuf/timestamp #13287

[Pulsar SQL] support protobuf/timestamp #13287

tjiuming commented Dec 14, 2021 •

edited

Loading

tjiuming commented Dec 14, 2021

tjiuming commented Dec 14, 2021

github-actions bot commented Dec 14, 2021

tjiuming commented Dec 14, 2021

codelipenghui left a comment

codelipenghui Dec 15, 2021

tjiuming Dec 15, 2021

codelipenghui Dec 15, 2021

tjiuming Dec 15, 2021

codelipenghui Dec 15, 2021

tjiuming Dec 15, 2021

RobertIndie Dec 24, 2021 •

edited

Loading

tjiuming commented Dec 15, 2021

RobertIndie left a comment •

edited

Loading

RobertIndie Dec 24, 2021 •

edited

Loading

RobertIndie Dec 24, 2021

[Pulsar SQL] support protobuf/timestamp #13287

[Pulsar SQL] support protobuf/timestamp #13287

Conversation

tjiuming commented Dec 14, 2021 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

tjiuming commented Dec 14, 2021

tjiuming commented Dec 14, 2021

github-actions bot commented Dec 14, 2021

tjiuming commented Dec 14, 2021

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui Dec 15, 2021

Choose a reason for hiding this comment

tjiuming Dec 15, 2021

Choose a reason for hiding this comment

codelipenghui Dec 15, 2021

Choose a reason for hiding this comment

tjiuming Dec 15, 2021

Choose a reason for hiding this comment

codelipenghui Dec 15, 2021

Choose a reason for hiding this comment

tjiuming Dec 15, 2021

Choose a reason for hiding this comment

RobertIndie Dec 24, 2021 • edited Loading

Choose a reason for hiding this comment

tjiuming commented Dec 15, 2021

RobertIndie left a comment • edited Loading

Choose a reason for hiding this comment

RobertIndie Dec 24, 2021 • edited Loading

Choose a reason for hiding this comment

RobertIndie Dec 24, 2021

Choose a reason for hiding this comment

tjiuming commented Dec 14, 2021 •

edited

Loading

RobertIndie Dec 24, 2021 •

edited

Loading

RobertIndie left a comment •

edited

Loading

RobertIndie Dec 24, 2021 •

edited

Loading