Support correlated subqueries for DELETE by Praveen2112 · Pull Request #9447 · trinodb/trino

Praveen2112 · 2021-09-30T12:34:11Z

Description

Support correlated subqueries for DELETE

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

Improvement for DELETE operations

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?

Related issues, pull requests, and links

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

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

# Section
* Support correlated subqueries for DELETE

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateJoinInDelete.java

Praveen2112 · 2021-10-08T11:32:59Z

@kasiafi AC

findepi · 2021-11-05T16:18:27Z

@Praveen2112 please rebase

...-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateJoinAndSemiJoinInDelete.java

takezoe · 2022-01-31T16:37:37Z

Hi, is there any progress on this? We are facing a similar case and would like to fix the issue if possible.

Praveen2112 · 2022-02-01T13:15:03Z

Sorry for the delay, will rebase and apply the comments by this week.

findepi · 2022-05-05T12:19:35Z

@Praveen2112 did you have a chance to look at https://github.com/trinodb/trino/pull/9447/files#r756968059 ?

Praveen2112 · 2022-05-05T12:51:24Z

@findepi Yes. I think we could implement it by re-wiriting only the probe side (for INNER and LEFT join) and leave build side part of the plan. Is my understanding is correct ? I have updated the PR based on this above mentioned approach.

findepi · 2022-05-13T08:06:46Z

@Praveen2112 can you please check the build status?

Praveen2112 · 2022-05-16T09:13:19Z

@findepi The build is now green.

findepi · 2022-05-17T12:44:14Z

(squashed a fixup)

findepi

"Support correlated EXISTS subquery for DELETE"

...-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateJoinAndSemiJoinInDelete.java

findepi · 2022-05-17T12:46:56Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

I don't understand why ReplicateSemiJoinInDelete/ReplicateJoinAndSemiJoinInDelete needs to be run before ReorderJoins.
I think ReplicateSemiJoinInDelete/ReplicateJoinAndSemiJoinInDelete would undo distirbution type picked by ReorderJoins. What am i missing?

Since ReorderJoins can flip the JoinNode - we would like to avoid it for the PlanNode between Delete and TableScan. So we run them before applying these optimizer.

This is not a good way to solve this issue. Optimizer rules need to be able to run regardless of which order they are applied (i.e., correctness of the plan should not depend on the order of rule application -- only query performance should be affected).

How is this PR different from what we did for #8286 ?

...-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateJoinAndSemiJoinInDelete.java

findepi · 2022-05-17T12:49:26Z

...-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateJoinAndSemiJoinInDelete.java

i guess this should be:

if (isDeleteQuery) { verify(joinType == INNER || joinType == LEFT

what about UPDATE?

UPDATE can be applied in a follow up PR.

i guess this should be:

if (isDeleteQuery) { verify(joinType == INNER || joinType == LEFT

has this been resolved?
or, am i being wrong here?

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

sopel39 · 2022-05-19T09:30:03Z

Which connectors support these? Should we add test cases to BaseConnectorTest?

Praveen2112 · 2022-05-23T06:43:46Z

Which connectors support these?

Hive currently supports this.

Should we add test cases to BaseConnectorTest?
We have added to BaseConnectorTest.

Praveen2112 · 2022-06-21T14:12:49Z

@findepi AC. Except migration of tests from BCT - which we might address in a different PR. WDYT ?

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateSemiJoinInDelete.java

findepi · 2022-07-18T11:05:45Z

...-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateJoinAndSemiJoinInDelete.java

i guess this should be:

if (isDeleteQuery) { verify(joinType == INNER || joinType == LEFT

has this been resolved?
or, am i being wrong here?

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

findepi · 2022-07-18T11:08:57Z

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/BeginTableWrite.java

cc @electrum @martint

...-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateJoinAndSemiJoinInDelete.java

Praveen2112 · 2022-07-18T12:30:22Z

@findepi Thanks for the review. AC

findepi · 2022-07-20T16:49:02Z

thanks for the update

pending comments

Praveen2112 · 2022-07-20T17:05:07Z

I wasn't able to add the comment there. So will add it here

This would make the query to fail a bit early at ReplicateJoinAndSemiJoinOptimizer instead of BeginTableWriteOptimizer. ReplicateJoinAndSemiJoinOptimizer mostly deals with configuring the distribution type for JoinNode not sure if we need to do this verification here. Additionally it would be better if we track all the failures related to plan structure at one point.

@findepi WDYT ?

findepi · 2022-07-21T07:12:50Z

Oh, this is because io.trino.sql.planner.optimizations.ReplicateJoinAndSemiJoinInDelete.Rewriter#isDeleteQuery is a field, not a context variable, so it doesn't distinguish between "main delete path" and the subqueries, where it shouldn't apply optimizations.
Fine, let's keep as-is for now.

#9447 (comment) remains outstanding.

findepi · 2022-07-21T07:17:03Z

Oh, this is because io.trino.sql.planner.optimizations.ReplicateJoinAndSemiJoinInDelete.Rewriter#isDeleteQuery is a field, not a context variable,

i take this back. ReplicateJoinAndSemiJoinInDelete doesn't recurse into right sides of a join or semi-join.
Where does non-INNER join come from?

Praveen2112 · 2022-07-21T07:33:17Z

ReplicateJoinAndSemiJoinInDelete doesn't recurse into right sides of a join or semi-join.

True.

Where does non-INNER join come from?

It comes when we translate correlated subqueries

Joins distribution would be set to BROADCAST in case of DELETE

Praveen2112 · 2022-07-27T14:41:35Z

@martint Sorry for continuing it here as I’am unable to add comments in that there.

The main issue here is that - DELETE or UPDATE query depends heavily on the pipeline if we could remove that dependency then the optimizers can be independent of each other. What are the other directions we could take for solving this issue ?

This PR is kind of an enhancement to #8286, #8286 solves the issue which was bcoz of AssignUniqueIdNode which creates an additional (unnecessary) exchanges in the plan. This PR solves the issue specific join which creates additional exchange node bcoz of partitioning join node.

findepi · 2022-07-29T09:14:30Z

CI failed (https://github.com/trinodb/trino/runs/7542141425?check_suite_focus=true). I rerun it.

colebow · 2022-07-29T16:55:22Z

@Praveen2112 - could you propose a release note for this fix?
cc @findepi

Praveen2112 · 2022-07-29T17:13:29Z

I have updated in PR message.

findepi · 2022-08-01T14:28:52Z

re #9447 (comment)
and #9447 (comment)

(can't reply inline..)

Optimizer rules need to be able to run regardless of which order they are applied

That's an oversimplification. There are some dependencies. For example

// PushJoinIntoTableScan must run after ReorderJoins (and DetermineJoinDistributionType)

correctness of the plan should not depend on the order of rule application

Absolutely. I want to stress this out, this is paramount.

(BTW I was convinced this isn't the case here.)

cla-bot bot added the cla-signed label Sep 30, 2021

Praveen2112 requested review from kasiafi and martint September 30, 2021 12:35

kasiafi reviewed Oct 5, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateJoinInDelete.java Outdated Show resolved Hide resolved

Praveen2112 force-pushed the praveen/011_2/delete_correlated_queries branch 2 times, most recently from a6c538d to 86e81bd Compare October 8, 2021 11:22

Praveen2112 marked this pull request as ready for review October 8, 2021 11:32

Praveen2112 requested a review from kasiafi October 12, 2021 09:47

findepi requested a review from sopel39 October 12, 2021 11:48

Praveen2112 force-pushed the praveen/011_2/delete_correlated_queries branch 2 times, most recently from 7e1f08e to 1136816 Compare November 8, 2021 07:37

findepi reviewed Nov 25, 2021

View reviewed changes

...-main/src/main/java/io/trino/sql/planner/optimizations/ReplicateJoinAndSemiJoinInDelete.java Outdated Show resolved Hide resolved

Praveen2112 force-pushed the praveen/011_2/delete_correlated_queries branch from 1136816 to 98f0e28 Compare May 2, 2022 15:09

Praveen2112 requested a review from findepi May 10, 2022 14:58

findepi force-pushed the praveen/011_2/delete_correlated_queries branch from c7b3865 to c726f4d Compare May 17, 2022 12:43

findepi reviewed May 17, 2022

View reviewed changes

Praveen2112 force-pushed the praveen/011_2/delete_correlated_queries branch from c726f4d to da82491 Compare June 21, 2022 14:06

Praveen2112 requested a review from findepi June 22, 2022 05:46

findepi reviewed Jul 18, 2022

View reviewed changes

Praveen2112 force-pushed the praveen/011_2/delete_correlated_queries branch from 7366f38 to 6d3fd8d Compare July 18, 2022 12:30

findepi requested a review from electrum July 21, 2022 07:13

Praveen2112 added 3 commits July 25, 2022 21:58

Use PlanNode#replaceChildren for creating new PlanNode explicitly

df7f943

Support correlated EXISTS subquery for DELETE

180101b

Joins distribution would be set to BROADCAST in case of DELETE

Support correlated IN subquery for DELETE

c081884

Praveen2112 force-pushed the praveen/011_2/delete_correlated_queries branch from 6d3fd8d to c081884 Compare July 25, 2022 16:29

empty

245e24f

findepi merged commit f1d47c3 into trinodb:master Jul 29, 2022

github-actions bot added this to the 392 milestone Jul 29, 2022

colebow mentioned this pull request Jul 29, 2022

Add Trino 392 release notes #13342

Closed

Conversation

Praveen2112 commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

Uh oh!

Praveen2112 commented Oct 8, 2021

Uh oh!

findepi commented Nov 5, 2021

Uh oh!

Uh oh!

takezoe commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Praveen2112 commented Feb 1, 2022

Uh oh!

findepi commented May 5, 2022

Uh oh!

Praveen2112 commented May 5, 2022

Uh oh!

findepi commented May 13, 2022

Uh oh!

Praveen2112 commented May 16, 2022

Uh oh!

findepi commented May 17, 2022

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sopel39 commented May 19, 2022

Uh oh!

Praveen2112 commented May 23, 2022

Uh oh!

Praveen2112 commented Jun 21, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Praveen2112 commented Jul 18, 2022

Uh oh!

findepi commented Jul 20, 2022

Uh oh!

Praveen2112 commented Jul 20, 2022

Uh oh!

findepi commented Jul 21, 2022

Uh oh!

findepi commented Jul 21, 2022

Uh oh!

Praveen2112 commented Jul 21, 2022

Uh oh!

Praveen2112 commented Jul 27, 2022

Uh oh!

findepi commented Jul 29, 2022

Praveen2112 commented Sep 30, 2021 •

edited

Loading

takezoe commented Jan 31, 2022 •

edited

Loading