Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-23140][SQL] Add DataSourceV2Strategy to Hive Session state's planner #20305

Closed
wants to merge 3 commits into from
Closed

[SPARK-23140][SQL] Add DataSourceV2Strategy to Hive Session state's planner #20305

wants to merge 3 commits into from

Conversation

jerryshao
Copy link
Contributor

What changes were proposed in this pull request?

DataSourceV2Strategy is missing in HiveSessionStateBuilder's planner, which will throw exception as described in SPARK-23140.

How was this patch tested?

Manual test.

@jerryshao
Copy link
Contributor Author

@cloud-fan , please help to review, thanks!

cloud-fan
cloud-fan reviewed Jan 18, 2018
View reviewed changes
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionStateBuilder.scala Outdated
@@ -101,6 +102,7 @@ class HiveSessionStateBuilder(session: SparkSession, parentState: Option[Session
override def strategies: Seq[Strategy] = {
experimentalMethods.extraStrategies ++
extraPlanningStrategies ++ Seq(
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change it to super. strategies ++ Seq(HiveTableScans, Scripts)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the ordering matters, If I put Hive related strategies in the end, some unit tests will be failed.

@cloud-fan
Copy link
Contributor

thanks, good catch!

@SparkQA
Copy link

SparkQA commented Jan 18, 2018

Test build #86311 has finished for PR 20305 at commit fc5d3ce.

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

@cloud-fan
Copy link
Contributor

LGTM

@SparkQA
Copy link

SparkQA commented Jan 18, 2018

Test build #86313 has finished for PR 20305 at commit b721eb6.

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

@SparkQA
Copy link

SparkQA commented Jan 18, 2018

Test build #86318 has finished for PR 20305 at commit f17b44d.

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

cloud-fan
cloud-fan reviewed Jan 18, 2018
View reviewed changes
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionStateBuilder.scala Outdated
BasicOperators
)
}
override def strategies: Seq[Strategy] = Seq(HiveTableScans, Scripts) ++ super.strategies
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the assumption that experimentalMethods.extraStrategies should always run first.

I think we can just do:

override def extraPlanningStrategies: Seq[Strategy] =
  super.extraPlanningStrategies ++ customPlanningStrategies ++ Seq(HiveTableScans, Scripts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, let me update it.

@SparkQA
Copy link

SparkQA commented Jan 18, 2018

Test build #86326 has finished for PR 20305 at commit 094b7eb.

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

@SparkQA
Copy link

SparkQA commented Jan 18, 2018

Test build #86328 has finished for PR 20305 at commit a978dcc.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master/2.3!

asfgit pushed a commit that referenced this pull request Jan 18, 2018
@jerryshao @cloud-fan
[SPARK-23140][SQL] Add DataSourceV2Strategy to Hive Session state's p…
f801ac4 
…lanner

## What changes were proposed in this pull request?

`DataSourceV2Strategy` is missing in `HiveSessionStateBuilder`'s planner, which will throw exception as described in [SPARK-23140](https://issues.apache.org/jira/browse/SPARK-23140).

## How was this patch tested?

Manual test.

Author: jerryshao <[email protected]>

Closes #20305 from jerryshao/SPARK-23140.

(cherry picked from commit 7a22483)
Signed-off-by: Wenchen Fan <[email protected]>
@asfgit asfgit closed this in 7a22483 Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloud-fan cloud-fan cloud-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.
3 participants
@jerryshao @cloud-fan @SparkQA