[SPARK-9563][SQL] Remove repartition operators when they are the child of Exchange and shuffle=True

[viirya]

JIRA: https://issues.apache.org/jira/browse/SPARK-9563

We should remove repartition operators when they are the child of Exchange and shuffle=True.

[SparkQA]

Test build #39858 has finished for PR 7959 at commit 5dc7be8.

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

[JoshRosen]

For completeness, we also need a test case to ensure that this pruning does not apply if shuffle=False.

[JoshRosen]

One other thought: a related optimization might be to collapse adjacent repartition calls. E.g. if I call repartition(10).repartition(100) then we'll actually repartition twice, whereas I think we may be able to safely drop the smaller repartition.

Similar optimizations may apply to coalesce, although I'd have to think a bit more about whether that risks introducing regressions in any corner-cases.

[viirya]

To optimise repartition(10).repartition(100) to repartition(100) seems reasonable. Is it also doable for repartition(100).repartition(10) to repartition(10)?

[SparkQA]

Test build #40000 has finished for PR 7959 at commit 4ba7a38.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

retest this please.

[SparkQA]

Test build #248 has finished for PR 7959 at commit 4ba7a38.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #40022 has finished for PR 7959 at commit 4ba7a38.

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

[SparkQA]

Test build #40285 has finished for PR 7959 at commit e1efa4c.

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

[JoshRosen]

How can this case ever occur, given that Exchange operators are currently added all at once in a single pass? Are you planning ahead in case we implement Repartition using Exchange, as in #8030?

[viirya]

Hmm, because logical.RepartitionByExpression will be transformed to execution.Exchange as well. I am wondering if it is possible to have an Exchange here.

[viirya]

Using Exchange for a Repartition is another possible case too.

[JoshRosen]

/cc @yhuai, do you have any thoughts on this PR? The basic change here seems fine to me.

[viirya]

test this please.

[viirya]

retest this please.

[SparkQA]

Test build #40429 has finished for PR 7959 at commit 0b334b7.

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

[viirya]

@JoshRosen Hi, I think this patch stays here too long. Is it ok to merge it?

[andrewor14]

@yhuai any thoughts? I think the high level approach makes sense.