Add ConnectorDeleteTableHandle to ConnectorProtocol #24721

ghelmling · 2025-03-12T23:50:23Z

Description

In PR #24528 we added a new ConnectorDeleteTableHandle interface for use in ConnectorMetadata::beginDelete and finishDelete. This changes adds a type parameter for ConnectorDeleteTableHandle implementations to the ConnectorProtocolTemplate declaration, with corresponding hooks in ConnectorProtocol for presto_protocol (de)serialization.

Motivation and Context

Adding the implementing type as a parameter to ConnectorProtocolTemplate is required to correctly serialize ConnectorDeleteTableHandle implementations to native workers.

Impact

Any current connectors defining ConnectorProtocolTemplate specializations will need to add a type parameter for the ConnectorDeleteTableHandle implementing type. If the connector does not currently handle ConnectorDeleteTableHandle, it can use the NotImplemented type.

Test Plan

No new tests added, as testing the ConnectorDeleteTableHandle serialization would require a corresponding implementation to test.

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 ==

Prestissimo Changes
* Added a type parameter for `ConnectorDeleteTableHandle` implementations to `ConnectorProtocolTemplate`, along with support for (de)serialization of connector-specific types.  Existing native connector implementations defining `ConnectorProtocolTemplate` specializations will need to update their definitions to supply their specific type or use `NotImplemented`.


Hive Connector Changes
* Updated native `HiveConnectorProtocol` to supply `NotImplemented` for `ConnectorDeleteTableHandle` type.


Iceberg Connector Changes
* Updated native `IcebergConnectorProtocol` to supply `NotImplemented` for `ConnectorDeleteTableHandle` type.

Differential Revision: D70209547

facebook-github-bot · 2025-03-12T23:50:31Z

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

zation99 · 2025-03-17T16:13:17Z

After I run make presto_protocol, there are more changes to the protocol related auto-generated files. The cpp build also failed.

ghelmling · 2025-03-17T17:02:47Z

After I run make presto_protocol, there are more changes to the protocol related auto-generated files. The cpp build also failed.

What is the diff you are seeing after "make presto_protocol"? I ran that as part of this change, but it's possible something else got in after a rebase. Can you share the diff after your build?

CPP build is passing on the CI signals for the PR. What failure do you see there?

Summary: Pull Request resolved: prestodb#24721 Differential Revision: D70209547

facebook-github-bot · 2025-03-17T20:20:32Z

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

zation99 · 2025-03-19T07:12:47Z

presto-native-execution/presto_cpp/presto_protocol/connector/iceberg/IcebergConnectorProtocol.h

@@ -27,6 +27,7 @@ using IcebergConnectorProtocol = ConnectorProtocolTemplate<
    IcebergSplit,
    NotImplemented,
    hive::HiveTransactionHandle,
+    NotImplemented,


There are still build failures related to Iceberg:
In file included from /Users/jiaqizhang/Repos/presto/presto-native-execution/presto_cpp/presto_protocol/presto_protocol.cpp:19:
/Users/jiaqizhang/Repos/presto/presto-native-execution/./presto_cpp/presto_protocol/connector/iceberg/presto_protocol_iceberg.cpp:1074:9: error: no member named 'affinitySchedulingSectionSize' in 'facebook::presto::protocol::iceberg::IcebergSplit'
1074 | p.affinitySchedulingSectionSize,
| ~ ^
/Users/jiaqizhang/Repos/presto/presto-native-execution/./presto_cpp/presto_protocol/connector/iceberg/presto_protocol_iceberg.cpp:1153:9: error: no member named 'affinitySchedulingSectionSize' in 'facebook::presto::protocol::iceberg::IcebergSplit'
1153 | p.affinitySchedulingSectionSize,
| ~ ^
2 errors generated.

why wouldn't this fail in the CI?

Those are not from my changes. It looks like other changes went in without rebuilding presto_protocol.

rschlussel · 2025-03-18T15:39:44Z

presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.cpp

@@ -3251,6 +3251,91 @@ void from_json(const json& j, CreateHandle& p) {
      "schemaTableName");
 }
 } // namespace facebook::presto::protocol
+/*


I know this isn't specific to your change, but why is the license generated a zillion times in this file?

These are coming from the special/*.(cpp|hpp).inc files which are copied in to the protocol files. We could drop the license headers from those or try to make the script smart enough to strip them out (java-to-struct-json.py I think).

rschlussel · 2025-03-19T15:31:17Z

presto-native-execution/presto_cpp/presto_protocol/connector/iceberg/IcebergConnectorProtocol.h

@@ -27,6 +27,7 @@ using IcebergConnectorProtocol = ConnectorProtocolTemplate<
    IcebergSplit,
    NotImplemented,
    hive::HiveTransactionHandle,
+    NotImplemented,


why wouldn't this fail in the CI?

amitkdutta

Thanks @ghelmling for making the protocol up to date and fix the build with new coordiantor.

shangm2

LGTM

NikhilCollooru

lgtm

Summary: Pull Request resolved: #24721 Differential Revision: D70209547

facebook-github-bot · 2025-03-21T22:49:34Z

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

Summary: Pull Request resolved: prestodb#24721 Differential Revision: D70209547

ghelmling requested a review from a team as a code owner March 12, 2025 23:50

facebook-github-bot added the fb-exported label Mar 12, 2025

Add ConnectorDeleteTableHandle to ConnectorProtocol (prestodb#24721)

517c837

Summary: Pull Request resolved: prestodb#24721 Differential Revision: D70209547

ghelmling force-pushed the export-D70209547 branch from 34f75af to 517c837 Compare March 17, 2025 20:20

zation99 approved these changes Mar 19, 2025

View reviewed changes

zation99 requested changes Mar 19, 2025

View reviewed changes

rschlussel reviewed Mar 19, 2025

View reviewed changes

zation99 mentioned this pull request Mar 20, 2025

CPP build failure with broken protocol #24763

Closed

zation99 previously approved these changes Mar 21, 2025

View reviewed changes

Apply missing presto_protocol changes from prior commits

fc33f25

ghelmling dismissed zation99’s stale review via fc33f25 March 21, 2025 05:50

ghelmling requested a review from a team as a code owner March 21, 2025 05:50

ghelmling requested a review from jaystarshot March 21, 2025 05:50

zation99 approved these changes Mar 21, 2025

View reviewed changes

zation99 mentioned this pull request Mar 21, 2025

[native] Add protocol generation and incremental build in debug CI build #24768

Merged

6 tasks

amitkdutta approved these changes Mar 21, 2025

View reviewed changes

This was referenced Mar 21, 2025

[native] Fix Iceberg protocol mapping #24762

Closed

update java class causing cpp protocol issues due to naming discrepancy #24748

Closed

shangm2 approved these changes Mar 21, 2025

View reviewed changes

NikhilCollooru approved these changes Mar 21, 2025

View reviewed changes

ghelmling merged commit 7cdf95b into prestodb:master Mar 21, 2025
98 checks passed

ghelmling added a commit that referenced this pull request Mar 21, 2025

Add ConnectorDeleteTableHandle to ConnectorProtocol (#24721)

212d4ac

Summary: Pull Request resolved: #24721 Differential Revision: D70209547

prestodb-ci mentioned this pull request Mar 28, 2025

Add release notes for 0.292 #24825

Merged

30 tasks

pradeepvaka pushed a commit to pradeepvaka/presto that referenced this pull request Apr 11, 2025

Add ConnectorDeleteTableHandle to ConnectorProtocol (prestodb#24721)

23ac449

Summary: Pull Request resolved: prestodb#24721 Differential Revision: D70209547

unidevel mentioned this pull request Apr 25, 2025

Add release notes for 0.292 unix280/presto#23

Closed

30 tasks

This was referenced May 6, 2025

Add release notes for 0.292 unix280/presto#28

Closed

Add release notes for 0.292 unix280/presto#29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add ConnectorDeleteTableHandle to ConnectorProtocol #24721

Add ConnectorDeleteTableHandle to ConnectorProtocol #24721

Uh oh!

ghelmling commented Mar 12, 2025 •

edited

Loading

Uh oh!

facebook-github-bot commented Mar 12, 2025

Uh oh!

zation99 commented Mar 17, 2025

Uh oh!

ghelmling commented Mar 17, 2025

Uh oh!

facebook-github-bot commented Mar 17, 2025

Uh oh!

zation99 Mar 19, 2025

Uh oh!

rschlussel Mar 19, 2025

Uh oh!

ghelmling Mar 19, 2025

Uh oh!

rschlussel Mar 18, 2025

Uh oh!

ghelmling Mar 19, 2025

Uh oh!

rschlussel Mar 19, 2025

Uh oh!

amitkdutta left a comment

Uh oh!

shangm2 left a comment

Uh oh!

NikhilCollooru left a comment

Uh oh!

Uh oh!

facebook-github-bot commented Mar 21, 2025

Uh oh!

Uh oh!

Add ConnectorDeleteTableHandle to ConnectorProtocol #24721

Add ConnectorDeleteTableHandle to ConnectorProtocol #24721

Uh oh!

Conversation

ghelmling commented Mar 12, 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 Mar 12, 2025

Uh oh!

zation99 commented Mar 17, 2025

Uh oh!

ghelmling commented Mar 17, 2025

Uh oh!

facebook-github-bot commented Mar 17, 2025

Uh oh!

zation99 Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

rschlussel Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

ghelmling Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

rschlussel Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

ghelmling Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

rschlussel Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

amitkdutta left a comment

Choose a reason for hiding this comment

Uh oh!

shangm2 left a comment

Choose a reason for hiding this comment

Uh oh!

NikhilCollooru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented Mar 21, 2025

Uh oh!

Uh oh!

ghelmling commented Mar 12, 2025 •

edited

Loading