flink
flink copied to clipboard
apache
[FLINK-32738][formats] PROTOBUF format supports projection push down
What is the purpose of the change
Enable projection pushdown for protobuf format
Brief change log
- Enable projection pushdown for protobuf format
Verifying this change
Please make sure both new and modified tests in this PR follows the conventions defined in our code quality guide: https://flink.apache.org/contributing/code-style-and-quality-common.html#testing
This change is already covered by existing tests, such as all the Proto to Row 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): (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 documented) ](https://github.com/apache/flink/pull/23323)
CI report:
- 008c690b6f4c0b3ce063943fe86ea44e4a005ace Azure: FAILURE
Bot commands
The @flinkbot bot supports the following commands:-
@flinkbot run azurere-run the last Azure build
@libenchao Hi Master, This CI seems failed with Node.js related issue. Would you please help review the Pb projection logic first? I have made it pass all the proto2row test. Let me know if any need clarification
@libenchao Hi Master, This CI seems failed with Node.js related issue. Would you please help review the Pb projection logic first? I have made it pass all the proto2row test. Let me know if any need clarification
@zhougit86 You can use the command '@flinkbot run azure' to rerun the CI as suggested in the flinkbot's comment.
Besides, you don't need to call me or any other Committers/PMC members "Master", we are all volunteers to this project.
@libenchao Hi Master, This CI seems failed with Node.js related issue. Would you please help review the Pb projection logic first? I have made it pass all the proto2row test. Let me know if any need clarification
@zhougit86 You can use the command '@flinkbot run azure' to rerun the CI as suggested in the flinkbot's comment.
Besides, you don't need to call me or any other Committers/PMC members "Master", we are all volunteers to this project.
@libenchao got you Benchao. And I found the CI has passed. Please help review the commit when you have time.
Sorry ,forgot to ask, is it ok for you to be the reviewer?
@zhougit86 Yes, I can review this, however, I have limited spare time, which means that I may respond slowly.
@zhougit86 Yes, I can review this, however, I have limited spare time, which means that I may respond slowly.
Ok, take your time. Let me know if any clarification needed
@maosuhan @ljw-hit Are you interested in reviewing this?
@maosuhan @ljw-hit Would you please help to review? this commit is to enable projection pushdown for the pb format.
@zhougit86 @libenchao I am very happy to review this code. I will take a look during this time.
@zhougit86 @libenchao I am very happy to review this code. I will take a look during this time.
@ljw-hit Hi Bro, May I know your review advice
Hi, I will give my review suggestions in the next two days
------------------ 原始邮件 ------------------ 发件人: "Xiaogang @.>; 发送时间: 2023年10月8日(星期天) 晚上6:45 收件人: @.>; 抄送: @.>; @.>; 主题: Re: [apache/flink] [FLINK-32738][formats] PROTOBUF format supports projection push down (PR #23323)
@zhougit86 @libenchao I am very happy to review this code. I will take a look during this time.
@ljw-hit Hi Bro, May I know your review advice
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
Hi, I will give my review suggestions in the next two days … ------------------ 原始邮件 ------------------ 发件人: "Xiaogang @.>; 发送时间: 2023年10月8日(星期天) 晚上6:45 收件人: @.>; 抄送: @.>; @.>; 主题: Re: [apache/flink] [FLINK-32738][formats] PROTOBUF format supports projection push down (PR #23323) @zhougit86 @libenchao I am very happy to review this code. I will take a look during this time. @ljw-hit Hi Bro, May I know your review advice — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
Hi @ljw-hit , sorry to disturb you. I know you must be very busy, could you please have a review on this?
@zhougit86 Sorry, I'm a little busy recently. You can take a look at this PR. There are some modifications to the logic of codegen and the code will be split. You may need to rebase this for compatibility.
@zhougit86 Sorry, I'm a little busy recently. You can take a look at this PR. There are some modifications to the logic of codegen and the code will be split. You may need to rebase this for compatibility.
@ljw-hit Ok, I will wait for your PR be modified first, and rebase my part on yours. Let me know once yours are merged
@zhougit86 Thank you for your patience. The ability to split protobuf code has been merged with commit a2ec4c. You can rebase and resolve any code conflicts. Let's work together to merge the protobuf projection pushdown capability.
@zhougit86 Thank you for your patience. The ability to split protobuf code has been merged with commit a2ec4c. You can rebase and resolve any code conflicts. Let's work together to merge the protobuf projection pushdown capability.
@ljw-hit Hi Bro, I have merged my commit based on yours. And made your new test passed. Please help me review. Thanks
@zhougit86 Thank you for your patience and contribution. I will complete the review within the next two weeks and provide comments.
@zhougit86 Thank you for your patience and contribution. I will complete the review within the next two weeks and provide comments.
@ljw-hit just a reminder to review, thx 💯
@ljw-hit @libenchao Hi Bro, would you please help review this commit again? Do you think it necessary to create 2 constructor PbDecodingFormat.java ?
@libenchao Hi benchao, seems ljw has been busy, do you mind help review this PR? thanks
@zhougit86 @libenchao I'm very sorry, there have been a lot of things going on at the company recently, and I haven't had time to invest in the community. I'm here now to continue following up on this PR, and let's work together to get it done.
@zhougit86 @libenchao I'm very sorry, there have been a lot of things going on at the company recently, and I haven't had time to invest in the community. I'm here now to continue following up on this PR, and let's work together to get it done.
@ljw-hit thanks bro :) , let me know if any question