Remove thrift protocol implementation in presto_cpp

[majetideepak]

"presto-native-execution/presto_cpp/main/thrift/" supports the thrift protocol. I believe this isn't finished yet and there has been no development since 2022 when it was first added.
My proposal is to remove this. We can then remove the fbthrift dependency.

CC: @pedroerp, @mbasmanova. @tdcmeehan

[pedroerp]

I don't think we have any usage of that at Meta, but @amitkdutta would know it for sure.

[amitkdutta]

CC: @spershin @xiaoxmeng

[spershin]

@majetideepak

Thrift transport protocol, when implemented, would reduce CPU consumption on both workers and the coordinator. It will also reduce the traffic volume as json is quite inefficient.

It is true that the project hasn't moved since 2022, when there were tech layoffs and we lost an engineer who started working on it.

Since then the project has never got enough priority as we are trying to move 100% of Presto traffic to Prestissimo.

I understand that removing one dependency is tempting, however it does not give us any real gain.

I suggest we don't remove the code yet, but instead eventually finish the project.

[tdcmeehan]

As far as I'm aware, the main challenge with Thrift is it doesn't support polymorphic types. This is problematic because our task update protocol is full of them (in the plan node for example). Is there any plan to address that? I think it's fine to keep the dependency presuming it will be eventually useful, but I'm just wondering if it's actually even feasible.

[pedroerp]

Not nearly as clean, but I suppose one could "simulate" polymorphism with an enum and optional fields.

[tdcmeehan]

Given that the "Thrift IDL" is actually be defined as code in Java, would this mean introducing the Thrift unions in our entire plan tree whenever there is polymorphism?

I also took a quick glance at the Thrift generator code and have some concerns about anyone's ability to pick this back up. I believe this is done with the help of a forked Python Thrift parser. And it seems that it works by first converting the Presto protocol, which is defined by the JSON annotations (not Drift/Thrift annotations), then converting that into Thrift code. If I read that right, I don't think that will work--it will need to read the Drift/Thrift annotations just to get correctly interpret the semantic differences between JSON and Thrift.

I'm wondering if we should start an RFC on how precisely to go about binary transport, to help iron out some of these issue. I think the goal is inarguable, but I'm just wondering if it needs a proper design.

[spershin]

It was intended (in the end) that thrift idl files would be the source for both c++ and java.
The way it is implemented now is not correct.
There was an idea to resolve polymorphism by using {"class name", "binary blob"} pairs.

[tdcmeehan]

@spershin given that the way it's implemented now isn't correct, should we just create an issue for how we see it ought to be implemented and delete this code? I believe it would make our build simpler and remove a presently unused dependency.