[SPARK-49017][SQL] Insert statement fails when multiple parameters are being used #47501

mihailom-db · 2024-07-26T12:52:51Z

What changes were proposed in this pull request?

Fix for multiple parameters support.

Why are the changes needed?

The use of multiple parameters with identifiers were broken

Does this PR introduce any user-facing change?

Yes, look at tests.

How was this patch tested?

New tests added to ParametersSuite.

Was this patch authored or co-authored using generative AI tooling?

No.

This reverts commit d54453e.

mihailom-db · 2024-07-30T08:04:45Z

@cloud-fan could you take a look at this PR?

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/parameters.scala

cloud-fan · 2024-08-02T15:24:49Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveIdentifierClause.scala

    _.containsAnyPattern(UNRESOLVED_IDENTIFIER, UNRESOLVED_IDENTIFIER_WITH_CTE)) {
    case p: PlanWithUnresolvedIdentifier if p.identifierExpr.resolved =>
-      executor.execute(p.planBuilder.apply(evalIdentifierExpr(p.identifierExpr)))
+      executor.execute(p.planBuilder.apply(evalIdentifierExpr(p.identifierExpr), p.otherPlans))


I think a problem is when to invoke the plan builder. Shall we wait for all p.otherPlans to be resolved first? Without a clear trigger condition, we may hit rule order issues.

Yeah, that is true. That is why I tried to add a couple of tests with interleaving of identifiers and non-identifiers. I can pass one more time through AstBuilder, as I believe only InsertInto and one more LogicalPlans were changed to have otherPlans as parameters.

sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

cloud-fan · 2024-08-08T08:44:40Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

      // It's a function call
      val funcCtx = ctx.functionName
-      val func = withFuncIdentClause(
+      val func: Expression = withFuncIdentClause(


unnecessary change?

Nope, there are two withFuncIdentClause, one that takes LogicalPlan and other with Expression. Compiler was not smart to pick up which one it was.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala

cloud-fan · 2024-08-14T17:27:10Z

@mihailom-db can you fix the merge conflicts?

# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

mihailom-db · 2024-08-19T08:14:36Z

@cloud-fan resolved conflicts, ready for merge

mihailom-db · 2024-08-19T08:16:20Z

@MaxGekk could you take a look?

sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala

MaxGekk · 2024-08-19T09:17:11Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

            partition,
            cols,
-            query,
+            otherPlans.head,


Are you sure that everywhere you call .head, it won't fail with NoSuchElementException. Could you clarify your assumption, please.

For all the places where we call .head we have otherPlans that was defined as Seq(query), so it will have exactly one element. But Seq was used, if in the future the need for multiple plans is needed.

MaxGekk

The last changes are trivial. Just waiting for a success build.

MaxGekk · 2024-08-19T10:22:04Z

+1, LGTM. Merging to master.
Thank you, @mihailom-db and @cloud-fan for review.

MaxGekk · 2024-08-19T10:24:27Z

@mihailom-db You pointed out only 4.0.0 as the affected version in SPARK-49017. Do the previous versions suffer from the same issue, and shall we backport the changes to other branches. Please, double check and confirm.

mihailom-db · 2024-08-19T12:45:40Z

@MaxGekk checked 3.4 does not have option of parameter markers in IDENTIFIER(...), but 3.5 seems to suffer from the same error, so backport is needed

MaxGekk · 2024-08-19T13:05:43Z

but 3.5 seems to suffer from the same error, so backport is needed

Please, open a PR with the backport to branch-3.5.

mihailom-db · 2024-08-20T06:28:26Z

@MaxGekk I have checked now, and there are multiple fixes that were not backported to branch-3.5, so I would say this one does not have to be backported as well. For example a fix for ExpressionWithUnresolvedIdentifier was not backported and this blocks backport of this PR as well.

MaxGekk · 2024-08-20T07:31:29Z

I have checked now, and there are multiple fixes that were not backported to branch-3.5

@mihailom-db Please, open separate PRs for the branch-3.5 with bug fixes.

…rs are being used ### What changes were proposed in this pull request? Fix for multiple parameters support. This PR is a backport of #47501. ### Why are the changes needed? The use of multiple parameters with identifiers were broken ### Does this PR introduce _any_ user-facing change? Yes, look at tests. ### How was this patch tested? New tests added to ParametersSuite. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #47814 from mihailom-db/fixParametersBackport. Authored-by: Mihailo Milosevic <mihailo.milosevic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…e being used ### What changes were proposed in this pull request? Fix for multiple parameters support. ### Why are the changes needed? The use of multiple parameters with identifiers were broken ### Does this PR introduce _any_ user-facing change? Yes, look at tests. ### How was this patch tested? New tests added to ParametersSuite. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47501 from mihailom-db/fixParameters. Authored-by: Mihailo Milosevic <mihailo.milosevic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…dentifier ### What changes were proposed in this pull request? This PR reverts #46580 (the tests are left) because it's no longer needed after #47501 . The `PlanWithUnresolvedIdentifier` becomes more flatten and all its children will be resolved by the early batch already. ### Why are the changes needed? code cleanup ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests ### Was this patch authored or co-authored using generative AI tooling? no Closes #48786 from cloud-fan/ident. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…dentifier ### What changes were proposed in this pull request? This PR reverts apache/spark#46580 (the tests are left) because it's no longer needed after apache/spark#47501 . The `PlanWithUnresolvedIdentifier` becomes more flatten and all its children will be resolved by the early batch already. ### Why are the changes needed? code cleanup ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests ### Was this patch authored or co-authored using generative AI tooling? no Closes #48786 from cloud-fan/ident. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

mihailom-db added 13 commits May 30, 2024 09:51

Fix OracleIntegrationSuite for JDK8

d54453e

Revert "Fix OracleIntegrationSuite for JDK8"

7d00764

This reverts commit d54453e.

Merge remote-tracking branch 'upstream/master'

ad01ff9

Merge remote-tracking branch 'upstream/master'

1592c78

Merge remote-tracking branch 'upstream/master'

7fe2a4d

Merge remote-tracking branch 'upstream/master'

8e811d4

Merge remote-tracking branch 'upstream/master'

7586b5f

Merge remote-tracking branch 'upstream/master'

92ac42b

Merge remote-tracking branch 'upstream/master'

9ad7a89

Merge remote-tracking branch 'upstream/master'

cf4fbdf

Add change

d2a27b6

Add full fix

6d0f64d

Add innerChildren

3adc765

github-actions bot added the SQL label Jul 26, 2024

Fix test names

9b39103

HyukjinKwon changed the title ~~[SPARK-49017] Insert statement fails when multiple parameters are being used~~ [SPARK-49017][SQL] Insert statement fails when multiple parameters are being used Jul 27, 2024

mihailom-db added 2 commits July 29, 2024 08:40

Merge remote-tracking branch 'upstream/master' into fixParameters

1c2319e

Fix errors

1967a91

mihailom-db added 2 commits August 1, 2024 13:54

Improve tests and rework bind

79c143a

Fix pattern

cefabfe

cloud-fan reviewed Aug 2, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Aug 2, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/parameters.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Aug 2, 2024

View reviewed changes

mihailom-db added 2 commits August 5, 2024 08:31

Incorporate changes

39d957b

Remove unexpected changes

8983655

cloud-fan reviewed Aug 5, 2024

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Aug 5, 2024

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Aug 5, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Show resolved Hide resolved

Improve tests

86dbe86

cloud-fan reviewed Aug 8, 2024

View reviewed changes

cloud-fan approved these changes Aug 8, 2024

View reviewed changes

Remove UnresolvedWithCTERelation

5ab6592

cloud-fan reviewed Aug 8, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala Outdated Show resolved Hide resolved

mihailom-db added 2 commits August 9, 2024 22:37

Revert CTE changes

71fbbce

Fix error

3cf26eb

cloud-fan approved these changes Aug 14, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fixParameters

b97116c

# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

MaxGekk reviewed Aug 19, 2024

View reviewed changes

Remove unnecessary comments

3757fe3

MaxGekk approved these changes Aug 19, 2024

View reviewed changes

MaxGekk closed this in 11b682c Aug 19, 2024

mihailom-db mentioned this pull request Aug 20, 2024

[SPARK-49017][SQL][3.5] Insert statement fails when multiple parameters are being used #47814

Closed

cloud-fan mentioned this pull request Nov 7, 2024

Revert "[SPARK-48273][SQL] Fix late rewrite of PlanWithUnresolvedIdentifier #48786

Closed

[SPARK-49017][SQL] Insert statement fails when multiple parameters are being used #47501

[SPARK-49017][SQL] Insert statement fails when multiple parameters are being used #47501

Uh oh!

Conversation

mihailom-db commented Jul 26, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

mihailom-db commented Jul 30, 2024

Uh oh!

Uh oh!

Uh oh!

cloud-fan Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

mihailom-db Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

mihailom-db Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cloud-fan commented Aug 14, 2024

Uh oh!

mihailom-db commented Aug 19, 2024

Uh oh!

mihailom-db commented Aug 19, 2024

Uh oh!

Uh oh!

MaxGekk Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

mihailom-db Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Aug 19, 2024

Uh oh!

MaxGekk commented Aug 19, 2024

Uh oh!

mihailom-db commented Aug 19, 2024

Uh oh!

MaxGekk commented Aug 19, 2024

Uh oh!

mihailom-db commented Aug 20, 2024

Uh oh!

MaxGekk commented Aug 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants