From fe7222873f7a612d1dea6855c3987046f2ff1ec1 Mon Sep 17 00:00:00 2001 From: Fu Chen Date: Wed, 9 Jun 2021 16:02:42 +0800 Subject: [PATCH 1/3] [SPARK-35673][SQL] Fix user-defined hint and unrecognized hint in subquery. --- .../sql/catalyst/plans/logical/hints.scala | 3 ++- .../sql/SparkSessionExtensionSuite.scala | 26 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/hints.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/hints.scala index 56829cfb4c3f0..08029442a2a0c 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/hints.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/hints.scala @@ -30,7 +30,8 @@ import org.apache.spark.sql.catalyst.trees.TreePattern.{TreePattern, UNRESOLVED_ case class UnresolvedHint(name: String, parameters: Seq[Any], child: LogicalPlan) extends UnaryNode { - override lazy val resolved: Boolean = false + override lazy val resolved: Boolean = child.resolved + override def output: Seq[Attribute] = child.output final override val nodePatterns: Seq[TreePattern] = Seq(UNRESOLVED_HINT) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/SparkSessionExtensionSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/SparkSessionExtensionSuite.scala index ff0855310004a..8225204089df0 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/SparkSessionExtensionSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/SparkSessionExtensionSuite.scala @@ -370,6 +370,32 @@ class SparkSessionExtensionSuite extends SparkFunSuite { } } } + + test("SPARK-35673: user-defined hint and unrecognized hint in subquery") { + withSession(Seq(_.injectPostHocResolutionRule(MyHintRule))) { session => + // unrecognized hint + QueryTest.checkAnswer( + session.sql( + """ + |SELECT * + |FROM ( + | SELECT /*+ some_random_hint_that_does_not_exist */ 42 + |) + |""".stripMargin), + Row(42) :: Nil) + + // user-defined hint + QueryTest.checkAnswer( + session.sql( + """ + |SELECT * + |FROM ( + | SELECT /*+ CONVERT_TO_EMPTY */ 42 + |) + |""".stripMargin), + Nil) + } + } } case class MyRule(spark: SparkSession) extends Rule[LogicalPlan] { From b5425432917c7cf3c79f231c3df9f789a9bc9468 Mon Sep 17 00:00:00 2001 From: Fu Chen Date: Thu, 10 Jun 2021 10:51:04 +0800 Subject: [PATCH 2/3] checkAnalysis for UnresolvedHint --- .../sql/catalyst/analysis/CheckAnalysis.scala | 3 +++ .../spark/sql/catalyst/plans/logical/hints.scala | 2 ++ .../catalyst/analysis/AnalysisErrorSuite.scala | 16 ++++++++++++++++ 3 files changed, 21 insertions(+) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala index bab6f9c239cc7..fc6e797065279 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala @@ -122,6 +122,9 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog { case u: UnresolvedRelation => u.failAnalysis(s"Table or view not found: ${u.multipartIdentifier.quoted}") + case u: UnresolvedHint => + u.failAnalysis(s"Hint not found: ${u.name}") + case InsertIntoStatement(u: UnresolvedRelation, _, _, _, _, _) => u.failAnalysis(s"Table not found: ${u.multipartIdentifier.quoted}") diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/hints.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/hints.scala index 08029442a2a0c..74cc23d6e5730 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/hints.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/hints.scala @@ -30,6 +30,8 @@ import org.apache.spark.sql.catalyst.trees.TreePattern.{TreePattern, UNRESOLVED_ case class UnresolvedHint(name: String, parameters: Seq[Any], child: LogicalPlan) extends UnaryNode { + // we need it to be resolved so that the analyzer can continue to analyze the rest of the query + // plan. override lazy val resolved: Boolean = child.resolved override def output: Seq[Attribute] = child.output diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala index 8ea84a484d570..71c82900234b9 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala @@ -791,4 +791,20 @@ class AnalysisErrorSuite extends AnalysisTest { assertAnalysisError(plan, s"Correlated column is not allowed in predicate ($msg)" :: Nil) } } + + test("SPARK-35673: fail if the plan still contains UnresolvedHint after analysis") { + val hintName = "some_random_hint_that_does_not_exist" + val plan = UnresolvedHint(hintName, Seq.empty, + Project(Alias(Literal(1), "x")() :: Nil, OneRowRelation()) + ) + assert(plan.resolved) + + val error = intercept[AnalysisException] { + SimpleAnalyzer.checkAnalysis(plan) + } + assert(error.message.contains("Hint not found: some_random_hint_that_does_not_exist")) + + // UnresolvedHint be removed by batch `Remove Unresolved Hints` + assertAnalysisSuccess(plan, true) + } } From 94d22b2c519f3f15af853a249218ed261640136e Mon Sep 17 00:00:00 2001 From: Fu Chen Date: Thu, 10 Jun 2021 10:54:39 +0800 Subject: [PATCH 3/3] typo --- .../apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala index 71c82900234b9..efe4177bb3162 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala @@ -802,7 +802,7 @@ class AnalysisErrorSuite extends AnalysisTest { val error = intercept[AnalysisException] { SimpleAnalyzer.checkAnalysis(plan) } - assert(error.message.contains("Hint not found: some_random_hint_that_does_not_exist")) + assert(error.message.contains(s"Hint not found: ${hintName}")) // UnresolvedHint be removed by batch `Remove Unresolved Hints` assertAnalysisSuccess(plan, true)