Support correlated subquery in UPDATE assignments (SET) by Praveen2112 · Pull Request #8286 · trinodb/trino

Praveen2112 · 2021-06-16T08:36:06Z

Additionally we support MarkDistinct and AssignUniqueId for Update node. It works only for broadcast join and there will be a follow up PR that flips the Join to BROADCAST like we do join SemiJoinNode in Delete operation

Praveen2112 · 2021-06-16T14:22:15Z

...rino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java

We could merge it with the previous tests but for HDP 3.1 it throws an IO Excpetion (https://issues.apache.org/jira/browse/HIVE-22318)

Happy to apply a workaround for it

cc: @losipiuk

We could merge it with the previous tests but for HDP 3.1 it throws an IO Excpetion

is the test run for any other Hive version than 3?

Please merge the test back, so that it's easier to see changes.

see // TODO (https://github.com/trinodb/trino/issues/8268) for the problem you're hitting.

...rino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java

findepi · 2021-06-16T14:59:09Z

...rino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java

Please merge the test back, so that it's easier to see changes.

see // TODO (https://github.com/trinodb/trino/issues/8268) for the problem you're hitting.

core/trino-main/src/test/java/io/trino/sql/planner/TestLogicalPlanner.java

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

ksobolew · 2021-06-16T15:04:17Z

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

nit: I would love to see in the commit description why this check is no longer needed :)

👍 Why this check is removed?

Now with support for correlated queries for assignment...we would translate them to a LEFT JOIN with a non SCALAR build source.

moved to top level discussion @ #8286 (comment)

findepi · 2021-06-17T12:34:25Z

@sopel39 can you take a look at property derivations?

sopel39 · 2021-06-17T14:21:24Z

@sopel39 can you take a look at property derivations?

These look good

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

...rino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java

findepi · 2021-06-21T09:52:03Z

...rino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java

Any particular reason to move this test case around?
Please keep it here for smaller diff

findepi · 2021-06-21T09:52:21Z

...rino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java

Add a comment hinting why this test case is interesting

There is a comment above which specifies the interesting reason for it.

i see a blank line above this query, which comment are you referring to?

I was referring the comment in Line 1420.

...rino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java

kokosing · 2021-06-21T11:37:06Z

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

👍 Why this check is removed?

findepi · 2021-06-21T11:46:52Z

@Praveen2112 do you consider this still a WIP?

core/trino-main/src/main/java/io/trino/sql/analyzer/Analyzer.java

core/trino-main/src/main/java/io/trino/sql/analyzer/Analysis.java

Praveen2112 · 2021-10-04T13:54:20Z

@martint AC

core/trino-main/src/main/java/io/trino/sql/analyzer/QueryType.java

This would remove the Analysis#resetUpdateType as the update type should never even be set to Update/delete for explain analyze queries

This makes the check if a table is update target or not independent on current state of the update type.

Praveen2112 · 2021-10-06T10:20:22Z

@martint AC

.

kasiafi · 2021-10-08T07:56:50Z

core/trino-main/src/main/java/io/trino/sql/analyzer/Analysis.java

This is used when creating a TableScanNode to set the updateTarget flag. Is it intentional that after this change we set the flag to true for ANALYZE, CREATE MATERIALIZED VIEW etc., while before this change it was set only for UPDATE and DELETE queries?

It will be still set only for UPDATE and DELETE queries - as targetTable is set only for those two types of queries and isUpdateTarget will return false if the targetTable is Optional#empty

core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/test/PlanBuilder.java

Optimizer could flip the join and we don't want to apply the delete/update for incorrect table.

When we pass a correlated query as an assignment to a column we would translate them to a LEFT JOIN with a non SCALAR build source so current restriction is removed.

Praveen2112 · 2021-10-08T08:48:20Z

@kasiafi Thanks for the review. AC

Praveen2112 added the WIP label Jun 16, 2021

cla-bot bot added the cla-signed label Jun 16, 2021

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch 4 times, most recently from 68af1d5 to 836ba76 Compare June 16, 2021 13:12

Praveen2112 commented Jun 16, 2021

View reviewed changes

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch from 836ba76 to 604e777 Compare June 16, 2021 14:23

Praveen2112 marked this pull request as ready for review June 16, 2021 14:23

Praveen2112 requested review from findepi, kasiafi, ksobolew, lukasz-walkiewicz and martint June 16, 2021 14:24

findepi added the tests:hive label Jun 16, 2021

findepi reviewed Jun 16, 2021

View reviewed changes

ksobolew reviewed Jun 16, 2021

View reviewed changes

findepi requested review from dain, electrum and sopel39 June 16, 2021 15:06

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch from fb737bc to 4055b11 Compare June 17, 2021 11:44

martint reviewed Jun 18, 2021

View reviewed changes

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

findepi reviewed Jun 21, 2021

View reviewed changes

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch from 4055b11 to 73e3911 Compare June 21, 2021 10:45

Praveen2112 changed the title ~~Support correlated subquery in UPDATE SET~~ Support correlated subquery in UPDATE assignments (SET) Jun 21, 2021

kokosing approved these changes Jun 21, 2021

View reviewed changes

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

Copy link

Member

kokosing Jun 21, 2021

Choose a reason for hiding this comment

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

👍 Why this check is removed?

Praveen2112 removed the WIP label Jun 21, 2021

Praveen2112 requested a review from martint September 30, 2021 10:21

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch from 1f28f71 to 6125f3c Compare September 30, 2021 12:23

Praveen2112 mentioned this pull request Sep 30, 2021

Support correlated subqueries for DELETE #9447

Merged

martint reviewed Sep 30, 2021

View reviewed changes

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch from 6125f3c to 8d471a6 Compare October 4, 2021 13:46

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch from 8d471a6 to 003984f Compare October 5, 2021 09:32

martint reviewed Oct 5, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/analyzer/QueryType.java Outdated Show resolved Hide resolved

Praveen2112 added 4 commits October 6, 2021 15:46

Introduce QueryType enum for Analysis

9a08ffc

Specify if a query is of Explain/Explain Analyze during Analysis

50fafb7

This would remove the Analysis#resetUpdateType as the update type should never even be set to Update/delete for explain analyze queries

Fix tracking of update target for EXPLAIN ANALYZE queries

b227200

This makes the check if a table is update target or not independent on current state of the update type.

Set updateType once for Materialized views refresh

b7702a8

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch from 003984f to b2173e0 Compare October 6, 2021 10:19

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch from b2173e0 to 93e9caf Compare October 7, 2021 06:44

martint approved these changes Oct 7, 2021

View reviewed changes

findepi added the enhancement New feature or request label Oct 7, 2021

kasiafi reviewed Oct 8, 2021

View reviewed changes

Praveen2112 added 4 commits October 8, 2021 14:17

Ensure delete/update is applied on actual table specified in query

a1f1cf3

Optimizer could flip the join and we don't want to apply the delete/update for incorrect table.

Assert plan for correlated subqueries with automatic join distribution

6db2338

Specify stream partitioning for AssignUniqueId in PropertyDerivations

093249d

Support correlated subquery in UPDATE assignments (SET)

06b408d

When we pass a correlated query as an assignment to a column we would translate them to a LEFT JOIN with a non SCALAR build source so current restriction is removed.

Praveen2112 force-pushed the praveen/010_2/set_correlated_queries branch from 93e9caf to 06b408d Compare October 8, 2021 08:48

kasiafi approved these changes Oct 8, 2021

View reviewed changes

Praveen2112 merged commit a72a739 into trinodb:master Oct 8, 2021

github-actions bot added this to the 364 milestone Oct 8, 2021

Praveen2112 mentioned this pull request Oct 18, 2021

Release notes for 364 #9534

Closed

12 tasks

findepi mentioned this pull request Nov 25, 2021

UPDATE statement #3325

Closed

8 tasks

Conversation

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

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!

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!

findepi commented Jun 17, 2021

Uh oh!

sopel39 commented Jun 17, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Jun 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Praveen2112 commented Oct 4, 2021

Uh oh!

Uh oh!

Praveen2112 commented Oct 6, 2021

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!

Praveen2112 commented Oct 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

Praveen2112 commented Jun 16, 2021 •

edited

Loading