This could work for simple java types but for complicated fields, we wont have the json codec until creating a instance.

Can you explain this a bit more? I see as an example TableWriteInfoCodec has a JsonCodec wired into its constructor, isn't this what's needed? Couldn't we do something along the lines of what is found in UuidToLeachSalzBinaryEncodingThriftCodec--its Thrift type is binary, and what we do specially is we convert the ByteBuffer contents and feed it into a JsonCodec, as found in e.g. TableWriteInfoCodec. Yes you would need static methods with @FromThrift and @ToThrift, but everything should be in that binary payload and easily convertible to the JsonCodec.

I totally agree with not adding cost to the project. For those connector-specific fields with json serde, we don't recommend fully converting it to thrift for now so we believe they are not tech debt. It might be confusing to have a json field in a thrift payload but they are all connector-specific and the serde framework this pr adds does give the freedom to use the appropriate serde for those fields in different scenarios.

Maybe I'm misunderstanding. From what I see in the code, I only see splits and transaction handles actually participating in the serialization. Other things like table write handles do not, and they are converted into JSON using the old method. I don't think it's OK to leave the framework to omit some connector-specific fields--consider the connector author in C++ needing to JSON serialize some fields, Thrift serialize others. I'm OK to wait for the better solution, which is that all connector specific fields may be serialized by the connector, rather than to have to potentially migrate at a later point in time after production dependencies on this halfway functionality have already been added.

But once again, I think as long as it's connector-dictated and not Presto engine-dictated, that's fine. If we can add the rest of the fields to ConnectorCodecProvider, but provide default implementations which serialize and deserialize as JSON, that's... messy, but fine.

But once again, I think as long as it's connector-dictated and not Presto engine-dictated, that's fine. If we can add the rest of the fields to ConnectorCodecProvider, but provide default implementations which serialize and deserialize as JSON, that's... messy, but fine.

The custom codec is being registered to thrift codec manager and during serde, thrift codec will automatically discover them including the thrift serde for split, transacation handle and all their fields recursively. If you are fine with adding custom serde in ConnectorCodecProvider manually, why not just registering the custom codec for the specific connector and let drift recursively find them.

This could work for simple java types but for complicated fields, we wont have the json codec until creating a instance.

Can you explain this a bit more? I see as an example TableWriteInfoCodec has a JsonCodec wired into its constructor, isn't this what's needed? Couldn't we do something along the lines of what is found in UuidToLeachSalzBinaryEncodingThriftCodec--its Thrift type is binary, and what we do specially is we convert the ByteBuffer contents and feed it into a JsonCodec, as found in e.g. TableWriteInfoCodec. Yes you would need static methods with @FromThrift and @ToThrift, but everything should be in that binary payload and easily convertible to the JsonCodec.

I totally agree with not adding cost to the project. For those connector-specific fields with json serde, we don't recommend fully converting it to thrift for now so we believe they are not tech debt. It might be confusing to have a json field in a thrift payload but they are all connector-specific and the serde framework this pr adds does give the freedom to use the appropriate serde for those fields in different scenarios.

Maybe I'm misunderstanding. From what I see in the code, I only see splits and transaction handles actually participating in the serialization. Other things like table write handles do not, and they are converted into JSON using the old method. I don't think it's OK to leave the framework to omit some connector-specific fields--consider the connector author in C++ needing to JSON serialize some fields, Thrift serialize others. I'm OK to wait for the better solution, which is that all connector specific fields may be serialized by the connector, rather than to have to potentially migrate at a later point in time after production dependencies on this halfway functionality have already been added.

The json codec is not available in a static method. But let me give the static method another try.

If it is fine to you, I can remove all hive related changes and only have thrift annotation for presto-main and presto-main-base.

Hey @tdcmeehan Feel free to give this pr another review. Thanks.

Appreciate the follow through, thanks @shangm2!

@shangm2 has imported this pull request. If you are a Meta employee, you can view this in D76874534.