Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
monkeyboy123 wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5cbd73
[SPARK-38333][SQL] DPP cause DataSourceScanExec java.lang.NullPointer…
monkeyboy123 Feb 25, 2022
801ee50
add UT
monkeyboy123 Feb 28, 2022
708a168
fix code style
monkeyboy123 Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...lyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/EquivalentExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ class EquivalentExpressions {
expr.find(_.isInstanceOf[LambdaVariable]).isDefined ||
// `PlanExpression` wraps query plan. To compare query plans of `PlanExpression` on executor,
// can cause error like NPE.
(expr.isInstanceOf[PlanExpression[_]] && TaskContext.get != null)
(expr.find(_.isInstanceOf[PlanExpression[_]]).isDefined && TaskContext.get != null)
Copy link
Contributor

@cloud-fan cloud-fan Feb 28, 2022

Choose a reason for hiding this comment

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

I think we should fix it at the master branch as well, as the code does not match the comment.

We can also add a UT in SubexpressionEliminationSuite, which tests addExprTree directly.

Copy link
Contributor Author

@monkeyboy123 monkeyboy123 Feb 28, 2022

Choose a reason for hiding this comment

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

OK, i will open another pr to fix it at the master branch and add a UT

Copy link
Contributor Author

@monkeyboy123 monkeyboy123 Mar 1, 2022

Choose a reason for hiding this comment

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

unit test added

Copy link
Contributor Author

@monkeyboy123 monkeyboy123 Mar 27, 2022

Choose a reason for hiding this comment

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

@cloud-fan How should this pr title be named? Maybe this is a potential problem.

Copy link
Contributor

@cloud-fan cloud-fan Mar 28, 2022

Choose a reason for hiding this comment

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

Do we have a master branch PR now?

Copy link
Contributor

@LuciferYang LuciferYang Mar 28, 2022

Choose a reason for hiding this comment

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

@monkeyboy123 we can replace expr.find(_.isInstanceOf[PlanExpression[_]]).isDefined with TreeNode.exists api now

Copy link
Contributor Author

@monkeyboy123 monkeyboy123 Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have a master branch PR now?

Sorry for late reply，i will open a pr right now.

Copy link
Contributor Author

@monkeyboy123 monkeyboy123 Mar 30, 2022

Choose a reason for hiding this comment

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

@monkeyboy123 we can replace expr.find(_.isInstanceOf[PlanExpression[_]]).isDefined with TreeNode.exists api now
ok

Copy link
Contributor Author

@monkeyboy123 monkeyboy123 Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have a master branch PR now?

done, new pr SPARK-38333


if (!skip && !addFunc(expr)) {
childrenToRecurse(expr).foreach(addExprTree(_, addFunc))
Expand Down
20 changes: 19 additions & 1 deletion .../test/scala/org/apache/spark/sql/catalyst/expressions/SubexpressionEliminationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
*/
package org.apache.spark.sql.catalyst.expressions

import org.apache.spark.SparkFunSuite
import org.apache.spark.{SparkFunSuite, TaskContext, TaskContextImpl}
import org.apache.spark.sql.catalyst.expressions.codegen._
import org.apache.spark.sql.catalyst.plans.logical.Command
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types.{BinaryType, DataType, IntegerType}

Expand Down Expand Up @@ -255,6 +256,21 @@ class SubexpressionEliminationSuite extends SparkFunSuite with ExpressionEvalHel
assert(equivalence2.getAllEquivalentExprs.count(_.size == 2) == 0)
}

test("SPARK-38333: DPP expression should not be eliminated") {
try {
// support we in executor
val context1 = new TaskContextImpl(0, 0, 0, 0, 0, null, null, null)
TaskContext.setTaskContext(context1)

val equivalence = new EquivalentExpressions
val expression = DynamicPruningExpression(Exists(TestCommand("foo")))
equivalence.addExprTree(expression)
assert(equivalence.getEquivalentExprs(expression).size == 0)
} finally {
TaskContext.unset()
}
}

test("SPARK-34723: Correct parameter type for subexpression elimination under whole-stage") {
withSQLConf(SQLConf.CODEGEN_METHOD_SPLIT_THRESHOLD.key -> "1") {
val str = BoundReference(0, BinaryType, false)
Expand Down Expand Up @@ -315,3 +331,5 @@ case class CodegenFallbackExpression(child: Expression)
extends UnaryExpression with CodegenFallback {
override def dataType: DataType = child.dataType
}

case class TestCommand(foo: String) extends Command