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

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
* ... 
* ... 

Hive Connector Changes
* ... 
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

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

shangm2 · 2025-05-22T21:14:06Z

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

+
+
+def main():
+    preprocess(sys.argv[1], "temp_presto_thrift.thrift")


This is to remove the "drift.recursive_reference=true" annotation from the original thrift file. This annotation is needed/generated by drift codec and we prefer not to write hacky code in drift repo in order to remove it. @vhsu14 maybe we can add some comment to emphasize this for future reference.

This seems little hacky.. can we add a comment here explaining it.

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.

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 from 2a99cd0 to 7ec0dac Compare May 21, 2025 18:49

Generate ProtocolToThrift files through make and commit

fd74671

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

shangm2 approved these changes May 22, 2025

View reviewed changes

shangm2 reviewed May 22, 2025

View reviewed changes

shangm2 reviewed May 27, 2025

View reviewed changes

NikhilCollooru reviewed May 27, 2025

View reviewed changes

NikhilCollooru approved these changes May 27, 2025

View reviewed changes



		def main():
		preprocess(sys.argv[1], "temp_presto_thrift.thrift")

Generate ProtocolToThrift files through make and commit #25162

Are you sure you want to change the base?

Generate ProtocolToThrift files through make and commit #25162

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!

shangm2 May 22, 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!

NikhilCollooru May 27, 2025

Choose a reason for hiding this comment

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!

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

shangm2 May 22, 2025 •

edited

Loading