maosuhan commented on code in PR #21436:
URL: https://github.com/apache/flink/pull/21436#discussion_r1037743338


##########
flink-formats/flink-protobuf/src/test/java/org/apache/flink/formats/protobuf/Pb3ToRowTest.java:
##########
@@ -85,6 +88,8 @@ public void testDeserialization() throws Exception {
 
         assertEquals(1, rowData.getInt(0));
         assertEquals(2L, rowData.getLong(1));
+
+        assertEquals(ts.getNanos(), row.getTimestamp(11, 
3).getNanoOfMillisecond());

Review Comment:
   I'm curious how does Timestamp work because I did not define Timestamp type 
for flink. I guess Timestamp is a message type in protobuf, how come it will 
convert to flink Timestamp automatically?



##########
flink-formats/flink-protobuf/src/main/java/org/apache/flink/formats/protobuf/util/PbFormatUtils.java:
##########
@@ -34,6 +35,15 @@ public static String getFullJavaName(Descriptors.Descriptor 
descriptor, String o
             String parentJavaFullName =
                     getFullJavaName(descriptor.getContainingType(), 
outerProtoName);
             return parentJavaFullName + "." + descriptor.getName();
+        } else if (descriptor.getFullName().contains("google")) {

Review Comment:
   @hdulay The compilation error is caused by a third-party import, right? 
Could you make it more general to adapt to all the import cases?



##########
flink-formats/flink-protobuf/src/test/proto/test_pb3.proto:
##########
@@ -33,6 +34,7 @@ message Pb3Test {
     bytes i = 9;
     map<string, string> map1 = 10;
     map<string, InnerMessageTest> map2 = 11;
+    google.protobuf.Timestamp ts = 12;

Review Comment:
   Also test in ProtobufSQLITCaseTest for end to end correctness.



-- 
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: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to