[SPARK-35742][SQL] Expression.semanticEquals should be symmetrical #32885

cloud-fan · 2021-06-11T17:01:00Z

What changes were proposed in this pull request?

Currently, there are some expressions that overwrite semanticEquals, which makes it not symmetrical. Ideally, expressions should overwrite canonicalized instead of semanticEquals.

This PR marks semanticEquals as final, and implement canonicalized for the few expressions that overwrote semanticEquals before.

Why are the changes needed?

To avoid subtle bugs (I haven't found a real bug yet).

Does this PR introduce any user-facing change?

no

How was this patch tested?

a new test

cloud-fan · 2021-06-11T17:02:18Z

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

all the subclasses of SubqueryExpression have implemented canonicalized.

cloud-fan · 2021-06-11T17:02:50Z

cc @viirya @maropu

viirya · 2021-06-11T17:22:41Z

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

Don't need canonicalized?

The default implementation works for AttributeReference

viirya · 2021-06-11T17:25:34Z

sql/core/src/main/scala/org/apache/spark/sql/execution/subquery.scala

It's already there, in L164

dongjoon-hyun · 2021-06-11T19:05:30Z

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

Yes, I agree with the idea.

dongjoon-hyun · 2021-06-11T19:09:55Z

BTW, could you make CI happy?

viirya

Looks good if CI can pass.

SparkQA · 2021-06-11T21:49:32Z

Test build #139711 has finished for PR 32885 at commit 73aa6f2.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-06-11T23:54:40Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44234/

HyukjinKwon · 2021-06-13T00:41:23Z

Yeah, +1 if CI passes.

cloud-fan · 2021-06-14T14:51:12Z

sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala

  // map and/or the sort-based aggregation once it has processed a given number of input rows.
  private val testFallbackStartsAt: Option[(Int, Int)] = {
-    sqlContext.getConf("spark.sql.TungstenAggregate.testFallbackStartsAt", null) match {
+    Option(sqlContext).map { sc =>


This is a hidden bug. SubqueryExpression will be sent to the executor side and build Projection, and be put in EquivalentExpressions, which needs to call canonicalized.

This means, Spark may serialize and send HashAggregateExec to the executor side, where sqlContext should be null.

It's hidden for a long time because ScalarSubquery didn't implement canonicalized, so the bug is not triggered. However, it also means semanticHash is wrong.

I think it only affects common subquery elimination, and shouldn't be a serious bug.

In the long term, I think we should only send an "expression evaluator" to the executor side. The semantic check should only be done in the driver side.

Sorry for the late comment @cloud-fan, but I think I've run into this issue before:
https://github.com/apache/spark/pull/28885/files#diff-9b62cef6bfdeb6c802bb120c7a724a974d5067a69585285bebb64c48603f8d6fR105-R108. The point is that there might be other nodes where canonicalization on executor side can cause issues. SortExec.enableRadixSort is the other one I found.

InSubqueryExec already implements canonicalized before this PR, so we need to fix these bugs anyway.

I have an idea to fix this problem in all physical plans:

remove SparkPlan.sqlContext, so that we can catch all the callers of it

add @transient final val session = SparkSession.getActiveSession.orNull, to replace the previous sqlContext

override conf in SparkPlan: if (session != null) session.sessionState.conf else SQLConf.get

@peter-toth what do you think? AFAIK the only reason to access SparkPlan.sqlContext at executor side is to read a conf, and we can do that with SQLConf.get at executor side.

Sounds good to me.

I'm quite busy this week and may not have time to implement this idea recently. @peter-toth feel free to pick up this idea and open a PR if you have time, or I'll do it next or next next week. Thanks in advance!

Ok, thanks. I will try to open a PR this week.

Opened #32947.

cloud-fan · 2021-06-15T02:50:36Z

sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q23b/simplified.txt

-                                          HashAggregate [c_customer_sk,sum,isEmpty] [sum(CheckOverflow((promote_precision(cast(cast(ss_quantity as decimal(10,0)) as decimal(12,2))) * promote_precision(cast(ss_sales_price as decimal(12,2)))), DecimalType(18,2), true)),sum(CheckOverflow((promote_precision(cast(cast(ss_quantity as decimal(10,0)) as decimal(12,2))) * promote_precision(cast(ss_sales_price as decimal(12,2)))), DecimalType(18,2), true)),sum,isEmpty]
-                                            InputAdapter
-                                              ReusedExchange [c_customer_sk,sum,isEmpty] #8
+                          ReusedExchange [c_customer_sk,c_first_name,c_last_name] #19


After the fix, more exchange reuse happens :)

Seems the problem was caused by ReusedSubquery.

SparkQA · 2021-06-15T03:43:25Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44315/

SparkQA · 2021-06-15T07:29:25Z

Test build #139789 has finished for PR 32885 at commit 5095fff.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2021-06-15T07:58:49Z

LGTM

cloud-fan · 2021-06-15T08:53:01Z

thanks for the review, merging to master!

Currently, there are some expressions that overwrite `semanticEquals`, which makes it not symmetrical. Ideally, expressions should overwrite `canonicalized` instead of `semanticEquals`. This PR marks `semanticEquals` as final, and implement `canonicalized` for the few expressions that overwrote `semanticEquals` before. To avoid subtle bugs (I haven't found a real bug yet). no a new test Closes apache#32885 from cloud-fan/attr. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan commented Jun 11, 2021

View reviewed changes

github-actions bot added the SQL label Jun 11, 2021

viirya reviewed Jun 11, 2021

View reviewed changes

cloud-fan mentioned this pull request Jun 11, 2021

[SPARK-35712][SQL] Simplify ResolveAggregateFunctions #32470

Closed

dongjoon-hyun reviewed Jun 11, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala Outdated

Copy link

Member

dongjoon-hyun Jun 11, 2021

Choose a reason for hiding this comment

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

Yes, I agree with the idea.

viirya reviewed Jun 11, 2021

View reviewed changes

cloud-fan added 2 commits June 14, 2021 22:17

Expression.semanticEquals should be symmetrical

5ab3deb

fix a bug

9364c42

cloud-fan force-pushed the attr branch from 73aa6f2 to 9364c42 Compare June 14, 2021 14:42

cloud-fan commented Jun 14, 2021

View reviewed changes

update plan golden files

5095fff

cloud-fan commented Jun 15, 2021

View reviewed changes

viirya approved these changes Jun 15, 2021

View reviewed changes

cloud-fan closed this in a50bd8f Jun 15, 2021

peter-toth mentioned this pull request Jun 17, 2021

[SPARK-29375][SPARK-28940][SPARK-32041][SQL] Whole plan exchange and subquery reuse #28885

Closed

monkeyboy123 mentioned this pull request Feb 26, 2022

[SPARK-38333][SQL] [3.1]DPP cause DataSourceScanExec java.lang.NullPointer… #35662

Closed

[SPARK-35742][SQL] Expression.semanticEquals should be symmetrical #32885

[SPARK-35742][SQL] Expression.semanticEquals should be symmetrical #32885

Uh oh!

Conversation

cloud-fan commented Jun 11, 2021

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jun 11, 2021

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 11, 2021

Uh oh!

SparkQA commented Jun 11, 2021

Uh oh!

HyukjinKwon commented Jun 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter-toth Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 15, 2021

Uh oh!

SparkQA commented Jun 15, 2021

Uh oh!

viirya commented Jun 15, 2021

Uh oh!

cloud-fan commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

peter-toth Jun 15, 2021 •

edited

Loading