[FLINK-30093] [formats] Protobuf Timestamp Compile Error by hdulay · Pull Request #21436 · apache/flink

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

            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

        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

    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

hdulay closed this Jul 14, 2024

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

Uh oh!

hdulay commented Nov 30, 2022

Uh oh!

flinkbot commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

MartijnVisser commented Nov 30, 2022

Uh oh!

rmetzger commented Dec 1, 2022

Uh oh!

rmetzger commented Dec 1, 2022

Uh oh!

hdulay commented Dec 1, 2022

Uh oh!

maosuhan commented Dec 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flinkbot commented Nov 30, 2022 •

edited

Loading