[SPARK-19149] [SQL] Unify two sets of statistics in LogicalPlan #16529
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @rxin @cloud-fan |
|
LGTM |
|
Test build #71124 has finished for PR 16529 at commit
|
|
Test build #71129 has finished for PR 16529 at commit
|
|
retest this please |
|
Test build #71131 has finished for PR 16529 at commit
|
rxin
reviewed
Jan 10, 2017
| @@ -81,44 +81,36 @@ abstract class LogicalPlan extends QueryPlan[LogicalPlan] with Logging { | |||
| } | |||
| } | |||
|
|
|||
| /** A cache for the estimated statistics, such that it will only be computed once. */ | |||
| private val statsCache = new ThreadLocal[Option[Statistics]] { | |||
There was a problem hiding this comment.
why does this need to be thread local?
There was a problem hiding this comment.
Is it possible a LogicalPlan is shared by different threads? May I ask what's your concern about multi-thread in #16401 (comment)?
There was a problem hiding this comment.
ok i was thinking you could just use an AtomicReference
|
Merging in master. I will fix the thread local thing in a pr. |
|
Actually the multi-threaded issue probably doesn't matter. I will just change it to the original Option implementation. |
This was referenced Jan 11, 2017
ghost
pushed a commit
to dbtsai/spark
that referenced
this pull request
Jan 11, 2017
## What changes were proposed in this pull request? This patch simplifies slightly the logical plan statistics cache implementation, as discussed in apache#16529 ## How was this patch tested? N/A - this has no behavior change. Author: Reynold Xin <rxin@databricks.com> Closes apache#16544 from rxin/SPARK-19149.
uzadude
pushed a commit
to uzadude/spark
that referenced
this pull request
Jan 27, 2017
## What changes were proposed in this pull request? Currently we have two sets of statistics in LogicalPlan: a simple stats and a stats estimated by cbo, but the computing logic and naming are quite confusing, we need to unify these two sets of stats. ## How was this patch tested? Just modify existing tests. Author: wangzhenhua <wangzhenhua@huawei.com> Author: Zhenhua Wang <wzh_zju@163.com> Closes apache#16529 from wzhfy/unifyStats.
uzadude
pushed a commit
to uzadude/spark
that referenced
this pull request
Jan 27, 2017
## What changes were proposed in this pull request? This patch simplifies slightly the logical plan statistics cache implementation, as discussed in apache#16529 ## How was this patch tested? N/A - this has no behavior change. Author: Reynold Xin <rxin@databricks.com> Closes apache#16544 from rxin/SPARK-19149.
cmonkey
pushed a commit
to cmonkey/spark
that referenced
this pull request
Feb 15, 2017
## What changes were proposed in this pull request? Currently we have two sets of statistics in LogicalPlan: a simple stats and a stats estimated by cbo, but the computing logic and naming are quite confusing, we need to unify these two sets of stats. ## How was this patch tested? Just modify existing tests. Author: wangzhenhua <wangzhenhua@huawei.com> Author: Zhenhua Wang <wzh_zju@163.com> Closes apache#16529 from wzhfy/unifyStats.
cmonkey
pushed a commit
to cmonkey/spark
that referenced
this pull request
Feb 15, 2017
## What changes were proposed in this pull request? This patch simplifies slightly the logical plan statistics cache implementation, as discussed in apache#16529 ## How was this patch tested? N/A - this has no behavior change. Author: Reynold Xin <rxin@databricks.com> Closes apache#16544 from rxin/SPARK-19149.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Currently we have two sets of statistics in LogicalPlan: a simple stats and a stats estimated by cbo, but the computing logic and naming are quite confusing, we need to unify these two sets of stats.
How was this patch tested?
Just modify existing tests.