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.

Sign up for GitHub

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

Jump to bottom

Add delete node processing in subfield prune optimizer #24206

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Add delete node processing in subfield prune optimizer #24206

merged 1 commit into from
Dec 9, 2024

Conversation

feilong-liu
Copy link
Contributor

@feilong-liu feilong-liu commented Dec 5, 2024 •
edited
Loading

Description

So that it can handle delete query

Motivation and Context

As title.

Impact

Enable it to handle delete query

Test Plan

Existing tests

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

General Changes
* Add delete node in subfield pruning optimizer :pr:`24206`

@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Dec 5, 2024
jaystarshot
jaystarshot previously approved these changes Dec 5, 2024
View reviewed changes
@feilong-liu feilong-liu force-pushed the prunefield_deletenode branch from 3d68d5e to 0adbc57 Compare December 5, 2024 19:27
feilong-liu
feilong-liu commented Dec 5, 2024
View reviewed changes
presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PushdownSubfields.java
context.get().variables.addAll(node.getInputDistribution().get().getInputVariables());
}
context.get().variables.add(node.getRowId());
return context.defaultRewrite(node, context.get());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaystarshot can you review again? I just changed this line from return super.visitDelete(node, context); to context.defaultRewrite(node, context.get()) like other functions in this class. Thanks!

rschlussel
rschlussel reviewed Dec 6, 2024
View reviewed changes
Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a test?

@feilong-liu feilong-liu merged commit cfb487c into prestodb:master Dec 9, 2024
57 checks passed
@feilong-liu feilong-liu deleted the prunefield_deletenode branch December 9, 2024 17:23
This was referenced Jan 28, 2025
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rschlussel rschlussel rschlussel left review comments

@jaystarshot jaystarshot jaystarshot approved these changes

@ClarenceThreepwood ClarenceThreepwood Awaiting requested review from ClarenceThreepwood ClarenceThreepwood is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Assignees
No one assigned
Labels
from:Meta PR from Meta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@feilong-liu @jaystarshot @rschlussel @prestodb-ci