You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/14 10:48:12 UTC
[GitHub] [pulsar] tjiuming opened a new pull request #13287: support protobuf/timestamp for pulsar-sql
tjiuming opened a new pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
fix: https://github.com/apache/pulsar/issues/3554
### Motivation
1. Support protobuf native timestamp for pulsar-sql, so that presto could display formatted timestamp.
2. Del conflict method `testMergeDifferentCleanupSubscription ` in `SinkConfigUtilTest`, to make sure build project successfully.
### Modifications
1. add protobuf/timestamp support in `PulsarProtobufNativeRowDecoderFactory`
2. 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a change in pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
tjiuming commented on a change in pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#discussion_r769184136
##########
File path: pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
##########
@@ -192,6 +194,12 @@ public long getLong() {
return floatToIntBits((Float) value);
}
+ //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"));
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#discussion_r769144564
##########
File path: pulsar-functions/utils/src/test/java/org/apache/pulsar/functions/utils/SinkConfigUtilsTest.java
##########
@@ -390,22 +390,6 @@ public void testMergeRuntimeFlags() {
);
}
- @Test
- public void testMergeDifferentCleanupSubscription() {
Review comment:
Could you please rebase the master branch? I think the test has already been removed on the master branch.
##########
File path: pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeRowDecoderFactory.java
##########
@@ -25,23 +25,13 @@
import com.google.common.collect.ImmutableList;
import com.google.protobuf.Descriptors;
+import com.google.protobuf.TimestampProto;
import io.airlift.log.Logger;
import io.prestosql.decoder.DecoderColumnHandle;
import io.prestosql.spi.PrestoException;
import io.prestosql.spi.connector.ColumnMetadata;
-import io.prestosql.spi.type.ArrayType;
-import io.prestosql.spi.type.BigintType;
-import io.prestosql.spi.type.BooleanType;
-import io.prestosql.spi.type.DoubleType;
-import io.prestosql.spi.type.IntegerType;
-import io.prestosql.spi.type.RealType;
-import io.prestosql.spi.type.RowType;
-import io.prestosql.spi.type.StandardTypes;
-import io.prestosql.spi.type.Type;
-import io.prestosql.spi.type.TypeManager;
-import io.prestosql.spi.type.TypeSignature;
-import io.prestosql.spi.type.TypeSignatureParameter;
-import io.prestosql.spi.type.VarbinaryType;
+import io.prestosql.spi.type.*;
Review comment:
Please avoid use star import.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a change in pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
tjiuming commented on a change in pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#discussion_r769184906
##########
File path: pulsar-functions/utils/src/test/java/org/apache/pulsar/functions/utils/SinkConfigUtilsTest.java
##########
@@ -390,22 +390,6 @@ public void testMergeRuntimeFlags() {
);
}
- @Test
- public void testMergeDifferentCleanupSubscription() {
Review comment:
the method still exists yesterday, but I checkout just now, it removed.
##########
File path: pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeRowDecoderFactory.java
##########
@@ -25,23 +25,13 @@
import com.google.common.collect.ImmutableList;
import com.google.protobuf.Descriptors;
+import com.google.protobuf.TimestampProto;
import io.airlift.log.Logger;
import io.prestosql.decoder.DecoderColumnHandle;
import io.prestosql.spi.PrestoException;
import io.prestosql.spi.connector.ColumnMetadata;
-import io.prestosql.spi.type.ArrayType;
-import io.prestosql.spi.type.BigintType;
-import io.prestosql.spi.type.BooleanType;
-import io.prestosql.spi.type.DoubleType;
-import io.prestosql.spi.type.IntegerType;
-import io.prestosql.spi.type.RealType;
-import io.prestosql.spi.type.RowType;
-import io.prestosql.spi.type.StandardTypes;
-import io.prestosql.spi.type.Type;
-import io.prestosql.spi.type.TypeManager;
-import io.prestosql.spi.type.TypeSignature;
-import io.prestosql.spi.type.TypeSignatureParameter;
-import io.prestosql.spi.type.VarbinaryType;
+import io.prestosql.spi.type.*;
Review comment:
ok, got it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] dao-jun removed a comment on pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
dao-jun removed a comment on pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#issuecomment-993687889
> @tjiuming:Thanks for your contribution. For this PR, do we need to update docs? (The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#discussion_r774908677
##########
File path: pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
##########
@@ -192,6 +194,12 @@ public long getLong() {
return floatToIntBits((Float) value);
}
+ //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"));
Review comment:
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?
##########
File path: pulsar-sql/presto-pulsar/src/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())
Review comment:
We should use the time in seconds here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a change in pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
tjiuming commented on a change in pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#discussion_r769183647
##########
File path: pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
##########
@@ -192,6 +194,12 @@ public long getLong() {
return floatToIntBits((Float) value);
}
+ //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"));
Review comment:
I’ll check it, but I confirmed `value instanceof DynamicMessage` is true, and protobuf/timestamp not extends DynamicMessage, if I use `value instanceof DynamicMessage`, it seems not work
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] dao-jun commented on pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
dao-jun commented on pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#issuecomment-993687889
> @tjiuming:Thanks for your contribution. For this PR, do we need to update docs? (The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
tjiuming commented on pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#issuecomment-993416273
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#discussion_r769150975
##########
File path: pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
##########
@@ -192,6 +194,12 @@ public long getLong() {
return floatToIntBits((Float) value);
}
+ //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"));
Review comment:
What is the difference between the following pattern:
```
if (columnType instanceof TimestampType && value instanceof Timestamp) {
return ((Timestamp) value).getSeconds();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
tjiuming commented on pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#issuecomment-993689954
> @tjiuming:Thanks for your contribution. For this PR, do we need to update docs? (The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
tjiuming commented on pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#issuecomment-993414607
TBR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
tjiuming commented on pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#issuecomment-994618683
@codelipenghui TBR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#issuecomment-993472939
@tjiuming:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #13287: support protobuf/timestamp for pulsar-sql
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #13287:
URL: https://github.com/apache/pulsar/pull/13287#discussion_r774908677
##########
File path: pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/decoder/protobufnative/PulsarProtobufNativeColumnDecoder.java
##########
@@ -192,6 +194,12 @@ public long getLong() {
return floatToIntBits((Float) value);
}
+ //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"));
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org