[native] [presto] Expand connector specific fields #25474

vhsu14 · 2025-07-01T20:40:20Z

Description

Native changes to expand connector specific fields for thrift migration
Depends on Add support to provide thrift codec for connector specific fields #25242

Motivation and Context

prestodb/rfcs#38

Test Plan

Build package and test in verifier: 230498

Release Notes

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

== RELEASE NOTES ==
Prestissimo (Native Execution) Changes
* Update thrift IDL to expand connector specific fields

facebook-github-bot · 2025-07-01T20:40:28Z

This pull request was exported from Phabricator. Differential Revision: D77612938

vhsu14 · 2025-07-01T23:24:25Z

presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp

+void toThrift(
+    const std::shared_ptr<
+        facebook::presto::protocol::ConnectorTransactionHandle>& proto,
+    ConnectorTransactionHandleWrapper& thrift) {}


toThrift for connector specific structs are not implemented yet because they are not used (we only need fromThrift for these structs as they are part of TaskUpdateRequest, which coordinator sends to worker). We can take this as a follow-up task to implement.

vhsu14 · 2025-07-01T23:48:15Z

presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp

+  } else if (thrift.jsonValue().has_value()) {
+    json j = json::parse(thrift.jsonValue().value());
+    from_json(j, proto);
+  }


This matches with Java coordinator changes. For connector structs, there will be 3 fields (connectorId, thriftValue, and jsonValue). When connectorId and thriftValue are supplied, we use thrift codec. When jsonValue is supplied, we use json codec.

arhimondr

Looks good to me % comments

arhimondr · 2025-07-02T20:45:46Z

presto-native-execution/presto_cpp/presto_protocol/core/special/ConnectorSplit.hpp.inc

+namespace facebook::presto::protocol {
+struct ConnectorSplit : public JsonEncodedSubclass {
+  static std::optional<std::string> serialize(ConnectorSplit& p) {
+    return std::nullopt;


This should throw by default. For example if a certain connector does not provide a custom serialize / deserialize methods but a coordinator sends a custom serialized field - this should throw an exception and cause a failure. Maybe throw something like VELOX_UNSUPPORTED instead of returning an Optional

arhimondr · 2025-07-02T20:46:00Z

presto-native-execution/presto_cpp/presto_protocol/core/special/ConnectorSplit.hpp.inc

+    return std::nullopt;
+  }
+  static std::shared_ptr<ConnectorSplit> deserialize(const std::string& data, std::shared_ptr<ConnectorSplit> p) {
+    return nullptr;


same here and for ConnectorTransactionHandle as well

arhimondr · 2025-07-02T20:49:31Z

presto-native-execution/presto_cpp/main/thrift/presto_thrift.thrift

-  1: string metadataUpdates;
+struct ConnectorSplitWrapper {
+  1: optional string connectorId;
+  2: optional binary thriftValue;


Should it be called thriftValue or customSerializedValue? In other words do we make an explicit assumption that a connector will serialize / deserialize with Thrift?

synced with Shang to make relevant changes on Java as well

Summary: Rollback Plan: Differential Revision: D77612938

facebook-github-bot · 2025-07-03T06:38:54Z

This pull request was exported from Phabricator. Differential Revision: D77612938

Summary: Pull Request resolved: prestodb#25474 Reviewed By: tanjialiang Differential Revision: D77612938

facebook-github-bot · 2025-07-14T19:47:42Z

This pull request was exported from Phabricator. Differential Revision: D77612938

Summary: Pull Request resolved: prestodb#25474 Reviewed By: tanjialiang Differential Revision: D77612938

facebook-github-bot · 2025-07-15T05:07:54Z

This pull request was exported from Phabricator. Differential Revision: D77612938

Summary: Pull Request resolved: prestodb#25474 Reviewed By: tanjialiang Differential Revision: D77612938

prestodb-ci · 2025-08-04T20:08:42Z

Saved that user @vhsu14 is from Meta

## Description - Native changes to expand connector specific fields for thrift migration - Depends on prestodb#25242 ## Motivation and Context prestodb/rfcs#38 ## Test Plan Build package and test in verifier: 230498 ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == Prestissimo (Native Execution) Changes * Update thrift IDL to expand connector specific fields ```

vhsu14 requested a review from a team as a code owner July 1, 2025 20:40

facebook-github-bot added the fb-exported label Jul 1, 2025

vhsu14 commented Jul 1, 2025

View reviewed changes

arhimondr previously approved these changes Jul 2, 2025

View reviewed changes

vhsu14 added a commit to vhsu14/presto that referenced this pull request Jul 3, 2025

[native] [presto] Expand connector specific fields (prestodb#25474)

112a7a3

Summary: Rollback Plan: Differential Revision: D77612938

vhsu14 dismissed arhimondr’s stale review via 112a7a3 July 3, 2025 06:38

vhsu14 force-pushed the export-D77612938 branch from 309a46f to 112a7a3 Compare July 3, 2025 06:38

arhimondr previously approved these changes Jul 3, 2025

View reviewed changes

tanjialiang previously approved these changes Jul 14, 2025

View reviewed changes

vhsu14 pushed a commit to vhsu14/presto that referenced this pull request Jul 14, 2025

[native] [presto] Expand connector specific fields (prestodb#25474)

6144d78

Summary: Pull Request resolved: prestodb#25474 Reviewed By: tanjialiang Differential Revision: D77612938

vhsu14 dismissed stale reviews from tanjialiang and arhimondr via 6144d78 July 14, 2025 19:43

vhsu14 force-pushed the export-D77612938 branch from 112a7a3 to 6144d78 Compare July 14, 2025 19:43

vhsu14 pushed a commit to vhsu14/presto that referenced this pull request Jul 14, 2025

[native] [presto] Expand connector specific fields (prestodb#25474)

c18d4d0

Summary: Pull Request resolved: prestodb#25474 Reviewed By: tanjialiang Differential Revision: D77612938

vhsu14 force-pushed the export-D77612938 branch from 6144d78 to c18d4d0 Compare July 14, 2025 19:47

vhsu14 pushed a commit to vhsu14/presto that referenced this pull request Jul 14, 2025

[native] [presto] Expand connector specific fields (prestodb#25474)

4e72a6a

Summary: Pull Request resolved: prestodb#25474 Reviewed By: tanjialiang Differential Revision: D77612938

vhsu14 force-pushed the export-D77612938 branch from c18d4d0 to 4e72a6a Compare July 14, 2025 20:30

vhsu14 pushed a commit to vhsu14/presto that referenced this pull request Jul 14, 2025

[native] [presto] Expand connector specific fields (prestodb#25474)

cf7001e

Summary: Pull Request resolved: prestodb#25474 Reviewed By: tanjialiang Differential Revision: D77612938

vhsu14 added a commit to vhsu14/presto that referenced this pull request Jul 15, 2025

[native] [presto] Expand connector specific fields (prestodb#25474)

09b8215

Summary: Pull Request resolved: prestodb#25474 Reviewed By: tanjialiang Differential Revision: D77612938

vhsu14 force-pushed the export-D77612938 branch from 4e72a6a to 09b8215 Compare July 15, 2025 05:07

[native] [presto] Expand connector specific fields (prestodb#25474)

d936e01

Summary: Pull Request resolved: prestodb#25474 Reviewed By: tanjialiang Differential Revision: D77612938

vhsu14 force-pushed the export-D77612938 branch from 09b8215 to d936e01 Compare July 15, 2025 05:12

gggrace14 self-requested a review July 15, 2025 16:51

gggrace14 approved these changes Jul 15, 2025

View reviewed changes

shangm2 merged commit 17a99a7 into prestodb:master Jul 15, 2025
116 of 117 checks passed

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

Merged

6 tasks

tdcmeehan added the from:Meta PR from Meta label Aug 4, 2025

[native] [presto] Expand connector specific fields #25474

[native] [presto] Expand connector specific fields #25474

Uh oh!

Conversation

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

Uh oh!

Description

Motivation and Context

Test Plan

Release Notes

Uh oh!

facebook-github-bot commented Jul 1, 2025

Uh oh!

vhsu14 Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

vhsu14 Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

arhimondr left a comment

Choose a reason for hiding this comment

Uh oh!

arhimondr Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

arhimondr Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

arhimondr Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

vhsu14 Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 3, 2025

Uh oh!

facebook-github-bot commented Jul 14, 2025

Uh oh!

facebook-github-bot commented Jul 15, 2025

Uh oh!

Uh oh!

prestodb-ci commented Aug 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vhsu14 commented Jul 1, 2025 •

edited

Loading