delete with multi columns IN Predicate #1001

aman-db · 2024-10-14T17:41:04Z

closes #953
Grammar changes required to handle queries like

DELETE FROM test_tbl
WHERE (version, type) IN (
    SELECT DISTINCT version, type
     FROM test_tbl_stg
);

Implemented a new visitor method - visitPredExprListIn
Added code to change the Optimized Plan for Delete with Exists

github-actions · 2024-10-14T17:43:47Z

Coverage tests results

454 tests +6 419 ✅ +5 4s ⏱️ ±0s
6 suites ±0 35 💤 +1
6 files ±0 0 ❌ ±0

Results for commit 2e50a96. ± Comparison against base commit 50771bf.

♻️ This comment has been updated with latest results.

jimidle

It is on the right track, assuming we wish to change ir.IN() like this. If we are chaing that, then make sure that TSQL changes if it needs to.

jimidle · 2024-10-16T21:16:00Z

core/src/main/antlr4/com/databricks/labs/remorph/parsers/snowflake/SnowflakeParser.g4

@@ -3557,7 +3557,7 @@ predicate
    | expression comparisonOperator expression                                                   # predBinop
    | expression comparisonOperator (ALL | SOME | ANY) L_PAREN subquery R_PAREN                  # predASA
    | expression IS NOT? NULL                                                                    # predIsNull
-    | expression NOT? IN L_PAREN (subquery | exprList) R_PAREN                                   # predIn
+    | (L_PAREN exprList R_PAREN | expression) NOT? IN L_PAREN (subquery | exprList) R_PAREN      # predIn


| expression NOT? IN L_PAREN (subquery | exprList) R_PAREN # predIn | L_PAREN exprList R_PAREN NOT? IN L_PAREN (subquery | exprList) R_PAREN # predExprListIn

keep these separate with a separate visitor then there is no need to do complex logic to see whether there is an exprList or an expression. visitpredExprListIn will autmatically know that it is two expression lists.

jimidle · 2024-10-16T21:19:04Z

...rc/main/scala/com/databricks/labs/remorph/parsers/snowflake/SnowflakeExpressionBuilder.scala

+    }
+    left.size match {
+      case 1 =>
+        val in = ir.In(left.head, right) // If there's only one expression on the left side


Revert to the existing code for visitPredIn and implement visitPredExprListIn if there is overlap then share the overlap with a buildXXX def to handle the common features so that the visitors are actually quite simple

jimidle · 2024-10-16T21:19:54Z

...main/scala/com/databricks/labs/remorph/parsers/snowflake/rules/DeleteOnMultipleColumns.scala

+      print(s"subquery: $subquery")
+      val newSubquery = transformSubquery(target, subquery)
+      delete.copy(where = Some(Exists(newSubquery)))
+//      delete.copy(where = newSubquery)


Remove commented out code.

This commented out code is still there.

jimidle · 2024-10-21T13:39:28Z

...rc/main/scala/com/databricks/labs/remorph/parsers/snowflake/SnowflakeExpressionBuilder.scala

+  override def visitPredExprListIn(ctx: PredExprListInContext): ir.Expression = {
+    // left side of In can be a list of expressions
+    val leftExprList = ctx.exprList(0).expr().asScala.map(_.accept(this))


// LHS is a list of expressions

jimidle

We need to reinstate the errorCheck(ctx) match { staements

jimidle · 2024-10-25T19:49:25Z

core/src/main/antlr4/com/databricks/labs/remorph/parsers/snowflake/SnowflakeParser.g4

@@ -3558,6 +3558,7 @@ predicate
    | expression comparisonOperator (ALL | SOME | ANY) L_PAREN subquery R_PAREN                  # predASA
    | expression IS NOT? NULL                                                                    # predIsNull
    | expression NOT? IN L_PAREN (subquery | exprList) R_PAREN                                   # predIn
+    | L_PAREN exprList R_PAREN  NOT? IN L_PAREN (subquery | exprList) R_PAREN                    # predExprListIn


Looks like the ANTLR formatter has been disabled. Extra space crept in there.

jimidle · 2024-10-25T19:51:45Z

...rc/main/scala/com/databricks/labs/remorph/parsers/snowflake/SnowflakeExpressionBuilder.scala

-  override def visitPredIn(ctx: PredInContext): ir.Expression = errorCheck(ctx) match {
-    case Some(errorResult) => errorResult
-    case None =>
-      val in = if (ctx.subquery() != null) {
-        // In the result of a sub query
-        ir.In(ctx.expression().accept(this), Seq(ir.ScalarSubquery(ctx.subquery().accept(vc.relationBuilder))))
-      } else {
-        // In a list of expressions
-        ir.In(ctx.expression().accept(this), ctx.exprList().expr().asScala.map(_.accept(this)))


You have deleted the error checking match. That needs to go back in so that errorNodes are generated

jimidle · 2024-10-25T19:52:21Z

...rc/main/scala/com/databricks/labs/remorph/parsers/snowflake/SnowflakeExpressionBuilder.scala

+  override def visitPredExprListIn(ctx: PredExprListInContext): ir.Expression = {
+    // left side of In is a list of expressions
+    val leftExprList = ctx.exprList(0).expr().asScala.map(_.accept(this))
+


Error handling needs to go back in

...main/scala/com/databricks/labs/remorph/parsers/snowflake/rules/DeleteOnMultipleColumns.scala

sundarshankar89 · 2024-10-28T03:25:00Z

tests/resources/functional/snowflake/dml/delete/test_delete_with_not_in.sql

+-- snowflake sql:
+DELETE FROM test_tbl
+WHERE (version, type) NOT IN (
+SELECT version1 , type2


Can you add more test case to check the behaviour., when select column names are expressions.

sundarshankar89 · 2024-11-06T05:08:04Z

core/src/test/scala/com/databricks/labs/remorph/coverage/AcceptanceSpec.scala

@@ -34,7 +34,9 @@ class SnowflakeAcceptanceSuite
            "test_command/test_command_3.sql",
            "test_skip_unsupported_operations/test_skip_unsupported_operations_7.sql",
            "test_skip_unsupported_operations/test_skip_unsupported_operations_9.sql",
-            "test_skip_unsupported_operations/test_skip_unsupported_operations_10.sql"),
+            "test_skip_unsupported_operations/test_skip_unsupported_operations_10.sql",
+            "core_engine/delete/test_delete_with_not_in.sql",


why are these added in ignored test?

aman-db added the do-not-merge label Oct 14, 2024

aman-db force-pushed the feature/multi_column_delete_to_exist branch from cc8fe09 to 057e3f9 Compare October 15, 2024 12:17

aman-db requested review from nfx, jimidle and sundarshankar89 October 16, 2024 18:01

aman-db changed the title ~~initial commit for delete with multi columns~~ delete with multi columns IN Predicate Oct 16, 2024

jimidle requested changes Oct 16, 2024

View reviewed changes

jimidle requested changes Oct 21, 2024

View reviewed changes

aman-db added 10 commits October 26, 2024 00:44

initial commit for delete with multi columns

e68a345

revert to original grammar

f1fb726

added vistExprList

387f638

change in grammar

9fea8b2

change in vistorPredIn and grammar

ac43dec

change in grammar

0066949

removed comments

705228a

updating otimizer plan for delete on multiple columns

3eb27da

adding functional tests

8325a74

resolved merge conflict

c518114

aman-db force-pushed the feature/multi_column_delete_to_exist branch from c94d95c to c518114 Compare October 25, 2024 19:42

jimidle requested changes Oct 25, 2024

View reviewed changes

jimidle assigned aman-db Oct 25, 2024

aman-db added 2 commits October 26, 2024 01:37

added new test

a9c8ff6

fixed formating issues in delete tests

566c8cb

sundarshankar89 requested changes Oct 28, 2024

View reviewed changes

aman-db and others added 2 commits November 5, 2024 11:17

Merge branch 'main' into feature/multi_column_delete_to_exist

8c1d673

moved functional tests to a different folder

4fe3205

aman-db removed the do-not-merge label Nov 5, 2024

aman-db marked this pull request as ready for review November 5, 2024 11:06

sundarshankar89 requested changes Nov 6, 2024

View reviewed changes

handled not exists condition

d5bd536

aman-db marked this pull request as draft November 6, 2024 09:54

aman-db added the do-not-merge label Nov 6, 2024

Merge branch 'main' into feature/multi_column_delete_to_exist

8058738

aman-db marked this pull request as ready for review November 6, 2024 11:03

aman-db removed the do-not-merge label Nov 6, 2024

aman-db requested review from jimidle and sundarshankar89 November 6, 2024 11:03

Merge branch 'main' into feature/multi_column_delete_to_exist

4ae2519

aman-db marked this pull request as draft November 11, 2024 14:59

aman-db added the do-not-merge label Nov 11, 2024

aman-db and others added 2 commits November 13, 2024 16:41

change in code to handle expressions in delete statements

f9e38f8

Merge branch 'main' into feature/multi_column_delete_to_exist

2e50a96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delete with multi columns IN Predicate #1001

delete with multi columns IN Predicate #1001

aman-db commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

jimidle left a comment

jimidle Oct 16, 2024

aman-db Nov 5, 2024

jimidle Oct 16, 2024 •

edited

Loading

aman-db Nov 5, 2024

jimidle Oct 16, 2024

jimidle Oct 21, 2024

aman-db Nov 5, 2024

jimidle Oct 21, 2024

aman-db Nov 11, 2024

jimidle left a comment

jimidle Oct 25, 2024

aman-db Nov 11, 2024

jimidle Oct 25, 2024

aman-db Nov 5, 2024

jimidle Oct 25, 2024

aman-db Nov 5, 2024

sundarshankar89 Oct 28, 2024

sundarshankar89 Nov 6, 2024

aman-db Nov 6, 2024

delete with multi columns IN Predicate #1001

Are you sure you want to change the base?

delete with multi columns IN Predicate #1001

Conversation

aman-db commented Oct 14, 2024 • edited Loading

github-actions bot commented Oct 14, 2024 • edited Loading

Coverage tests results

jimidle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimidle Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimidle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aman-db commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

jimidle Oct 16, 2024 •

edited

Loading