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-21180][SQL] Remove conf from stats functions since now we have conf in LogicalPlan #18391

Closed
wants to merge 3 commits into from
Closed

[SPARK-21180][SQL] Remove conf from stats functions since now we have conf in LogicalPlan #18391

wants to merge 3 commits into from

Conversation

wzhfy
Copy link
Contributor

@wzhfy wzhfy commented Jun 22, 2017 •
edited
Loading

What changes were proposed in this pull request?

After wiring SQLConf in logical plan (PR 18299), we can remove the need of passing conf into def stats and def computeStats.

How was this patch tested?

Covered by existing tests, plus some modified existing tests.

@wzhfy
Copy link
Contributor Author

wzhfy commented Jun 22, 2017

cc @rxin

kiszk
kiszk reviewed Jun 22, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LocalRelation.scala
@@ -67,7 +67,7 @@ case class LocalRelation(output: Seq[Attribute], data: Seq[InternalRow] = Nil)
}
}

override def computeStats(conf: SQLConf): Statistics =
override def computeStats: Statistics =
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove import org.apache.spark.sql.internal.SQLConf, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked all modified files and removed unused imports. Thanks!

kiszk
kiszk reviewed Jun 22, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala
@@ -436,7 +436,7 @@ case class CatalogRelation(
createTime = -1
))

override def computeStats(conf: SQLConf): Statistics = {
override def computeStats: Statistics = {
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove import org.apache.spark.sql.internal.SQLConf, too?

kiszk
kiszk reviewed Jun 22, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala
@@ -90,8 +90,8 @@ abstract class LogicalPlan extends QueryPlan[LogicalPlan] with QueryPlanConstrai
* first time. If the configuration changes, the cache can be invalidated by calling
* [[invalidateStatsCache()]].
*/
final def stats(conf: SQLConf): Statistics = statsCache.getOrElse {
statsCache = Some(computeStats(conf))
final def stats: Statistics = statsCache.getOrElse {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

kiszk
kiszk reviewed Jun 22, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/hints.scala
@@ -44,8 +44,8 @@ case class ResolvedHint(child: LogicalPlan, hints: HintInfo = HintInfo())

override lazy val canonicalized: LogicalPlan = child.canonicalized

override def computeStats(conf: SQLConf): Statistics = {
val stats = child.stats(conf)
override def computeStats: Statistics = {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@SparkQA
Copy link

SparkQA commented Jun 22, 2017

Test build #78459 has finished for PR 18391 at commit 5999539.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class FilterEstimation(plan: Filter) extends Logging
  • case class InnerOuterEstimation(join: Join) extends Logging
  • case class LeftSemiAntiEstimation(join: Join)

@kiszk
Copy link
Member

kiszk commented Jun 23, 2017

thanks, SGTM

@SparkQA
Copy link

SparkQA commented Jun 23, 2017

Test build #78496 has finished for PR 18391 at commit b6025b4.

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

gatorsmile
gatorsmile reviewed Jun 23, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala
@@ -436,7 +436,7 @@ case class CatalogRelation(
createTime = -1
))

override def computeStats(conf: SQLConf): Statistics = {
override def computeStats: Statistics = {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this file's import org.apache.spark.sql.internal.SQLConf need to be removed

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I overlooked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I checked the other interface file, there are several files with this name...

gatorsmile
gatorsmile reviewed Jun 23, 2017
View reviewed changes
...src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala Outdated
SQLConf.get.setConf(SQLConf.CBO_ENABLED, false)
assert(plan.stats == expectedStatsCboOff)
} finally {
SQLConf.get.unsetConf(SQLConf.CBO_ENABLED)
Copy link
Member

Choose a reason for hiding this comment

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

first, get the original and then recover the original one in the finally block. You can check how we did it in SQLConfSuite

gatorsmile
gatorsmile reviewed Jun 23, 2017
View reviewed changes
.../src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/AggregateEstimationSuite.scala Outdated
// From UnaryNode.computeStats, childSize * outputRowSize / childRowSize
Statistics(sizeInBytes = 48 * (8 + 4 + 8) / (8 + 4)))
} finally {
SQLConf.get.unsetConf(SQLConf.CBO_ENABLED)
Copy link
Member

Choose a reason for hiding this comment

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

The same here.

gatorsmile
gatorsmile reviewed Jun 23, 2017
View reviewed changes
...t/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/StatsEstimationTestBase.scala Outdated
}

override def afterAll(): Unit = {
SQLConf.get.unsetConf(SQLConf.CBO_ENABLED)
Copy link
Member

Choose a reason for hiding this comment

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

The same here.

@SparkQA
Copy link

SparkQA commented Jun 23, 2017

Test build #78521 has finished for PR 18391 at commit cd9f424.

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

@gatorsmile
Copy link
Member

LGTM

@gatorsmile
Copy link
Member

Thanks! Merging to master.

@asfgit asfgit closed this in b803b66 Jun 23, 2017
robert3005 pushed a commit to palantir/spark that referenced this pull request Jun 29, 2017
@wzhfy
[SPARK-21180][SQL] Remove conf from stats functions since now we have…
a6296bf 
… conf in LogicalPlan

## What changes were proposed in this pull request?

After wiring `SQLConf` in logical plan ([PR 18299](apache#18299)), we can remove the need of passing `conf` into `def stats` and `def computeStats`.

## How was this patch tested?

Covered by existing tests, plus some modified existing tests.

Author: wangzhenhua <wangzhenhua@huawei.com>
Author: Zhenhua Wang <wzh_zju@163.com>

Closes apache#18391 from wzhfy/removeConf.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gatorsmile gatorsmile gatorsmile left review comments

@kiszk kiszk kiszk 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.
4 participants
@wzhfy @SparkQA @kiszk @gatorsmile