[SPARK-28292][SQL] Enable inject user-defined Hint #25071

AngersZhuuuu · 2019-07-08T06:47:30Z

What changes were proposed in this pull request?

Current catalyst construct, we can't add our user-defined hint to Analyzer. Since Catalyst will resolve hint first when analyze LogicalPlan, after it resolve it' s own hints, it will call RemoveAllHint, so when we add Rule of hint to SparkSessionExtension, it will be ignored.
This PR is a small extension to SparkSessionExtension. It enable user to add user-defined hint to Analyze.

FAQ:
Q: Why we need this extension?
A: Such as DataBricks, in their delta platform, the add more hint for better join behavior such as RANK_JOIN, and SKEW_JOIN, through these hints and their parameter, Catalyst will choose a better behavior to solve un-healthy join operator. And we also can add some other hints to restrict user's behavior such as we only can submit query like SELECT * FROM TABLE with a spec hint flag.
With this extension, you will have more freedom to customize your catalyst.
And will a hint extension, you can combine you code with SparkSessionExtension. More safety.

Q: How to write your hints.
A: Blow is a simple example, it's function is to use hint to pass a LIMIT behavior to a subquery:

object MyHint extends Rule[LogicalPlan] {
 // you hint name, a Set of string, for same Hint
  private val MY_HINT_NAME = Set("MY_HINT")

  def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperators {
  // match when there is a hint in you SQL
    case h: UnresolvedHint if MY_HINT_NAME.contains(h.name.toUpperCase(Locale.ROOT)) =>
     // resolve hint parameters
    val limits = h.parameters match {
        case Seq(IntegerLiteral(limits)) =>
          limits
        case Seq(limits: Int) =>
          limits
        case _ =>
          throw new AnalysisException(s"$MY_HINT_NAME Hint expects a limits number as parameter")
      }
      // The result LogicalPlan with you hint , in there, it just add a Limit to current subquery
      GlobalLimit(Literal(limits), LocalLimit(Literal(limits), h.child))
  }
}

SQL =>
SELECT /* MY_HINT(10) */ * FROM TEST_TABLE

it will return same behavior as
SELECT * FROM TEST_TABLE LIMIT 10

How was this patch tested?

Added unit test.

dongjoon-hyun · 2019-07-10T03:45:31Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala

+    * Inject an analyzer resolution `Hint` builder into the [[SparkSession]]. These analyzer
+    * rules will be executed as part of the resolution phase of analysis.
+    */
+  def injectResolutionHint(builder: RuleBuilder): Unit = {


Did you try to use def injectResolutionRule(builder: RuleBuilder)? Is it insufficient for your use-case?

@dongjoon-hyun
Since for Hint , it will call ResolveHints.RemoveAllHintsafter it handle all Spark's hint ,.
So add Hint rule in
def injectResolutionRule(builder: RuleBuilder)
won't work since all hint has been cleared, That's why I add a new method for hint.

dongjoon-hyun · 2019-07-10T03:47:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

-      new ResolveHints.RemoveAllHints(conf)),
+      new ResolveHints.ResolveJoinStrategyHints(conf) +:
+        ResolveHints.ResolveCoalesceHints +:
+        extendedResolutionHints :+


Oh, I see.

@gatorsmile and @cloud-fan . How do you think about this extension?

cc: @maryannxue , too. (I think this kind of extensions is basically bug-prone and we always need a better design...)

@maropu @maryannxue Any advise to improve this feature.

dongjoon-hyun · 2019-07-10T03:48:04Z

Thank you for your contribution, @AngersZhuuuu . Please add a test case .

dongjoon-hyun · 2019-07-10T03:48:12Z

ok to test

SparkQA · 2019-07-10T03:52:55Z

Test build #107431 has finished for PR 25071 at commit e3db168.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

AngersZhuuuu · 2019-07-10T03:57:26Z

Thank you for your contribution, @AngersZhuuuu . Please add a test case .

I am finding place to add test case. ==
By the way , this pr has been used well in our prod

SparkQA · 2019-07-10T07:05:02Z

Test build #107432 has finished for PR 25071 at commit 23a8bcc.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-10T14:40:31Z

Test build #107462 has finished for PR 25071 at commit 4ea911a.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-10T14:56:53Z

Test build #107464 has finished for PR 25071 at commit e283ad9.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-07-10T18:20:47Z

@AngersZhuuuu . The regression test is used to protect your patch. Without that, your contribution can be disabled accidentally in the future. :)

By the way , this pr has been used well in our prod

sql/core/src/test/scala/org/apache/spark/sql/SparkSessionExtensionSuite.scala

AngersZhuuuu · 2019-07-11T01:27:45Z

@AngersZhuuuu . The regression test is used to protect your patch. Without that, your contribution can be disabled accidentally in the future. :)

By the way , this pr has been used well in our prod

I know it,。
I'm not familiar with the unit tests about spark, so sometimes it takes time to find the right place and way to write unit tests。I am learning how to write good unit tests for these。
Thank you very much for your guidance

SparkQA · 2019-07-11T05:20:39Z

Test build #107496 has finished for PR 25071 at commit 8a60a22.

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

AngersZhuuuu · 2019-07-13T08:20:18Z

@AngersZhuuuu . The regression test is used to protect your patch. Without that, your contribution can be disabled accidentally in the future. :)

By the way , this pr has been used well in our prod

Unit test added and passed all test.

dongjoon-hyun · 2019-07-16T04:29:19Z

Retest this please.

maropu · 2019-07-16T05:29:19Z

Hi, @AngersZhuuuu, could you show us use cases for this extension and add them in the PR description? (I think this is one of FAQ for these kinds of similar extensions)

AngersZhuuuu · 2019-07-16T05:52:34Z

Hi, @AngersZhuuuu, could you show us use cases for this extension and add them in the PR description? (I think this is one of FAQ for these kinds of similar extensions)

I will do this later after work.

SparkQA · 2019-07-16T07:05:01Z

Test build #107717 has finished for PR 25071 at commit 8a60a22.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2019-07-16T13:30:20Z

retest this please

AngersZhuuuu · 2019-07-16T14:42:41Z

Hi, @AngersZhuuuu, could you show us use cases for this extension and add them in the PR description? (I think this is one of FAQ for these kinds of similar extensions)

Update description, please review it and what else can we add.

SparkQA · 2019-07-16T16:50:42Z

Test build #107740 has finished for PR 25071 at commit 8a60a22.

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

SparkQA · 2019-09-10T15:08:59Z

Test build #110419 has finished for PR 25071 at commit 5ca207c.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

maryannxue · 2019-09-10T15:20:27Z

Sorry to chime in late. First of all, I don't think it's a safe or meaningful change at this point. After join hint refactoring, we now have the assumption that there is no ResolvedHint node in the optimizer, all hints will either become a specific real operator (like repartitioning) or be associated with a Join operator. This means your custom hint-handling rule can only deal with the information at UnresolvedHint level, and I don't see a strong reason for doing that.

AngersZhuuuu · 2019-09-10T15:34:36Z

Sorry to chime in late. First of all, I don't think it's a safe or meaningful change at this point. After join hint refactoring, we now have the assumption that there is no ResolvedHint node in the optimizer, all hints will either become a specific real operator (like repartitioning) or be associated with a Join operator. This means your custom hint-handling rule can only deal with the information at UnresolvedHint level, and I don't see a strong reason for doing that.

In our env, we use this feature to combine some hint defined by ourself. Different hint combined for different spark env for different goal.

May be it have some threshold to use Hint, but inject unsafe rule or strategies also cause problems.

maryannxue · 2019-09-10T15:52:40Z

May be it have some threshold to use Hint, but inject unsafe rule or strategies also cause problems.

Not sure if I understand you. But if you are saying that Spark already has some sort of unsafe extension interface so it shouldn't matter to add more, I would strongly disagree.

AngersZhuuuu · 2019-09-10T16:10:08Z

May be it have some threshold to use Hint, but inject unsafe rule or strategies also cause problems.

Not sure if I understand you. But if you are saying that Spark already has some sort of unsafe extension interface so it shouldn't matter to add more, I would strongly disagree.

I think safety is not the point we need to concern, but demand. For our env, we need this to organize different hints。

maryannxue · 2019-09-10T16:22:42Z

I think safety is not the point we need to concern, but demand.

If that were true, there would be no point of having interfaces at all, nor should we have this conversation here.

I suggest you either come up with a more complete solution to this demand of yours, or have your own fork.

gatorsmile · 2019-09-10T17:04:17Z

Adding new extensions is not needed. See the WIP PR: #25746

SparkQA · 2019-09-10T19:12:07Z

Test build #110423 has finished for PR 25071 at commit af759fa.

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

AmplabJenkins · 2019-09-16T18:09:56Z

Can one of the admins verify this patch?

HyukjinKwon · 2019-09-16T23:44:34Z

Closing this in favour of #25746

AngersZhuuuu · 2021-06-09T03:56:03Z

ping @HyukjinKwon @dongido001 @cloud-fan @gatorsmile Since https://issues.apache.org/jira/browse/SPARK-35673

[SPARK-28292] Enable inject user-defined Hint

e3db168

AngersZhuuuu changed the title ~~[SPARK-28292] Enable inject user-defined Hint~~ [WIP][SPARK-28292] Enable inject user-defined Hint Jul 8, 2019

AngersZhuuuu changed the title ~~[WIP][SPARK-28292] Enable inject user-defined Hint~~ [WIP][SPARK-28292][SQL] Enable inject user-defined Hint Jul 8, 2019

dongjoon-hyun added the SQL label Jul 8, 2019

AngersZhuuuu changed the title ~~[WIP][SPARK-28292][SQL] Enable inject user-defined Hint~~ [SPARK-28292][SQL] Enable inject user-defined Hint Jul 8, 2019

dongjoon-hyun reviewed Jul 10, 2019

View reviewed changes

Use Javadoc style indentation for multiline comments

23a8bcc

[SPARK-28292] Add unit test

4ea911a

Fix scala style

e283ad9

dongjoon-hyun reviewed Jul 10, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SparkSessionExtensionSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 10, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SparkSessionExtensionSuite.scala Outdated Show resolved Hide resolved

Fix scala style

8a60a22

Merge branch 'master' into SPARK-28292

5ca207c

remove empty space

af759fa

HyukjinKwon closed this Sep 16, 2019

binmahone mentioned this pull request Feb 18, 2025

[FEA] user defined hints to better optimize query execution NVIDIA/spark-rapids#12162

Open

[SPARK-28292][SQL] Enable inject user-defined Hint #25071

[SPARK-28292][SQL] Enable inject user-defined Hint #25071

Uh oh!

Conversation

AngersZhuuuu commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dongjoon-hyun Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

maropu Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 10, 2019

Uh oh!

dongjoon-hyun commented Jul 10, 2019

Uh oh!

SparkQA commented Jul 10, 2019

Uh oh!

AngersZhuuuu commented Jul 10, 2019

Uh oh!

SparkQA commented Jul 10, 2019

Uh oh!

SparkQA commented Jul 10, 2019

Uh oh!

SparkQA commented Jul 10, 2019

Uh oh!

dongjoon-hyun commented Jul 10, 2019

Uh oh!

Uh oh!

Uh oh!

AngersZhuuuu commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jul 11, 2019

Uh oh!

AngersZhuuuu commented Jul 13, 2019

Uh oh!

dongjoon-hyun commented Jul 16, 2019

Uh oh!

maropu commented Jul 16, 2019

Uh oh!

AngersZhuuuu commented Jul 16, 2019

Uh oh!

SparkQA commented Jul 16, 2019

Uh oh!

maropu commented Jul 16, 2019

Uh oh!

AngersZhuuuu commented Jul 16, 2019

Uh oh!

SparkQA commented Jul 16, 2019

Uh oh!

SparkQA commented Sep 10, 2019

Uh oh!

maryannxue commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AngersZhuuuu commented Sep 10, 2019

Uh oh!

maryannxue commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AngersZhuuuu commented Sep 10, 2019

Uh oh!

maryannxue commented Sep 10, 2019

Uh oh!

gatorsmile commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AngersZhuuuu commented Jul 8, 2019 •

edited

Loading

AngersZhuuuu commented Jul 11, 2019 •

edited

Loading

maryannxue commented Sep 10, 2019 •

edited

Loading

maryannxue commented Sep 10, 2019 •

edited

Loading

gatorsmile commented Sep 10, 2019 •

edited

Loading