Skip to content

[SPARK-31875][SQL] Provide a option to disable user supplied Hints #28683

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
dilipbiswal wants to merge 8 commits into from
Closed

[SPARK-31875][SQL] Provide a option to disable user supplied Hints #28683

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b35e677
[SPARK-31875] Provide a option to disable user supplied Hints
dilipbiswal May 31, 2020
6fa79a3
Code review
dilipbiswal Jun 1, 2020
215f31c
Code review
dilipbiswal Jun 20, 2020
827776c
Code review
dilipbiswal Jul 2, 2020
9af1413
Code review
dilipbiswal Jul 3, 2020
f718567
Review
dilipbiswal Jul 7, 2020
f3d030f
Review
dilipbiswal Jul 7, 2020
dbdab2d
Review
dilipbiswal Jul 8, 2020
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: 2 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@ class Analyzer(
val postHocResolutionRules: Seq[Rule[LogicalPlan]] = Nil

lazy val batches: Seq[Batch] = Seq(
Batch("Disable Hints", Once,
new ResolveHints.DisableHints(conf)),
Batch("Hints", fixedPoint,
new ResolveHints.ResolveJoinStrategyHints(conf),
new ResolveHints.ResolveCoalesceHints(conf)),
Expand Down
11 changes: 11 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveHints.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,4 +278,15 @@ object ResolveHints {
h.child
}
}

/**
* Removes all the hints when `spark.sql.ignorePlanHints` is set.
* This is executed at the very beginning of the Analyzer to disable
* the hint functionality.
*/
class DisableHints(conf: SQLConf) extends RemoveAllHints(conf: SQLConf) {
override def apply(plan: LogicalPlan): LogicalPlan = {
if (conf.getConf(SQLConf.IGNORE_HINTS)) super.apply(plan) else plan

@cloud-fan cloud-fan Jul 7, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ignore or disable? we should be consistent

@dilipbiswal dilipbiswal Jul 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

+1. You are right. I will change the config to spark.sql.optimizer.disableHints to make it consistent. But could you please help answer @dongjoon-hyun's question on if we need a ".enabled" at the end ?

@dongjoon-hyun dongjoon-hyun Jul 7, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, @dilipbiswal . It seems that he also meantSQLConf.IGNORE_HINTS should be SQLConf.DISABLE_HINTS.

}
}
}
9 changes: 9 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2106,6 +2106,15 @@ object SQLConf {
.booleanConf
.createWithDefault(true)

val IGNORE_HINTS =
Comment thread
dongjoon-hyun marked this conversation as resolved.
Outdated
buildConf("spark.sql.ignorePlanHints")
.internal()
.doc("When true, the optimizer will ignore user-specified hints that are additional " +
Comment thread
dongjoon-hyun marked this conversation as resolved.
Outdated
"directives for better planning of a query.")
.version("3.1.0")
.booleanConf
.createWithDefault(false)

val NESTED_PREDICATE_PUSHDOWN_FILE_SOURCE_LIST =
buildConf("spark.sql.optimizer.nestedPredicatePushdown.supportedFileSources")
.internal()
Expand Down
39 changes: 39 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3521,6 +3521,45 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
|""".stripMargin), Row(1))
}
}

test("SPARK-31875: remove hints from plan when spark.sql.ignorePlanHints = true") {
withSQLConf(SQLConf.IGNORE_HINTS.key -> "true") {
withTempView("t1", "t2") {
Seq[Integer](1, 2).toDF("c1").createOrReplaceTempView("t1")
Seq[Integer](1, 2).toDF("c1").createOrReplaceTempView("t2")
val repartitionHints = Seq(
"COALESCE(2)",
"REPARTITION(c1)",
"REPARTITION(c1, 2)",
"REPARTITION_BY_RANGE(c1, 2)",
"REPARTITION_BY_RANGE(c1)"
)
val joinHints = Seq(
"BROADCASTJOIN (t1)",
"MAPJOIN(t1)",
"SHUFFLE_MERGE(t1)",
"MERGEJOIN(t1)",
"SHUFFLE_REPLICATE_NL(t1)"
)

repartitionHints.foreach { hintName =>
val sqlText = s"SELECT /*+ $hintName */ * FROM t1"
val sqlTextWithoutHint = "SELECT * FROM t1"
val expectedPlan = sql(sqlTextWithoutHint)
val actualPlan = sql(sqlText)
comparePlans(actualPlan.queryExecution.analyzed, expectedPlan.queryExecution.analyzed)
}

joinHints.foreach { hintName =>
val sqlText = s"SELECT /*+ $hintName */ * FROM t1 INNER JOIN t2 ON t1.c1 = t2.c1"
val sqlTextWithoutHint = "SELECT * FROM t1 INNER JOIN t2 ON t1.c1 = t2.c1"
val expectedPlan = sql(sqlTextWithoutHint)
val actualPlan = sql(sqlText)
comparePlans(actualPlan.queryExecution.analyzed, expectedPlan.queryExecution.analyzed)
}
}
}
}
}

case class Foo(bar: Option[String])