[FLINK-30093] [formats] Protobuf Timestamp Compile Error #21436

hdulay · 2022-11-30T16:47:50Z

FLINK-30093

What is the purpose of the change

This PR enables support for Google Protobuf Timestamp datatypes and possibly other Google Protobuf datatypes.

Brief change log

Adding <includeMavenTypes>direct</includeMavenTypes> Specifies whether to extract .proto files from Maven dependencies and add them to the protoc import path. Options: "none" (do not extract any proto files), "direct" (extract only from direct dependencies), "transitive" (extract from direct and transitive dependencies)
Ensuring the generated code provides the correct package name for all google proto types.

Verifying this change

Added Google Protobuf Timestamp to existing Proto tests.

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

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yno)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)

hdulay · 2022-11-30T16:49:58Z

Hi @maosuhan . Since you were the originator of this code, I was hoping you could check this enhancement to the protobuf work. Thanks

flinkbot · 2022-11-30T16:52:18Z

CI report:

10bbd43 Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

MartijnVisser · 2022-11-30T18:46:38Z

@hdulay Thanks for opening your first PR. Please check out the code style & quality guide for naming conventions on commits https://flink.apache.org/contributing/code-style-and-quality-preamble.html

rmetzger · 2022-12-01T11:18:48Z

Thx for the PR. Can you also update the commit message, for example to the PR title?

rmetzger · 2022-12-01T11:20:23Z

The CI failure seems to be caused by a known instability: https://issues.apache.org/jira/browse/FLINK-30257

hdulay · 2022-12-01T15:01:33Z

Updated commit message. Tnx

maosuhan · 2022-12-02T02:27:17Z

@hdulay Thanks for raising this issue and your fix PR. I will help to review this PR. cc @MartijnVisser

maosuhan · 2022-12-02T02:30:13Z

...rmats/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")) {


@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?

maosuhan · 2022-12-02T02:56:35Z

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());


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?

The org.apache.flink.table.data.binary.BinaryRowData.getTimestamp() method from the flink-table module does the translation to org.apache.flink.table.data.TimestampData.

@hdulay I think there is still gap between protobuf and flink internal memory layout. I tried to build a concrete Timestamp like Timestamp ts =Timestamp.newBuilder().setSeconds(xxx).setNanos(yyy).build() in unit test. And you can easily find the difference.

@maosuhan Yes as I stepped through the ProtobufSQLITCaseTest to add a timestamp field, I realized this issue goes beyond just source generation and compilation errors. Is there an example of how to add new data type? I'm assuming I'll need to somehow map it to the TimestampType logical type. Thanks

Just to step in: adding a new datatype requires a FLIP and a discussion. Why can't any of the existing datatypes be reused?

@MartijnVisser Hi. I'm suggesting to add support for 3rd party protobuf datatype. Which needs a mapping to either a RowType or a SimpleType in the protobuf codegen components. I'm assuming this is the correct approach. Thanks

@hdulay Yes, I agree with your idea. RowType must be doable to reflect a Timestamp pb type. If you want to make it more easy to use, you can treat google.protobuf.Timestamp a special type and manually convert Timestamp to TimestampData in code.

Thanks. Let me know if it's appropriate to convert this PR to a draft. Thanks for all the guidance.

maosuhan · 2022-12-02T02:57:51Z

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;


Also test in ProtobufSQLITCaseTest for end to end correctness.

MartijnVisser requested a review from libenchao November 30, 2022 18:47

flinkbot added component=Formats component=TableSQL/Ecosystem labels Nov 30, 2022

hdulay changed the title ~~FLINK-30093 Formats Protobuf Timestamp~~ [FLINK-30093] [formats] Protobuf Timestamp Nov 30, 2022

hdulay changed the title ~~[FLINK-30093] [formats] Protobuf Timestamp~~ [FLINK-30093] [formats] Protobuf Timestamp Compile Error Nov 30, 2022

hdulay force-pushed the FLINK-30093 branch from 869e061 to f98b6e9 Compare December 1, 2022 14:53

[FLINK-30093] [formats] Protobuf Timestamp Compile Error

10bbd43

hdulay force-pushed the FLINK-30093 branch from f98b6e9 to 10bbd43 Compare December 1, 2022 14:55

maosuhan reviewed Dec 2, 2022

View reviewed changes

laughingman7743 mentioned this pull request Jan 7, 2023

[FLINK-30093][formats] Fix compile errors for google.protobuf.Timestamp type #21613

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-30093] [formats] Protobuf Timestamp Compile Error #21436

[FLINK-30093] [formats] Protobuf Timestamp Compile Error #21436

hdulay commented Nov 30, 2022

hdulay commented Nov 30, 2022

flinkbot commented Nov 30, 2022 •

edited

MartijnVisser commented Nov 30, 2022

rmetzger commented Dec 1, 2022

rmetzger commented Dec 1, 2022

hdulay commented Dec 1, 2022

maosuhan commented Dec 2, 2022

maosuhan Dec 2, 2022

hdulay Dec 3, 2022

maosuhan Dec 2, 2022

hdulay Dec 3, 2022

maosuhan Dec 4, 2022

hdulay Dec 4, 2022

MartijnVisser Dec 4, 2022

hdulay Dec 4, 2022

maosuhan Dec 5, 2022

hdulay Dec 5, 2022

maosuhan Dec 2, 2022

[FLINK-30093] [formats] Protobuf Timestamp Compile Error #21436

Are you sure you want to change the base?

[FLINK-30093] [formats] Protobuf Timestamp Compile Error #21436

Conversation

hdulay commented Nov 30, 2022

What is the purpose of the change

Brief change log

Verifying this change

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

Documentation

hdulay commented Nov 30, 2022

flinkbot commented Nov 30, 2022 • edited

CI report:

MartijnVisser commented Nov 30, 2022

rmetzger commented Dec 1, 2022

rmetzger commented Dec 1, 2022

hdulay commented Dec 1, 2022

maosuhan commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flinkbot commented Nov 30, 2022 •

edited