Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 9 commits into from
Closed

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

wants to merge 9 commits into from

Conversation

laughingman7743
Copy link
Contributor

@laughingman7743 laughingman7743 commented Jan 7, 2023 •
edited
Loading

What is the purpose of the change

The current implementation does not take into consideration the case where another package is imported and used for a message, so it seems that the way getOuterProtoPrefix is determined needs to be improved.
This improvement allows the google.protobuf.Timestamp type to be handled.

Brief change log

  • Fixed getOuterProtoPrefix method
  • Changed signature of getFullJavaName method
  • Added test case for google.protobuf.Timestamp type (with multiple_files and outer_classname options)
  • Fixed proto file format according to style guide

Verifying this change

The added test case of google.protobuf.Timestamp type should succeed.

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): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • 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)

@laughingman7743 laughingman7743 marked this pull request as ready for review January 7, 2023 05:38
@flinkbot
Copy link
Collaborator

flinkbot commented Jan 7, 2023 •
edited
Loading

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@laughingman7743 laughingman7743 mentioned this pull request Jan 7, 2023
Closed
@laughingman7743
Copy link
Contributor Author

Related: #21436

@laughingman7743 laughingman7743 changed the title [FLINK-30093][Formats] Fix compile errors for google.protobuf.Timestamp type [FLINK-30093][formats] Fix compile errors for google.protobuf.Timestamp type Jan 7, 2023
maosuhan
maosuhan reviewed Jan 7, 2023
View reviewed changes
Copy link
Contributor

@maosuhan maosuhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laughingman7743 Thanks for your contribution, the code is in a very good shape. I just left a few comments. Please take a look at it when you have time.

...rotobuf/src/main/java/org/apache/flink/formats/protobuf/deserialize/ProtoToRowConverter.java Outdated Show resolved Hide resolved
...rmats/flink-protobuf/src/main/java/org/apache/flink/formats/protobuf/util/PbFormatUtils.java Outdated Show resolved Hide resolved
...rmats/flink-protobuf/src/main/java/org/apache/flink/formats/protobuf/util/PbFormatUtils.java Outdated Show resolved Hide resolved
flink-formats/flink-protobuf/src/test/proto/test_simple_noouter_nomulti.proto Outdated Show resolved Hide resolved
flink-formats/flink-protobuf/src/test/proto/test_simple.proto Show resolved Hide resolved
flink-formats/flink-protobuf/src/test/proto/test_simple_outer_nomulti.proto Show resolved Hide resolved
flink-formats/flink-protobuf/src/test/proto/test_timestamp.proto Outdated Show resolved Hide resolved
@laughingman7743
Copy link
Contributor Author

@maosuhan I have addressed the comments, PTAL when you have time 🙏

@laughingman7743
Copy link
Contributor Author

@flinkbot run azure

@maosuhan
Copy link
Contributor

maosuhan commented Jan 8, 2023

@laughingman7743 Thanks for your quick quick fix. We are almost there. Would you add another unit test like TimestampMultiRowToProto.java to make sure flink row can be correctly converted to protobuf object?

@laughingman7743
Copy link
Contributor Author

laughingman7743 commented Jan 8, 2023 •
edited
Loading

@maosuhan I have added test cases to convert from flink internal primitive data to proto timestamp data. I will add any other test cases that are needed.
6d28b33

@maosuhan
Copy link
Contributor

maosuhan commented Jan 9, 2023

@laughingman7743 The code LGTM. Would you also add documentation in protobuf page describing how to use timestamp type or other internal provided type? Some users may not know the corresponding flink type of Timestamp is a row structure.

@laughingman7743
Copy link
Contributor Author

@maosuhan I have added a description of data mapping of type google.protobuf.Timestamp to the documentation.
23b5fb0

https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=44568&view=logs&j=af184cdd-c6d8-5084-0b69-7e9c67b35f7a&t=160c9ae5-96fd-516e-1c91-deb81f59292a
The e2e test seems to be failing, but I can't find the cause. Could the changes made in this pull request be affecting it? Please let me know if I am missing something.

@laughingman7743
Copy link
Contributor Author

Currently all tests pass. (Maybe there is a flaky test in the e2e test).
https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=44585&view=results

@maosuhan
Copy link
Contributor

@laughingman7743 It looks all right now. @libenchao Do you have time to take another look at this?

@libenchao
Copy link
Member

@maosuhan I will, but I've been a little busy lately. To manage your expectation, it won't be quick.

@libenchao
Copy link
Member

@laughingman7743 Thanks for your contribution and @maosuhan thanks for your review. The PR looks good, I'm merging.

@libenchao libenchao closed this in 7ea4476 Jan 19, 2023
chucheng92 pushed a commit to chucheng92/flink that referenced this pull request Feb 3, 2023
@laughingman7743 @chucheng92
[FLINK-30093][protobuf] Fix compile errors for google.protobuf.Timest…
5937148 
…amp type

Close apache#21613
akkinenivijay pushed a commit to krisnaru/flink that referenced this pull request Feb 11, 2023
@laughingman7743 @akkinenivijay
[FLINK-30093][protobuf] Fix compile errors for google.protobuf.Timest…
7ae0175 
…amp type

Close apache#21613
baugarten pushed a commit to baugarten/flink that referenced this pull request Jun 9, 2023
@laughingman7743 @baugarten-stripe
[FLINK-30093][protobuf] Fix compile errors for google.protobuf.Timest…
9e30572 
…amp type

Close apache#21613
@baugarten baugarten mentioned this pull request Jun 9, 2023
Open
morozov pushed a commit to morozov/flink that referenced this pull request Apr 26, 2024
@kurnoolsaketh @maosuhan
@Jiabao-Sun @laughingman7743
DE-3884: integrate protobuf into decodable-flink (apache#51)
4a8b6a5 
* [FLINK-18202][protobuf] Introduce protobuf format

This closes apache#14376

* [FLINK-29062][build] Fix protobuf plugin proxy issue on flink-protobuf module.

* [FLINK-30093][protobuf] Fix compile errors for google.protobuf.Timestamp type

Close apache#21613

* add schema as a format option

* bump to 1.15-SNAPSHOT

---------

Co-authored-by: maosuhan <[email protected]>
Co-authored-by: jiabao.sun <[email protected]>
Co-authored-by: laughingman7743 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maosuhan maosuhan Awaiting requested review from maosuhan

Assignees
No one assigned
Labels
component=Formats component=TableSQL/Ecosystem
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@laughingman7743 @flinkbot @maosuhan @libenchao