[SPARK-28746][SQL] Add partitionby hint for sql queries #25464

ulysses-you · 2019-08-15T15:15:45Z

What changes were proposed in this pull request?

Now, RepartitionByExpression is allowed at Dataset method Dataset.repartition(). But in spark sql, we do not have an equivalent functionality.
In hive, we can use distribute by, so it's worth to add a hint to support such function.
Similar jira SPARK-24940

Why are the changes needed?

Make repartition hints consistent with repartition api .

Does this PR introduce any user-facing change?

This pr intends to support quries below;

// SQL cases
 - sql("SELECT /*+ REPARTITION(c) */ * FROM t")
 - sql("SELECT /*+ REPARTITION(1, c) */ * FROM t")
 - sql("SELECT /*+ REPARTITION_BY_RANGE(c) */ * FROM t")
 - sql("SELECT /*+ REPARTITION_BY_RANGE(1, c) */ * FROM t")

How was this patch tested?

UT

merge master

pull master

merge

maropu · 2019-08-16T01:34:41Z

@gatorsmile @maryannxue We need this?

maryannxue · 2019-08-21T19:43:18Z

I'm not against it, but is it possible to extend the existing "repartition" hint grammar to achieve this?

maropu · 2019-08-21T23:15:22Z

Yea, that approach looks more reasonable to me. Could you brush up the code based on the suggestion? @ulysses-you

ulysses-you · 2019-08-22T01:19:14Z

Thanks for review this. Make hint consistent with repartition api is a good idea.

ulysses-you · 2019-08-22T23:47:03Z

retest this please

maropu · 2019-08-23T00:33:53Z

ok to test

SparkQA · 2019-08-23T00:36:32Z

Test build #109600 has finished for PR 25464 at commit c57805b.

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

maropu · 2019-08-23T02:30:02Z

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

+      RepartitionByExpression(
+        exprs.map(_.asInstanceOf[UnresolvedAttribute]), h.child, numPartitions)
+    }
+


How about the case, SELECT /*+ REPARTITION(a) */ * FROM t?

maropu · 2019-08-23T02:30:20Z

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

+      if (errExprs.nonEmpty) throw new AnalysisException(
+        s"""Invalid type exprs : $errExprs
+           |expects UnresolvedAttribute type
+        """.stripMargin)


Plz add tests for this exception.

OK, I will add this later.

You don't add this test yet?

SparkQA · 2019-08-23T04:40:45Z

Test build #109603 has finished for PR 25464 at commit 4fbf34a.

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

SparkQA · 2019-08-23T07:05:02Z

Test build #109615 has finished for PR 25464 at commit b0ae689.

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

ulysses-you · 2019-08-23T09:35:08Z

retest this please

maropu · 2019-08-26T06:41:56Z

retest this please

SparkQA · 2019-08-26T07:05:01Z

Test build #109720 has finished for PR 25464 at commit b0ae689.

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

maropu · 2019-08-26T07:05:50Z

retet this please

ulysses-you · 2019-08-27T02:46:07Z

retest this please

maropu · 2019-08-27T02:47:42Z

oh.. typo..

maropu · 2019-08-27T05:05:24Z

retest this please

…8746

SparkQA · 2019-10-25T03:48:31Z

Test build #112637 has finished for PR 25464 at commit 507735c.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

maropu · 2019-10-25T04:13:50Z

retest this please

SparkQA · 2019-10-25T05:59:29Z

Test build #112639 has finished for PR 25464 at commit 44aa0b8.

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

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

Test build #112646 has finished for PR 25464 at commit 44aa0b8.

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

maropu · 2019-10-28T02:30:44Z

retest this please

maropu

LGTM. Anyone could do final checks? @cloud-fan @HyukjinKwon

SparkQA · 2019-10-28T06:27:53Z

Test build #112749 has finished for PR 25464 at commit 44aa0b8.

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

HyukjinKwon · 2019-10-29T09:31:03Z

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

+          throw new AnalysisException(s"$hintName Hint parameter should include columns, but " +
+            s"${invalidParams.mkString(", ")} found")
+        }
+        RepartitionByExpression(


Can we then consistently throw an exception like Dataset.repartition?

val sortOrders = partitionExprs.filter(_.expr.isInstanceOf[SortOrder]) if (sortOrders.nonEmpty) throw new IllegalArgumentException( s"""Invalid partitionExprs specified: $sortOrders |For range partitioning use repartitionByRange(...) instead. """.stripMargin)

Good point, add an IllegalArgumentException check.

HyukjinKwon

LGTM too otherwise.

SparkQA · 2019-10-29T17:19:52Z

Test build #112849 has finished for PR 25464 at commit 4ac7eb6.

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

maropu · 2019-10-29T23:26:37Z

Thanks for the contribution, @ulysses-you ! Merged to master.

maropu · 2019-10-29T23:31:36Z

FYI: I added @ulysses-you in the Spark contributor list.

ulysses-you · 2019-10-30T00:19:43Z

Thanks for great help ! @maropu @HyukjinKwon @cloud-fan @maryannxue

cloud-fan · 2019-11-22T11:05:14Z

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

+            createRepartition(shuffle = false, hint)
+          case "REPARTITION_BY_RANGE" =>
+            createRepartitionByRange(hint)
+          case _ => plan


shouldn't we return hint here? This will cause stack overflow once the hint is not the root node.

Ah, yes. I will make a followup. Thanks for catching this.

…unknown hint resolution ### What changes were proposed in this pull request? This is rather a followup of #25464 (see https://github.com/apache/spark/pull/25464/files#r349543286) It will cause an infinite recursion via mapping children - we should return the hint rather than its parent plan in unknown hint resolution. ### Why are the changes needed? Prevent Stack over flow during hint resolution. ### Does this PR introduce any user-facing change? Yes, it avoids stack overflow exception It was caused by #25464 and this is only in the master. No behaviour changes to end users as it happened only in the master. ### How was this patch tested? Unittest was added. Closes #26642 from HyukjinKwon/SPARK-30003. Authored-by: HyukjinKwon <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

hvanhovell · 2020-02-05T22:59:12Z

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

+          s"""Invalid partitionExprs specified: $sortOrders
+             |For range partitioning use REPARTITION_BY_RANGE instead.
+           """.stripMargin)
+        val invalidParams = partitionExprs.filter(!_.isInstanceOf[UnresolvedAttribute])


I think this check breaks the old API, in Spark 2.4 it is possible to use an expression here. I think we need to back this out.

AFAIK 2.4 only supports something like REPARTITION(5), the parameters here means anything after the partition number parameter, e.g. REPARTITION(5, para1, para2, ...)

Yea, I think so, too.

Ah ok, you are right. Never the less I think we should support expressions for REPARTITION here.

hvanhovell · 2020-02-05T23:00:13Z

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

+      def createRepartitionByExpression(
+          numPartitions: Int, partitionExprs: Seq[Any]): RepartitionByExpression = {
+        val sortOrders = partitionExprs.filter(_.isInstanceOf[SortOrder])
+        if (sortOrders.nonEmpty) throw new IllegalArgumentException(


Style, please put this inside curly braces and on a new line.

@ulysses-you Could you do follow-up for the two comments from @hvanhovell ?

Actually it might be better to just handle this later when we happen to touch this codes given that we don't usually make followups for minor styles issues.

hvanhovell · 2020-02-05T23:01:43Z

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

+
+      def createRepartitionByExpression(
+          numPartitions: Int, partitionExprs: Seq[Any]): RepartitionByExpression = {
+        val invalidParams = partitionExprs.filter(!_.isInstanceOf[UnresolvedAttribute])


Same comment as above.

ulysses-you and others added 5 commits July 9, 2019 14:38

Merge pull request #1 from apache/master

b259859

merge master

Merge pull request #2 from apache/master

33a8b0e

pull master

Merge pull request #3 from apache/master

5c7e871

merge

Merge pull request #4 from apache/master

ebe9220

merge

SPARK-28746

4f06914

dongjoon-hyun added the SQL label Aug 15, 2019

ulysses-you added 3 commits August 16, 2019 07:54

fix

bed289e

fix

67ce5d5

fix

c204ce3

move to existing repartition hint grammar

c57805b

fix style

4fbf34a

maropu reviewed Aug 23, 2019

View reviewed changes

add ut

b0ae689

ulysses-you added 2 commits October 25, 2019 09:39

Merge branch 'master' of https://github.com/apache/spark into SPARK-2…

e5eb369

…8746

fix

44aa0b8

maropu approved these changes Oct 28, 2019

View reviewed changes

HyukjinKwon reviewed Oct 29, 2019

View reviewed changes

HyukjinKwon approved these changes Oct 29, 2019

View reviewed changes

fix

4ac7eb6

maropu closed this in 6958d7e Oct 29, 2019

cloud-fan reviewed Nov 22, 2019

View reviewed changes

HyukjinKwon mentioned this pull request Nov 22, 2019

[SPARK-30003][SQL] Do not throw stack overflow exception in non-root unknown hint resolution #26642

Closed

maropu mentioned this pull request Dec 17, 2019

[SPARK-30278][SQL][DOC] Update Spark SQL document menu for new changes #26917

Closed

jackylee-ch mentioned this pull request Jan 6, 2020

[SPARK-30036][SQL] Fix: REPARTITION hint does not work with order by #26946

Closed

hvanhovell reviewed Feb 5, 2020

View reviewed changes

ulysses-you deleted the SPARK-28746 branch March 3, 2021 06:31

[SPARK-28746][SQL] Add partitionby hint for sql queries #25464

[SPARK-28746][SQL] Add partitionby hint for sql queries #25464

Uh oh!

Conversation

ulysses-you commented Aug 15, 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?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

maropu commented Aug 16, 2019

Uh oh!

maryannxue commented Aug 21, 2019

Uh oh!

maropu commented Aug 21, 2019

Uh oh!

ulysses-you commented Aug 22, 2019

Uh oh!

ulysses-you commented Aug 22, 2019

Uh oh!

maropu commented Aug 23, 2019

Uh oh!

SparkQA commented Aug 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 23, 2019

Uh oh!

SparkQA commented Aug 23, 2019

Uh oh!

ulysses-you commented Aug 23, 2019

Uh oh!

maropu commented Aug 26, 2019

Uh oh!

SparkQA commented Aug 26, 2019

Uh oh!

maropu commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulysses-you commented Aug 27, 2019

Uh oh!

maropu commented Aug 27, 2019

Uh oh!

maropu commented Aug 27, 2019

Uh oh!

SparkQA commented Oct 25, 2019

Uh oh!

maropu commented Oct 25, 2019

Uh oh!

SparkQA commented Oct 25, 2019

Uh oh!

SparkQA commented Oct 25, 2019

Uh oh!

maropu commented Oct 28, 2019

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 29, 2019

Uh oh!

maropu commented Oct 29, 2019

Uh oh!

maropu commented Oct 29, 2019

Uh oh!

ulysses-you commented Aug 15, 2019 •

edited

Loading

maropu commented Aug 26, 2019 •

edited

Loading

hvanhovell Feb 5, 2020 •

edited

Loading