[GitHub] [spark] maropu commented on a change in pull request #28683: [SPARK-31875][SQL] Provide a option to disable user supplied Hints
maropu commented on a change in pull request #28683:
URL: https://github.com/apache/spark/pull/28683#discussion_r450558846
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##
@@ -2106,6 +2106,15 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val OPTIMIZER_IGNORE_HINTS =
Review comment:
Both names (`spark.sql.optimizer.ignoreHints` and
`spark.sql.ignorePlanHints`) is okay to me.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28683: [SPARK-31875][SQL] Provide a option to disable user supplied Hints
maropu commented on a change in pull request #28683:
URL: https://github.com/apache/spark/pull/28683#discussion_r450530315
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##
@@ -2106,6 +2106,15 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val OPTIMIZER_IGNORE_HINTS =
Review comment:
How about `spark.sql.ignorePlanHints`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28683: [SPARK-31875][SQL] Provide a option to disable user supplied Hints
maropu commented on a change in pull request #28683:
URL: https://github.com/apache/spark/pull/28683#discussion_r433036289
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##
@@ -2065,6 +2065,16 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val OPTIMIZER_HINTS_ENABLED =
+buildConf("spark.sql.optimizer.hints.enabled")
+ .internal()
+ .doc("Hints are additional directives that aids optimizer in better
planning of a query. " +
+"This configuration when set to `false`, disables the application of
user " +
+"specified hints.")
Review comment:
nit: How about rephrasing it like this? (It seems the ohter similar
`.enabled` configs say `When true, brabrabra`)
```
When false, the optimizer will ignore user-specified hints that are
additional directives
for better planning of a query.
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28683: [SPARK-31875][SQL] Provide a option to disable user supplied Hints
maropu commented on a change in pull request #28683:
URL: https://github.com/apache/spark/pull/28683#discussion_r433036289
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##
@@ -2065,6 +2065,16 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val OPTIMIZER_HINTS_ENABLED =
+buildConf("spark.sql.optimizer.hints.enabled")
+ .internal()
+ .doc("Hints are additional directives that aids optimizer in better
planning of a query. " +
+"This configuration when set to `false`, disables the application of
user " +
+"specified hints.")
Review comment:
nit: How about rephrasing it like this?
```
When false, the optimizer will ignore user-specified hints that are
additional directives
for better planning of a query.
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28683: [SPARK-31875][SQL] Provide a option to disable user supplied Hints
maropu commented on a change in pull request #28683:
URL: https://github.com/apache/spark/pull/28683#discussion_r432931780
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveHintsSuite.scala
##
@@ -294,4 +294,50 @@ class ResolveHintsSuite extends AnalysisTest {
caseSensitive = true)
}
}
+
+ test("disable hint resolution when spark.sql.optimizer.hints.enabled =
false") {
+checkAnalysis(
+ UnresolvedHint("COALESCE", Seq(Literal(10)), table("TaBlE")),
+ testRelation,
+ caseSensitive = true,
+ enableHints = false
Review comment:
Ah, yea. that sounds better.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28683: [SPARK-31875][SQL] Provide a option to disable user supplied Hints
maropu commented on a change in pull request #28683:
URL: https://github.com/apache/spark/pull/28683#discussion_r432930260
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveHintsSuite.scala
##
@@ -294,4 +294,50 @@ class ResolveHintsSuite extends AnalysisTest {
caseSensitive = true)
}
}
+
+ test("disable hint resolution when spark.sql.optimizer.hints.enabled =
false") {
+checkAnalysis(
+ UnresolvedHint("COALESCE", Seq(Literal(10)), table("TaBlE")),
+ testRelation,
+ caseSensitive = true,
+ enableHints = false
Review comment:
Could you use `withSQLConf` instead for the tests? I think `enableHints`
is only used for this test suite, so we don't need to add the option in
`AnalysisTest`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28683: [SPARK-31875][SQL] Provide a option to disable user supplied Hints
maropu commented on a change in pull request #28683:
URL: https://github.com/apache/spark/pull/28683#discussion_r432929981
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveHints.scala
##
@@ -144,7 +144,9 @@ object ResolveHints {
}
def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsUp {
- case h: UnresolvedHint if
STRATEGY_HINT_NAMES.contains(h.name.toUpperCase(Locale.ROOT)) =>
+ case h: UnresolvedHint
+if (conf.optimizerHintsEnabled &&
+ STRATEGY_HINT_NAMES.contains(h.name.toUpperCase(Locale.ROOT))) =>
Review comment:
nit format;
```
case h: UnresolvedHint if conf.optimizerHintsEnabled &&
STRATEGY_HINT_NAMES.contains(h.name.toUpperCase(Locale.ROOT)) =>
if (h.parameters.isEmpty) {
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28683: [SPARK-31875][SQL] Provide a option to disable user supplied Hints
maropu commented on a change in pull request #28683:
URL: https://github.com/apache/spark/pull/28683#discussion_r432929929
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveHints.scala
##
@@ -248,7 +250,8 @@ object ResolveHints {
}
def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperators {
- case hint @ UnresolvedHint(hintName, _, _) =>
hintName.toUpperCase(Locale.ROOT) match {
+ case hint @ UnresolvedHint(hintName, _, _)
+if conf.optimizerHintsEnabled => hintName.toUpperCase(Locale.ROOT)
match {
Review comment:
nit format;
```
case hint @ UnresolvedHint(hintName, _, _) if
conf.optimizerHintsEnabled =>
hintName.toUpperCase(Locale.ROOT) match {
case "REPARTITION" =>
createRepartition(shuffle = true, hint)
...
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28683: [SPARK-31875][SQL] Provide a option to disable user supplied Hints
maropu commented on a change in pull request #28683:
URL: https://github.com/apache/spark/pull/28683#discussion_r432929771
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##
@@ -2065,6 +2065,16 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val OPTIMIZER_HINTS_ENABLED =
+buildConf("spark.sql.optimizer.hints.enabled")
+ .internal()
+ .doc("Hints are additional directives that aids optimizer in better
planning of a query." +
+" This configuration when set to `false`, disables the application of
user" +
Review comment:
nit: for consistency, could you move the space in the head into the tail?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
