Skip to content

[SPARK-23229][SQL] Dataset.hint should use planWithBarrier logical plan - #20405

Closed
jaceklaskowski wants to merge 1 commit into
apache:masterfrom
jaceklaskowski:SPARK-23229-hint-planWithBarrier
Closed

[SPARK-23229][SQL] Dataset.hint should use planWithBarrier logical plan#20405
jaceklaskowski wants to merge 1 commit into
apache:masterfrom
jaceklaskowski:SPARK-23229-hint-planWithBarrier

Conversation

@jaceklaskowski

@jaceklaskowskijaceklaskowski commented Jan 26, 2018

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Every time Dataset.hint is used it triggers execution of logical commands, their unions and hint resolution (among other things that analyzer does).

hint should use planWithBarrier instead.

Fixes https://issues.apache.org/jira/browse/SPARK-23229

How was this patch tested?

Existing unit tests, local build + awaiting Jenkins

@jaceklaskowski

jaceklaskowski commented Jan 26, 2018

Copy link
Copy Markdown
ContributorAuthor

/cc @cloud-fan

viirya
viirya reviewed Jan 26, 2018
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
@scala.annotation.varargs
def hint(name: String, parameters: Any*): Dataset[T] = withTypedPlan {
UnresolvedHint(name, parameters, logicalPlan)
UnresolvedHint(name, parameters, planWithBarrier)

@viiryaviiryaJan 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ResolveBroadcastHints rule will traverse recursively the children of logical plan. If we wrap it with a barrier, we can't be traverse down the tree.

@jaceklaskowskijaceklaskowskiJan 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding however is that planWithBarrier is already analyzed (and ResolveBroadcastHints as the very first rule had its chance to do its work). That's the extra processing hint does every time it's called. Using planWithBarrier makes it less "painful".

Just use hint twice and see the analyzed plan.

@cloud-fancloud-fanJan 26, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @viirya has a valid concern. think about

val df1 = spark.table("t").select("id")
df1.hint("broadcast", "t")

We should transform down the plan of df1, find the bottom table relation and apply the hint.

@jaceklaskowskijaceklaskowskiJan 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that that's what ResolveBroadcastHints does --> https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveHints.scala#L93-L101, doesn't it? I'm going to write a test case for it to confirm (and that's what I was asking for in the email to dev@spark the other day).

@viiryaviiryaJan 27, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaceklaskowski Because the logical plan is wrapped in analysis barrier, ResolveBroadcastHints can't traverse down it to reach the UnresolvedRelation/SubqueryAlias. at https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveHints.scala#L60-L61.

@cloud-fancloud-fanJan 29, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a possible workaround is to explicitly go through the barrier in the hint resolution rules, so that we can still use barrier here and skip analysis in other analyzer rules.

@viiryaviiryaJan 29, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. makes sense to me.

@cloud-fancloud-fanMay 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we go with this workaround?

@viiryaviiryaMay 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. cc @jaceklaskowski

@SparkQA

SparkQA commented Jan 26, 2018

Copy link
Copy Markdown

Test build #86702 has finished for PR 20405 at commit 47bb245.

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

@jaceklaskowski

jaceklaskowski commented Jan 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Looks like the tests failed due to "java.io.IOException: Failed to delete: /home/jenkins/workspace/SparkPullRequestBuilder/target/tmp/spark-5a9b5811-306d-4ba0-8bfb-9e263ddf47b8" Is this because of the change or a "misnomer"?

@HyukjinKwon

HyukjinKwon commented Jul 16, 2018

Copy link
Copy Markdown
Member

retest this please

@SparkQA

SparkQA commented Jul 16, 2018

Copy link
Copy Markdown

Test build #93078 has finished for PR 20405 at commit 47bb245.

  • This patch fails to generate documentation.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile

gatorsmile commented Jul 16, 2018

Copy link
Copy Markdown
Member

@HyukjinKwon Does the existing AnalysisBarrier introduce a regression when users use the hint like df1.hint("broadcast", "t")?

@viirya

viirya commented Jul 16, 2018

Copy link
Copy Markdown
Member

When we use a hint an analyzed plan goes through analysis again. If I remember it correctly it is happened before analysis barrier is introduced. This PR wants to wrap an AnalysisBarrier on it so we won't re-analyze it.

@SparkQA

SparkQA commented Jul 25, 2018

Copy link
Copy Markdown

Test build #93559 has finished for PR 20405 at commit 47bb245.

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

@cloud-fan

cloud-fan commented Jul 27, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

I think this can be closed in favor of #21822

@dongjoon-hyun

dongjoon-hyun commented Sep 11, 2018

Copy link
Copy Markdown
Member

Hi, @jaceklaskowski . Could you close this PR?

@dongjoon-hyun

dongjoon-hyun commented Sep 16, 2018

Copy link
Copy Markdown
Member

Gentle ping, @jaceklaskowski .

@maropu

maropu commented Sep 18, 2018

Copy link
Copy Markdown
Member

kindly ping

@HyukjinKwon

HyukjinKwon commented Sep 27, 2018

Copy link
Copy Markdown
Member

ping @jaceklaskowski

@dongjoon-hyun

dongjoon-hyun commented Oct 2, 2018

Copy link
Copy Markdown
Member

@jaceklaskowski seems to ignore Git messages here. Should we ping him email since he is Spark dev mailing list?

@felixcheung

felixcheung commented Oct 2, 2018

Copy link
Copy Markdown
Member

probably.

@dongjoon-hyundongjoon-hyun mentioned this pull request Oct 24, 2018
Closed
zifeif2 pushed a commit to zifeif2/spark that referenced this pull request Nov 22, 2025
@srowen@zifeif2
[BUILD] Close stale PRs
25b8667
Closesapache#22567Closesapache#18457Closesapache#21517Closesapache#21858Closesapache#22383Closesapache#19219Closesapache#22401Closesapache#22811Closesapache#20405Closesapache#21933Closesapache#22819 from srowen/ClosePRs.
Authored-by: Sean Owen <sean.owen@databricks.com>
Signed-off-by: Sean Owen <sean.owen@databricks.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viiryaviiryaviirya left review comments
@cloud-fancloud-fancloud-fan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jaceklaskowski@SparkQA@HyukjinKwon@gatorsmile@viirya@cloud-fan@dongjoon-hyun@maropu@felixcheung