Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DeleteNode to presto_protocol serialization #24274

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghelmling
Copy link
Contributor

Description

This adds basic support for DeleteNode to presto_protocol serialization. This is required for DELETE statement plans to be passed through to Prestissimo workers.

Motivation and Context

This is the first step in adding DELETE query plan support to Prestissimo/Velox, by supporting basic serialization of the Java com.facebook.presto.spi.plan.DeleteNode class and members. While this includes basic (de)serialization, the generated protocol::DeleteNode class is not yet hooked up to any connectors. Conversion to Velox Hive Connector types will follow in a subsequent PR.

Impact

DeleteNode has been added to presto_protocol so it is now available for use by Prestissimo connector implementations.

Test Plan

A basic test, DeleteTest.cpp, is included, which validates round-trip serialization of a sample DeleteNode representation.

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.

== NO RELEASE NOTE ==

@ghelmling ghelmling requested a review from presto-oss December 18, 2024 01:55
@ghelmling ghelmling force-pushed the delete_node_native branch 2 times, most recently from 9600b84 to 1ecbd10 Compare December 19, 2024 19:09
@aditi-pandit
Copy link
Contributor

@ghelmling : Thanks for this change.

Quick question : Do you have a design for this work ? Would be great to read about it.

Please can you separate the velox changes out of this PR. We typically push PRs that only "Advance Velox" e.g. #24135

@ghelmling
Copy link
Contributor Author

Sorry about the velox changes. Those should now be reverted, and I've rebased on the current master.

Unfortunately we do not have a public design doc for this work, as it is based on internal systems. But, in general, you can think of it as similar to positional delete support in Iceberg. The end goal for the full set of Prestissimo/Velox worker changes will be to support writing out delta files identifying the rows which have been deleted from base files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants