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

Remove empty UPDATE also when not wrapped with exchange #13794

Closed
wants to merge 2 commits into from

Conversation

findinpath
Copy link
Contributor

@findinpath findinpath commented Aug 23, 2022

Description

Replace UPDATE plan node with an empty VALUES node when the predicate is optimized to false.

Is this change a fix, improvement, new feature, refactoring, or other?

Fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Core query engine

How would you describe this change to a non-technical end user or system administrator?

Support queries like the following:

UPDATE table_name SET col1 = 'some_value' WHERE false;

Related issues, pull requests, and links

Fixes #12422
Enhances to #13755

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
(x) Release notes entries required with the following suggested text:

RN done via #13755

@cla-bot cla-bot bot added the cla-signed label Aug 23, 2022
@findinpath findinpath changed the title Update where false Remove empty UPDATE also when wrapped with exchange Aug 23, 2022
@findinpath findinpath marked this pull request as ready for review August 23, 2022 10:33
implements Rule<TableFinishNode>
{
private static final Pattern<TableFinishNode> PATTERN = tableFinish()
.with(source().matching(exchange()
Copy link
Member

Choose a reason for hiding this comment

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

Remove empty UPDATE also when wrapped with exchange

i think the "wrapped with exchange" case was satisfied.

Copy link
Member

Choose a reason for hiding this comment

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

I merged #13755 without resolving #13755 (comment) because I don't know whether it matters. I didn't want to have a dead code.

@findinpath findinpath changed the title Remove empty UPDATE also when wrapped with exchange Remove empty UPDATE also when not wrapped with exchange Aug 23, 2022
@findepi findepi mentioned this pull request Aug 23, 2022
Replace the `UPDATE` plan node with an empty `VALUES`
node when the predicate is optimized to `false`.
@findinpath
Copy link
Contributor Author

While testing the PR i've stumbled on the following edge case:

UPDATE mock.default.nation SET name = 'x' WHERE name IN (SELECT name FROM mock.default.nation WHERE false)
io.trino.testing.QueryFailedException: Invalid descendant for DeleteNode or UpdateNode: io.trino.sql.planner.plan.ExchangeNode

	at io.trino.testing.AbstractTestingTrinoClient.execute(AbstractTestingTrinoClient.java:123

The plan node for the query above looks like:

ExchangeNode
  UpdateNode
    ProjectNode
      FilterNode
        ProjectNode
          SemiJoinNode
            ExchangeNode
              Values (rowCount = 0)

@findepi
Copy link
Member

findepi commented Aug 23, 2022

@findinpath reported #13803 for this case.

@findepi
Copy link
Member

findepi commented Aug 23, 2022

@findinpath before this PR, is there a query that fails that wouldn't fail after this PR?

what is an example of a query which produces different plans before and after this PR?

@findinpath
Copy link
Contributor Author

what is an example of a query which produces different plans before and after this PR?

To be honest, I don't have yet a usecase for an UpdateNode not wrapped by an exchange node.
I'll close the PR for this reason.

@findinpath findinpath closed this Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

UPDATE does not support WHERE false
2 participants