[SPARK-46366][SQL] Use WITH expression in BETWEEN to avoid duplicate expressions #44299

dbatomic · 2023-12-11T16:37:50Z

What changes were proposed in this pull request?

Prior to this change e BETWEEN lower AND upper expression used to be transformed into lower <= e && e <= upper. This means that e would be evaluated twice which is problematic from both correctness and performance perspectives.

Suggested fix is to use WITH expression that was introduced with this change.

Why are the changes needed?

Current implementation is not correct for non deterministic expressions, since two calls might return different results.

Does this PR introduce any user-facing change?

With this change generated plan for BETWEEN statement will be different. An example of generated plan is provided in tests.

How was this patch tested?

Existing tests plus new test in PlanGenerationTestSuite.

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

Yes.

…n_v2

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/betweenExpression.scala

dtenedor · 2023-12-14T19:47:10Z

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

+  /**
+   * Transform UnresolvedBetweenExpression into a [BetweenExpr].
+   */
+  object ResolveBetweenExpression extends Rule[LogicalPlan] {


We should avoid trying to add new analyzer rules at this point. Could we accomplish this by combining UnresolvedBetweenExpression and ResolvedBetweeExpression into just one BetweenExpression and make that inherit from RuntimeReplaceable? Then its replacement can be the With expression of interest.

This is what I tried initially. I tried to explain the problem in PR description, but I guess that I wasn't clear enough. Let me try to do this again:
Initially I tried with BetweenExpr extends RuntimeReplaceable. The problem that I hit was that CommonExpressionRef required dataType of CommonExpressionDef to be known for it to be created:

def this(exprDef: CommonExpressionDef) = this(exprDef.id, exprDef.dataType, exprDef.nullable)

To get around this I would like to create replacement of BetweenExpr after I resolve it's Child expressions and hence after I have it's dataType. I hit a wall doing this so I did this thing with UnresolvedBetween->BetweenExpr (for which I will know the types)->With, which works, but I agree that it would be better if we could avoid this + get rid of extra rule.

I will give it another try. If you guys know a way to get around this I would appreciate some help :)

Ok, I hope that i got it right in the latest iteration. Please take a look.

Btw, thanks for the comment, this is definitely much cleaner.

…/else combination. Even with this rule there are changes in DS suite. We should work to eliminate these.

dtenedor

LGTM!

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

…m operator list since it is now in function list.

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

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Between.scala

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/PlanGenerationTestSuite.scala

…o between_expression_v2 merge

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Between.scala

cloud-fan · 2023-12-27T11:52:53Z

let's not forget about this comment :) https://github.com/apache/spark/pull/44299/files#r1436957948

cloud-fan · 2023-12-28T07:41:37Z

thanks, merging to master!

### What changes were proposed in this pull request? Fix for between with ScalarSubqueries. ### Why are the changes needed? There is a regression introduced from a previous PR #44299. This needs to be addressed as between operator was completely broken with resolved ScalarSubqueries. ### Does this PR introduce _any_ user-facing change? No, the bug is not release yet. ### How was this patch tested? Tests added to golden file. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #47581 from mihailom-db/fixbetween. Authored-by: Mihailo Milosevic <mihailo.milosevic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ql-api-docs.py` to avoid duplication ### What changes were proposed in this pull request? The pr aims to delete `ExpressionInfo[between]` from `gen-sql-api-docs.py` to avoid duplication. ### Why are the changes needed? - In the following doc, `between` is repeatedly displayed `twice` https://spark.apache.org/docs/preview/api/sql/index.html#between <img width="1062" alt="image" src="https://github.com/user-attachments/assets/1aa2ad22-6346-40d7-be1d-cab73b79959a"> After the pr: <img width="751" alt="image" src="https://github.com/user-attachments/assets/a66d607a-9dcb-4d96-a8f9-024f3844055b"> - After #44299, the expression 'between' has been added to `Spark 4.0`. ### Does this PR introduce _any_ user-facing change? Yes, only for docs. ### How was this patch tested? Manually check. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #48183 from panbingkun/SPARK-49733. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

dbatomic added 5 commits December 11, 2023 14:57

intial prototype

c018e6a

Adding test to plan generation test suite

8c17973

Adding explain check

8c929c2

adding new comment

e5da5c5

Merge remote-tracking branch 'upstream/master' into between_expressio…

38f3ea6

…n_v2

github-actions bot added SQL CONNECT labels Dec 11, 2023

dbatomic and others added 4 commits December 12, 2023 15:01

Merge branch 'apache:master' into between_expression_v2

57d7994

fixing test in expressionparsersuite

feceae3

Fixing streaming logic

765e499

Updating golden files

1b25d4f

MaxGekk reviewed Dec 14, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/betweenExpression.scala Outdated Show resolved Hide resolved

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/betweenExpression.scala Outdated Show resolved Hide resolved

dtenedor reviewed Dec 14, 2023

View reviewed changes

dbatomic added 8 commits December 18, 2023 12:20

Removing unresolved between expression.

a69dd57

Renaming BetweenExpr -> Between

a038faa

Fixing golden files

d0462ff

Adding comments.

596befe

Adding more rigid coercion rules for between, there are more alike if…

1a2ec50

…/else combination. Even with this rule there are changes in DS suite. We should work to eliminate these.

Using built in function for resolution.

0faa792

Updating changed plans

7f6b0c9

Updating golden files

a1dc501

dtenedor approved these changes Dec 19, 2023

View reviewed changes

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

dbatomic and others added 7 commits December 19, 2023 22:45

Updating golden files for sql-expression-schema. Removing between fro…

6569653

…m operator list since it is now in function list.

Doc update for between expression

e64fc19

Updating expression documentation

57c88ee

Merge branch 'apache:master' into between_expression_v2

1776f8a

fixing failing doc test

26653f0

Documentation update

dc36be3

Merge branch 'apache:master' into between_expression_v2

ad61074

cloud-fan reviewed Dec 22, 2023

View reviewed changes

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

cloud-fan reviewed Dec 22, 2023

View reviewed changes

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

cloud-fan reviewed Dec 22, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Between.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 22, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Between.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 22, 2023

View reviewed changes

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/PlanGenerationTestSuite.scala Show resolved Hide resolved

dbatomic added 2 commits December 26, 2023 16:47

Resolving PR comments

3cf76b1

Merge branch 'between_expression_v2' of github.com:dbatomic/spark int…

550fa3d

…o between_expression_v2 merge

cloud-fan reviewed Dec 27, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Between.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 27, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Between.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 27, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Between.scala Outdated Show resolved Hide resolved

PR comments

7dfd72d

cloud-fan reviewed Dec 27, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Between.scala Outdated Show resolved Hide resolved

dbatomic added 3 commits December 27, 2023 13:26

PR comment on better documentation

4fb6631

doc update

a960c91

adding rand example to predicate function golden files.

1ce26eb

cloud-fan closed this in 93a2526 Dec 28, 2023

mihailomilosevic2001 mentioned this pull request Aug 5, 2024

[SPARK-49063][SQL] Fix Between with ScalarSubqueries #47581

Closed

panbingkun mentioned this pull request Sep 20, 2024

[SPARK-49733][SQL][DOCS] Delete ExpressionInfo[between] from gen-sql-api-docs.py to avoid duplication #48183

Closed

[SPARK-46366][SQL] Use WITH expression in BETWEEN to avoid duplicate expressions #44299

[SPARK-46366][SQL] Use WITH expression in BETWEEN to avoid duplicate expressions #44299

Uh oh!

Conversation

dbatomic commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Uh oh!

dtenedor Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

dbatomic Dec 15, 2023

Choose a reason for hiding this comment

Uh oh!

dbatomic Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

dtenedor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan commented Dec 27, 2023

Uh oh!

cloud-fan commented Dec 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dbatomic commented Dec 11, 2023 •

edited

Loading