Generate ProtocolToThrift files through make and commit #25162

vhsu14 · 2025-05-20T21:53:20Z

Description

In #25079, ProtocolToThrift files were manually written. This PR generates these files using pythons scripts and chevron templates.

Motivation and Context

Impact

Test Plan

Unit test + Verifier (test_id 225961)

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==
General Changes
* Update ProtocolToThrift files to be generated for cpp thrift serde.

facebook-github-bot · 2025-05-20T22:01:48Z

@vhsu14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

tdcmeehan · 2025-05-21T12:52:08Z

If we're using Thrift, why can't we just use either the Drift IDL generator, or have both Java and C++ auto-generate the Thrift structures from a checked in IDL?

vhsu14 · 2025-05-21T16:23:13Z

If we're using Thrift, why can't we just use either the Drift IDL generator, or have both Java and C++ auto-generate the Thrift structures from a checked in IDL?

We are using Drift to annotate classes in Java and generate an IDL for C++ to auto-gen the Thrift structures. Using a checked-in IDL has some cons in Java:

While we are in the process of switching, we need to maintain two sets of classes and write toThrift/ fromThrift to convert the objects. This requires huge amounts of effort, can be prone to mistakes, and causes GC.
Thrift structures don't support having class methods, so for every class that has methods, we will need to have a utils class for it.
Everyone needs to be aware of the checked-in IDL. Whenever we want to modify classes in the Thrift flow, we need to make corresponding changes to the IDL.

Thanks for the comment. Let me know if you have further questions.

tdcmeehan · 2025-05-21T16:56:47Z

Can we discuss the approach for code generation and overall timeline in this RFC? The protocol infrastructure has its own overhead and technical debt, and it's not really clear to me how this is going to work.

For example, some questions I have:

Does the existing readme in the Thrift directory apply to the current changes?
What is the timeline for maintaining both JSON and Thrift, or do we plan to maintain both?
Is the current Thrift file manually generated? Is this transitional, or going forward would it be a part of the developer workflow to regenerate the Thrift IDL using the Drift tool?

We use RFCs to get alignment on design and also to document decisions made. These are all important decisions that needs to be memorialized into the above RFC.

shangm2 · 2025-05-21T18:04:27Z

Can we discuss the approach for code generation and overall timeline in this RFC? The protocol infrastructure has its own overhead and technical debt, and it's not really clear to me how this is going to work.

For example, some questions I have:

Does the existing readme in the Thrift directory apply to the current changes?

What is the timeline for maintaining both JSON and Thrift, or do we plan to maintain both?

Is the current Thrift file manually generated? Is this transitional, or going forward would it be a part of the developer workflow to regenerate the Thrift IDL using the Drift tool?

We use RFCs to get alignment on design and also to document decisions made. These are all important decisions that needs to be memorialized into the above RFC.

Hey @tdcmeehan . Let me know if the following answers your questions. And yes, we will incorporate these questions and answers into the RFC as well.

Does the existing readme in the Thrift directory apply to the current changes?
Answer: if you are referring to the pipeline of converting thrift to json with the help of python script and yaml file, then yes, we still need this pipeline to generate some code, e.g. those in presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp so that we dont need to write them manually.
What is the timeline for maintaining both JSON and Thrift, or do we plan to maintain both?
Answer: I don't think we have a timeline to deprecate json at least for now. But we can discuss if/how we can do this once the thrift migration is in a stable state.
Is the current Thrift file manually generated? Is this transitional, or going forward would it be a part of the developer workflow to regenerate the Thrift IDL using the Drift tool?
Answer: the idl file is auto generated using drift library. It is achieved when the presto-thrift-spec gets built which happens everything when you build presto. Here are two related prs:

vhsu14 · 2025-05-21T18:12:40Z

presto-native-execution/presto_cpp/main/thrift/presto_protocol-to-thrift-json.yml

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+StructMap:


StructMap maps fields that have different names for protocol and thrift classes

vhsu14 · 2025-05-21T18:14:13Z

presto-native-execution/presto_cpp/main/thrift/presto_protocol-to-thrift-json.yml

+    fields:
+      infoUnion: { field_name: info }
+
+SkipStruct:


We don't generate toThrift/fromThrift classes for the following structs because they are related to OperatorInfo, which is an empty struct in C++

vhsu14 · 2025-05-21T18:16:30Z

presto-native-execution/presto_cpp/main/thrift/presto_protocol-to-thrift-json.yml

+  StageExecutionId:
+  StageId:
+
+WrapperStruct:


These are single field structs in IDL that are directly declared in C++. For example,

In IDL:
struct ConnectorId {
1: string catalogName;
}

In C++:
using ConnectorId = std::string;

vhsu14 · 2025-05-21T18:18:03Z

presto-native-execution/presto_cpp/main/thrift/presto_protocol-to-thrift-json.yml

+  RuntimeStats:
+  SqlFunctionId:
+
+Special:


These are structs that require custom conversions, which are defined under the /special folder

vhsu14 · 2025-05-21T18:23:21Z

presto-native-execution/presto_cpp/main/thrift/Makefile

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+all: ProtocolToThrift.h ProtocolToThrift.cpp


This Makefile defines the flow for generating ProtocolToThrift files as defined in README.md. For now, it depends on presto_thrift.thrift, which is a copy of Drift generated IDL. @shangm2 is working on having it auto-generated as part of presto-thrift-spec and we will reference to the generated IDL once that is done.

facebook-github-bot · 2025-05-21T21:22:56Z

@vhsu14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

shangm2

LGTM

presto-native-execution/presto_cpp/main/thrift/thrift2json.py

shangm2 · 2025-05-27T20:14:14Z

presto-native-execution/presto_cpp/main/thrift/Makefile

+	./presto_protocol-to-thrift-json.py presto_thrift.json ../../presto_protocol/core/presto_protocol_core.json  | jq . > presto_protocol-to-thrift-json.json
+
+presto_thrift.json: presto_thrift.thrift ./thrift2json.py
+	./thrift2json.py presto_thrift.thrift | jq . > presto_thrift.json


The .thrift file is at presto-thrift-spec/target/thrift/presto-thrift-protocol.thrift

NikhilCollooru · 2025-05-27T20:25:00Z

presto-native-execution/presto_cpp/main/thrift/thrift2json.py

+
+
+if __name__ == "__main__":
+    sys.exit(main())


add new line at end of file.

facebook-github-bot · 2025-06-03T18:48:35Z

@vhsu14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-06-05T20:59:16Z

@vhsu14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

…todb#25162)" This reverts commit 369570e.

vhsu14 requested a review from a team as a code owner May 20, 2025 21:53

vhsu14 force-pushed the generate-protocol-to-thrift-files branch from d505a2f to dd4a16c Compare May 21, 2025 00:12

vhsu14 commented May 21, 2025

View reviewed changes

vhsu14 force-pushed the generate-protocol-to-thrift-files branch 2 times, most recently from 7ec0dac to fd74671 Compare May 21, 2025 20:58

shangm2 previously approved these changes May 22, 2025

View reviewed changes

shangm2 reviewed May 22, 2025

View reviewed changes

presto-native-execution/presto_cpp/main/thrift/thrift2json.py Show resolved Hide resolved

shangm2 reviewed May 27, 2025

View reviewed changes

NikhilCollooru reviewed May 27, 2025

View reviewed changes

NikhilCollooru previously approved these changes May 27, 2025

View reviewed changes

vhsu14 dismissed stale reviews from NikhilCollooru and shangm2 via 7e1daf7 May 31, 2025 00:14

vhsu14 force-pushed the generate-protocol-to-thrift-files branch 2 times, most recently from 7e1daf7 to e68e3b1 Compare June 3, 2025 18:47

vhsu14 force-pushed the generate-protocol-to-thrift-files branch from e68e3b1 to 8e72f74 Compare June 4, 2025 02:29

NikhilCollooru previously approved these changes Jun 4, 2025

View reviewed changes

gggrace14 previously approved these changes Jun 4, 2025

View reviewed changes

vhsu14 dismissed stale reviews from gggrace14 and NikhilCollooru via f72d9a5 June 5, 2025 06:15

vhsu14 force-pushed the generate-protocol-to-thrift-files branch from 8e72f74 to f72d9a5 Compare June 5, 2025 06:15

Generate ProtocolToThrift files through make and commit

405a1c6

vhsu14 force-pushed the generate-protocol-to-thrift-files branch from f72d9a5 to 405a1c6 Compare June 5, 2025 15:44

gggrace14 self-requested a review June 5, 2025 20:55

gggrace14 approved these changes Jun 5, 2025

View reviewed changes

vhsu14 merged commit 369570e into prestodb:master Jun 5, 2025
104 of 107 checks passed

majetideepak mentioned this pull request Jun 10, 2025

[native] Type mismatch build error in fromThrift() call with TimeZoneKey on MacOS #25267

Closed

soumiiow added a commit to soumiiow/presto that referenced this pull request Jun 16, 2025

Revert "Generate ProtocolToThrift files through make and commit (pres…

ffa094c

…todb#25162)" This reverts commit 369570e.

soumiiow added a commit to soumiiow/presto that referenced this pull request Jun 17, 2025

Revert "Generate ProtocolToThrift files through make and commit (pres…

8d63294

…todb#25162)" This reverts commit 369570e.

soumiiow added a commit to soumiiow/presto that referenced this pull request Jun 17, 2025

Revert "Generate ProtocolToThrift files through make and commit (pres…

94d8d77

…todb#25162)" This reverts commit 369570e.

unidevel mentioned this pull request Jun 19, 2025

Add release notes for 0.294 unix280/presto#33

Closed

6 tasks

This was referenced Jul 4, 2025

Add release notes for 0.294 unix280/presto#35

Merged

Add release notes for 0.294 unix280/presto#36

Closed

Add release notes for 0.294 unix280/presto#37

Closed

This was referenced Jul 24, 2025

Add release notes for 0.294 unix280/presto#39

Merged

Add release notes for 0.294 unix280/presto#40

Merged

prestodb-ci mentioned this pull request Jul 28, 2025

Add release notes for 0.294 #25633

Open

6 tasks

Generate ProtocolToThrift files through make and commit #25162

Generate ProtocolToThrift files through make and commit #25162

Uh oh!

Conversation

vhsu14 commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

facebook-github-bot commented May 20, 2025

Uh oh!

tdcmeehan commented May 21, 2025

Uh oh!

vhsu14 commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdcmeehan commented May 21, 2025

Uh oh!

shangm2 commented May 21, 2025

Uh oh!

vhsu14 May 21, 2025

Choose a reason for hiding this comment

Uh oh!

vhsu14 May 21, 2025

Choose a reason for hiding this comment

Uh oh!

vhsu14 May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhsu14 May 21, 2025

Choose a reason for hiding this comment

Uh oh!

vhsu14 May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 21, 2025

Uh oh!

shangm2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shangm2 May 27, 2025

Choose a reason for hiding this comment

Uh oh!

NikhilCollooru May 27, 2025

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 3, 2025

Uh oh!

facebook-github-bot commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

vhsu14 commented May 20, 2025 •

edited

Loading

vhsu14 commented May 21, 2025 •

edited

Loading

vhsu14 May 21, 2025 •

edited

Loading

vhsu14 May 21, 2025 •

edited

Loading