[SPARK-26576][SQL] Broadcast hint not applied to partitioned table by jzhuge · Pull Request #23507 · apache/spark

jzhuge · 2019-01-10T08:06:57Z

What changes were proposed in this pull request?

Make sure broadcast hint is applied to partitioned tables.

Since the issue exists in branch 2.0 to 2.4, but not in master, I created this PR for branch-2.4.

How was this patch tested?

A new unit test in PruneFileSourcePartitionsSuite
Unit test suites touched by SPARK-14581: JoinOptimizationSuite, FilterPushdownSuite, ColumnPruningSuite, and PruneFiltersSuite

@cloud-fan @davies @rxin

cloud-fan · 2019-01-10T09:58:22Z

can we check the physical plan to make sure BroadcastJoin is picked?

+1. And since master doesn't use ResolvedHint in optimization anymore, this issue doesn't exist in master, but figured it wouldn't hurt to add a test in master too.

Fine with porting at least the unit test to master.

cloud-fan · 2019-01-10T09:58:32Z

ok to test

cloud-fan · 2019-01-10T09:58:42Z

cc @gatorsmile @maryannxue

SparkQA · 2019-01-10T14:17:21Z

Test build #101013 has finished for PR 23507 at commit c3217f7.

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

SparkQA · 2019-01-10T22:25:59Z

Test build #101035 has finished for PR 23507 at commit dae47f1.

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

gatorsmile · 2019-01-11T02:22:29Z

The bug is not here. We should get rid of this lines:
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala#L71-L72

I have tried this fix, it works. However, I am not sure about the original rational to add this code in SPARK-14581, and trying to minimize the impact of removing it because there are other callers of PhysicalOperation.unapply.
If @davies and @cloud-fan are ok and we can pass test cases covering the scenarios for this code, I'd be happy to go with this fix.

Just a bug.

seems better to remove that line.

jzhuge · 2019-01-11T02:33:42Z

Test build #101035 failures seem unrelated. VersionSuite and HiveClientSuites pass locally for me.

SparkQA · 2019-01-11T08:05:02Z

Test build #101053 has finished for PR 23507 at commit 2189ce0.

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

cloud-fan · 2019-01-11T12:04:47Z

retest this please

SparkQA · 2019-01-11T16:31:24Z

Test build #101082 has finished for PR 23507 at commit 2189ce0.

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

gatorsmile · 2019-01-11T17:20:35Z

LGTM. Thanks! Merged to 2.4 and 2.3.

## What changes were proposed in this pull request? Make sure broadcast hint is applied to partitioned tables. Since the issue exists in branch 2.0 to 2.4, but not in master, I created this PR for branch-2.4. ## How was this patch tested? - A new unit test in PruneFileSourcePartitionsSuite - Unit test suites touched by SPARK-14581: JoinOptimizationSuite, FilterPushdownSuite, ColumnPruningSuite, and PruneFiltersSuite cloud-fan davies rxin Closes#23507 from jzhuge/SPARK-26576. Authored-by: John Zhuge <jzhuge@apache.org> Signed-off-by: gatorsmile <gatorsmile@gmail.com>

## What changes were proposed in this pull request? Make sure broadcast hint is applied to partitioned tables. Since the issue exists in branch 2.0 to 2.4, but not in master, I created this PR for branch-2.4. ## How was this patch tested? - A new unit test in PruneFileSourcePartitionsSuite - Unit test suites touched by SPARK-14581: JoinOptimizationSuite, FilterPushdownSuite, ColumnPruningSuite, and PruneFiltersSuite cloud-fan davies rxin Closes#23507 from jzhuge/SPARK-26576. Authored-by: John Zhuge <jzhuge@apache.org> Signed-off-by: gatorsmile <gatorsmile@gmail.com> (cherry picked from commit b9eb0e8) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

jzhuge · 2019-01-11T19:01:23Z

Thx @gatorsmile!

Should we merge it to master any way? Remove the buggy/dead code and add a unit test.

cloud-fan · 2019-01-12T10:55:58Z

The buggy code doesn't exist in master anymore, but it will be great to put the test in master. Can you send a PR to do it? thanks!

## What changes were proposed in this pull request? Make sure broadcast hint is applied to partitioned tables. ## How was this patch tested? - A new unit test in PruneFileSourcePartitionsSuite - Unit test suites touched by SPARK-14581: JoinOptimizationSuite, FilterPushdownSuite, ColumnPruningSuite, and PruneFiltersSuite Closesapache#23507 from jzhuge/SPARK-26576. Authored-by: John Zhuge <jzhuge@apache.org> Signed-off-by: gatorsmile <gatorsmile@gmail.com> (cherry picked from commit b9eb0e8)

srowen · 2019-01-13T14:14:00Z

I'm not 100% sure, but I think this is causing StreamingQuerySuite to fail consistently in 2.3, but not 2.4. See failures from https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/ and drill down to things like https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-2.3-test-maven-hadoop-2.6/577/testReport/ . Most of the subsequent build failures seem to be related to this test.

CC @maropu as this probably affects a 2.3.3 release.
@jzhuge @gatorsmile what do you think, is it an issue, and should we fix-forward or just revert from 2.3?

## What changes were proposed in this pull request? Make sure broadcast hint is applied to partitioned tables. ## How was this patch tested? - A new unit test in PruneFileSourcePartitionsSuite - Unit test suites touched by SPARK-14581: JoinOptimizationSuite, FilterPushdownSuite, ColumnPruningSuite, and PruneFiltersSuite Closes#23507 from jzhuge/SPARK-26576. Closes#23530 from jzhuge/SPARK-26576-master. Authored-by: John Zhuge <jzhuge@apache.org> Signed-off-by: gatorsmile <gatorsmile@gmail.com>

jzhuge · 2019-01-14T00:19:14Z

…

On Sun, Jan 13, 2019 at 6:14 AM Sean Owen ***@***.***> wrote: I'm not 100% sure, but I think this is causing StreamingQuerySuite to fail consistently in 2.3, but not 2.4. See failures from https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/ and drill down to things like https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-2.3-test-maven-hadoop-2.6/577/testReport/ . Most of the subsequent build failures seem to be related to this test. CC @maropu <https://github.com/maropu> as this probably affects a 2.3.3 release. @jzhuge <https://github.com/jzhuge> @gatorsmile <https://github.com/gatorsmile> what do you think, is it an issue, and should we fix-forward or just revert from 2.3? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23507 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABy-pElCg5Zk5nm3rmk93znWrHd3KIhHks5vCz9YgaJpZM4Z4-eK> .

-- John Zhuge

gatorsmile · 2019-01-14T06:39:37Z

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-2.3-test-maven-hadoop-2.6/581/testReport/junit/org.apache.spark.sql.streaming/StreamingOuterJoinSuite/windowed_right_outer_join/history/

shows this is not caused by this commit.

maropu · 2019-01-14T13:14:40Z

Sorry, but the rightest column in the page @gatorsmile showed means what? It seems StreamingOuterJoinSuite fails but the column shows pass?
Anyway, I'm not sure about why though, most of StreamingOuterJoinSuite test runs failed after this commit.... I'm running the test in branch-2.3 on my AWS env now...

maropu · 2019-01-14T13:53:16Z

I tried to run the unit tests windowed left outer join and windowed right outer join in StreamingOuterJoinSuite 3 times on my AWS env.(m4.2xlarge) though, all the runs passed....
Added cc: @tdas because these tests are related to structured streaming stuffs

srowen · 2019-01-14T14:49:21Z

It's possible it merely uncovered a different problem, in the test or main code, that only exists in 2.3.

How important is the change for 2.3? given that there is a release imminent, would it be OK to revert it now in 2.3, or does that lose an important fix?

maropu · 2019-01-15T01:14:55Z

+1 for reverting it from v2.3 (it might be worth digging into the cause though, IMO its not worth blocking the v2.3 release).
If other guys agree to that, I'll do it.

gatorsmile · 2019-01-15T01:18:26Z

I am fine to revert it, although I do not think they are related.

…

On Mon, Jan 14, 2019 at 5:15 PM Takeshi Yamamuro ***@***.***> wrote: +1 for reverting it from v2.3 (it might be worth digging into the cause though, IMO its not worth blocking the v2.3 release). If other guys agree to that, I'll do it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23507 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALCApfzW_cmV9X40QWQUEhF2gwSbOH3Rks5vDSu0gaJpZM4Z4-eK> .

jzhuge · 2019-01-15T01:47:04Z

+1 on reverting Failed to reproduce the issue. My internal Jenkins job mimic'ng https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-2.3-test-maven-hadoop-2.7/572/ completed 16,477 tests successfully. 14:32:59 + build/mvn -version 14:33:01 Using `mvn` from path: /mnt/builds/slave/tools/hudson.tasks.Maven_MavenInstallation/Maven_3.3.9/bin/mvn 14:33:01 Apache Maven 3.3.9 (bb52d8502b132ec0a5a3f4c09453c07478323dc5; 2015-11-10T16:41:47+00:00) 14:33:01 Maven home: /mnt/builds/slave/tools/hudson.tasks.Maven_MavenInstallation/Maven_3.3.9 14:33:01 Java version: 1.8.0_192, vendor: Azul Systems, Inc. 14:33:01 Java home: /usr/lib/jvm/zulu-8-amd64/jre 14:33:01 Default locale: en_US, platform encoding: UTF-8 14:33:01 OS name: "linux", version: "4.4.0-1072-aws", arch: "amd64", family: "unix" 14:33:01 + build/mvn clean package --batch-mode -DskipTests -Phadoop-2.7 -Pyarn -Phive -Phive-thriftserver -Pkinesis-asl -Pmesos 14:43:04 Executing Maven: -B -f /mnt/builds/slave/workspace/jzhuge-test2/pom.xml -Dmaven.repo.local=/mnt/builds/slave/workspace/jzhuge-test2/.repository test -DzincPort=6666 --fail-at-end -Phadoop-2.7 -Pyarn -Phive -Phive-thriftserver -Pkinesis-asl -Pmesos

…

On Mon, Jan 14, 2019 at 6:50 AM Sean Owen ***@***.***> wrote: It's possible it merely uncovered a different problem, in the test or main code, that only exists in 2.3. How important is the change for 2.3? given that there is a release imminent, would it be OK to revert it now in 2.3, or does that lose an important fix? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23507 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABy-pAvV0YizQrVOUP-I-RHzXaKeVAo8ks5vDJk_gaJpZM4Z4-eK> .

-- John Zhuge

maropu · 2019-01-15T04:42:46Z

What I really concern about is; should we fix the StreamingOuterJoinSuite issue before the v2.3.3 release? StreamingOuterJoinSuite sometimes fails before this commit and the test seems to be originally flaky in branch-2.3 only...

srowen · 2019-01-15T18:47:00Z

That would be ideal, of course; I have no idea what the issue is or difference is with 2.4. Up to you whether you want to investigate and hold up 2.3.3, or just punt on it and revert this commit and go ahead.

maropu · 2019-01-16T02:05:23Z

ok, for now, I'll revert the commit and start the vote for the v2.3.3 release.

maropu · 2019-01-16T14:33:30Z

I reverted this commit from branch-2.3, and then added a tag for v2.3.3-rc1 in the head.
https://github.com/apache/spark/commits/branch-2.3

I'm re-publishing a candidate now, so I'll start a first vote for v2.3.3 in a few days after the Jenkins tests checked.

gatorsmile · 2019-01-18T21:20:15Z

Thanks! @maropu

## What changes were proposed in this pull request? Make sure broadcast hint is applied to partitioned tables. ## How was this patch tested? - A new unit test in PruneFileSourcePartitionsSuite - Unit test suites touched by SPARK-14581: JoinOptimizationSuite, FilterPushdownSuite, ColumnPruningSuite, and PruneFiltersSuite Closesapache#23507 from jzhuge/SPARK-26576. Closesapache#23530 from jzhuge/SPARK-26576-master. Authored-by: John Zhuge <jzhuge@apache.org> Signed-off-by: gatorsmile <gatorsmile@gmail.com>

Make sure broadcast hint is applied to partitioned tables. Since the issue exists in branch 2.0 to 2.4, but not in master, I created this PR for branch-2.4. - A new unit test in PruneFileSourcePartitionsSuite - Unit test suites touched by SPARK-14581: JoinOptimizationSuite, FilterPushdownSuite, ColumnPruningSuite, and PruneFiltersSuite cloud-fan davies rxin Closesapache#23507 from jzhuge/SPARK-26576. Authored-by: John Zhuge <jzhuge@apache.org> Signed-off-by: gatorsmile <gatorsmile@gmail.com> (cherry picked from commit b9eb0e8) Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/PruneFileSourcePartitionsSuite.scala

## What changes were proposed in this pull request? Make sure broadcast hint is applied to partitioned tables. Since the issue exists in branch 2.0 to 2.4, but not in master, I created this PR for branch-2.4. ## How was this patch tested? - A new unit test in PruneFileSourcePartitionsSuite - Unit test suites touched by SPARK-14581: JoinOptimizationSuite, FilterPushdownSuite, ColumnPruningSuite, and PruneFiltersSuite cloud-fan davies rxin Closesapache#23507 from jzhuge/SPARK-26576. Authored-by: John Zhuge <jzhuge@apache.org> Signed-off-by: gatorsmile <gatorsmile@gmail.com>

cloud-fan reviewed Jan 10, 2019
View reviewed changes

gatorsmile reviewed Jan 11, 2019
View reviewed changes

jzhuge added 3 commits January 10, 2019 20:21

[SPARK-26576][SQL] Broadcast hint not applied to partitioned table
3947488

Update unit test
2fcfb79

Remove ResolvedHint case in PhysicalOperation.unapply instead
2189ce0

jzhuge force-pushed the SPARK-26576 branch from b5676c7 to 2189ce0Compare January 11, 2019 04:22

cloud-fan approved these changes Jan 11, 2019
View reviewed changes

eatoncys mentioned this pull request Jan 11, 2019
[SPARK-26599][SQL]BroardCast hint can not work with PruneFileSourcePartitions #23517
Closed

gatorsmile closed this Jan 11, 2019

jzhuge mentioned this pull request Jan 12, 2019
[SPARK-26576][SQL][TEST] Broadcast hint not applied to partitioned table #23530
Closed

Uh oh!

Conversation

jzhuge commented Jan 10, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fanJan 10, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzhugeJan 10, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 10, 2019

Uh oh!

cloud-fan commented Jan 10, 2019

Uh oh!

SparkQA commented Jan 10, 2019

Uh oh!

SparkQA commented Jan 10, 2019

Uh oh!

gatorsmileJan 11, 2019

Choose a reason for hiding this comment

Uh oh!

jzhugeJan 11, 2019

Choose a reason for hiding this comment

Uh oh!

gatorsmileJan 11, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fanJan 11, 2019

Choose a reason for hiding this comment

Uh oh!

jzhuge commented Jan 11, 2019

Uh oh!

SparkQA commented Jan 11, 2019

Uh oh!

cloud-fan commented Jan 11, 2019

Uh oh!

SparkQA commented Jan 11, 2019

Uh oh!

gatorsmile commented Jan 11, 2019

Uh oh!

jzhuge commented Jan 11, 2019

Uh oh!

cloud-fan commented Jan 12, 2019

Uh oh!

srowen commented Jan 13, 2019

Uh oh!

jzhuge commented Jan 14, 2019 via email

Uh oh!

gatorsmile commented Jan 14, 2019

Uh oh!

maropu commented Jan 14, 2019

Uh oh!

maropu commented Jan 14, 2019

Uh oh!

srowen commented Jan 14, 2019

Uh oh!

maropu commented Jan 15, 2019

Uh oh!

gatorsmile commented Jan 15, 2019 via email

Uh oh!

jzhuge commented Jan 15, 2019 via email

Uh oh!

maropu commented Jan 15, 2019

Uh oh!

srowen commented Jan 15, 2019

Uh oh!

maropu commented Jan 16, 2019

Uh oh!

maropu commented Jan 16, 2019

Uh oh!

gatorsmile commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

maryannxueJan 10, 2019•
edited
Loading