You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by concretevitamin <gi...@git.apache.org> on 2014/06/27 01:14:34 UTC
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
GitHub user concretevitamin opened a pull request:
https://github.com/apache/spark/pull/1238
[SQL] [WIP] Prototype implementation of size estimations for Catalyst logical plans.
The idea is that every Catalyst logical plan gets hold of an Estimates class, the usage of which provides useful estimations on various statistics. See the implementations of `ParquetRelation` and `MetastoreRelation`.
This patch also includes one usage of the estimation interface -- namely, use physical table sizes from the estimate interface to convert an equi-join to a broadcast join (when doing so is beneficial, as determined by a size threshold).
This PR still needs some cleanups, but just want to put it out here first to gather feedback on the interface & high-level approach.
TODOs:
- [ ] Add a separate test suite & improve test coverage.
- [ ] Support `ParquetRelation` in the aforementioned BHJ optimization.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/concretevitamin/spark estimates
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1238.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1238
----
commit 54671bb8003629220375343427259cdecb8003ce
Author: Zongheng Yang <zo...@gmail.com>
Date: 2014-06-24T23:32:27Z
Prototype impl of estimations for Catalyst logical plans.
- Also add simple size-getters for ParquetRelation and
MetastoreRelation.
- Add a rule to auto-convert equi-joins to BroadcastHashJoin, if a table
has smaller size, based on the above getter (for MetastoreRelation).
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15243150
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala ---
@@ -66,8 +66,8 @@ abstract class SparkPlan extends QueryPlan[SparkPlan] with Logging {
* linking.
*/
@DeveloperApi
-case class SparkLogicalPlan(alreadyPlanned: SparkPlan, tableName: String = "SparkLogicalPlan")
- extends BaseRelation with MultiInstanceRelation {
+case class SparkLogicalPlan(alreadyPlanned: SparkPlan)(@transient sqlContext: SQLContext)
--- End diff --
That will be pretty awesome & reduce much boilerplate code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15019589
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,28 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ // TODO: handle overflow?
+ /**
+ * Estimates of various statistics. The default estimation logic simply sums up the corresponding
+ * statistic produced by the children. To override this behavior, override `statistics` and
+ * assign it a overriden version of `Statistics`.
+ */
+ case class Statistics(
+ /**
+ * Number of output tuples. For leaf operators this defaults to 1, otherwise it is set to the
+ * product of children's `numTuples`.
+ */
+ numTuples: Long = childrenStats.map(_.numTuples).product,
+
+ /**
+ * Physical size in bytes. For leaf operators this defaults to 1, otherwise it is set to the
+ * product of children's `sizeInBytes`.
+ */
+ sizeInBytes: Long = childrenStats.map(_.sizeInBytes).product
+ )
+ lazy val statistics: Statistics = new Statistics
+ lazy val childrenStats = children.map(_.statistics)
--- End diff --
I think we need a lazy marker for this, otherwise during the initialization of `childrenStats`, `statistics` in all children will get evaluated too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47291843
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48576858
QA results for PR 1238:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16492/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14904711
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala ---
@@ -47,6 +47,13 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
/**
* Uses the ExtractEquiJoinKeys pattern to find joins where at least some of the predicates can be
* evaluated by matching hash keys.
+ *
+ * This strategy applies a simple optimization based on the estimates of the physical sizes of
+ * the two join sides. When planning a [[execution.BroadcastHashJoin]], if one side has an
+ * estimated physical size smaller than the user-settable threshold
+ * `spark.sql.auto.convert.join.size`, the planner would mark it as the ''build'' relation and
--- End diff --
Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-50522286
Jenkins test this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15205438
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,26 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ // TODO: handle overflow?
--- End diff --
I think we will need to handle overflow before we can merge this it. It would be bad if overflow caused us to broadcast a table that too large to fit in memory.
Is there a reason we aren't just using BigInt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47998294
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49219047
Jenkins, test this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14902843
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -263,6 +268,19 @@ private[hive] case class MetastoreRelation
new Partition(hiveQlTable, p)
}
+ // TODO: are there any stats in hiveQlTable.getSkewedInfo that we can use?
+ @transient override lazy val statistics = new Statistics {
--- End diff --
We could certainly move it out of the Statistics class to reside in `MetastoreRelation`, but does `new Statistics` incur extra overheads? Otherwise I think the copy constructor may not be the good style here, as we have a long comment and relatively long implementation for the field `sizeInBytes`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48530250
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-50548734
Thanks! I've merged this into master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47292446
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47987609
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-50522696
QA tests have started for PR 1238. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17369/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14273921
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,18 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
+ lazy val numTuples: Long = childrenEstimations.map(_.numTuples).sum
+ lazy val size: Long = childrenEstimations.map(_.size).sum
--- End diff --
maybe rename size to sizeInBytes? So it is obvious in usage that this doesn't mean numTuples.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14906952
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -92,6 +114,8 @@ abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
abstract class LeafNode extends LogicalPlan with trees.LeafNode[LogicalPlan] {
self: Product =>
+ override lazy val statistics = Statistics(numTuples = 1L, sizeInBytes = 1L)
--- End diff --
Setting large default values here (1) doesn't seem to make intuitive sense, and (2) can cause superfluous overflows when things are getting multiplied.
This [commit](https://github.com/concretevitamin/spark/commit/f41ecf22d516bfeabc4168f2a1e6518169d85c29) introduced this change, which makes 1L a special default value that gets preserved even when a bunch of operators lack meaningful estimates -- because the default estimates will just multiply a bunch of 1's together, producing the special value 1L. With this preservation, we could test the absence of meaningful estimates and correspondingly do special handling, as shown in that commit. (One example in that commit: if RDDs don't have meaningful estimates, the field defaults to [SQLConf#statsDefaultSizeInBytes](https://github.com/apache/spark/pull/1238/files#diff-41ef65b9ef5b518f77e2a03559893f4dR61), which is set to be greater than the auto join conversion threshold.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14902139
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,28 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ // TODO: handle overflow?
+ /**
+ * Estimates of various statistics. The default estimation logic simply sums up the corresponding
+ * statistic produced by the children. To override this behavior, override `statistics` and
+ * assign it a overriden version of `Statistics`.
+ */
+ case class Statistics(
+ /**
+ * Number of output tuples. For leaf operators this defaults to 1, otherwise it is set to the
+ * product of children's `numTuples`.
+ */
+ numTuples: Long = childrenStats.map(_.numTuples).product,
+
+ /**
+ * Physical size in bytes. For leaf operators this defaults to 1, otherwise it is set to the
+ * product of children's `sizeInBytes`.
+ */
+ sizeInBytes: Long = childrenStats.map(_.sizeInBytes).product
+ )
+ lazy val statistics: Statistics = new Statistics
+ lazy val childrenStats = children.map(_.statistics)
--- End diff --
We are already memoizing the statistics in the child, so we probably don't need to do it again here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48547543
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15206175
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -263,6 +268,19 @@ private[hive] case class MetastoreRelation
new Partition(hiveQlTable, p)
}
+ @transient override lazy val statistics = Statistics(
+ // TODO: check if this estimate is valid for tables after partition pruning.
+ sizeInBytes = {
+ // NOTE: kind of hacky, but this should be relatively cheap if parameters for the table are
+ // populated into the metastore. An alternative would be going through Hadoop's FileSystem
+ // API, which can be expensive if a lot of RPCs are involved. Besides `totalSize`, there are
+ // also `numFiles`, `numRows`, `rawDataSize` keys we can look at in the future.
+ val sizeMaybeFromMetastore =
+ Option(hiveQlTable.getParameters.get("totalSize")).map(_.toLong).getOrElse(-1L)
+ math.max(sizeMaybeFromMetastore, 1L)
--- End diff --
Why `1L` and not default size?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14901688
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -263,6 +268,19 @@ private[hive] case class MetastoreRelation
new Partition(hiveQlTable, p)
}
+ // TODO: are there any stats in hiveQlTable.getSkewedInfo that we can use?
+ @transient override lazy val statistics = new Statistics {
--- End diff --
This is still using the old non case class style.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48576861
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48576864
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16492/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48538216
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14273941
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -258,6 +263,19 @@ private[hive] case class MetastoreRelation
new Partition(hiveQlTable, p)
}
+ // TODO: are there any stats in hiveQlTable.getSkewedInfo that we can use?
+ @transient override lazy val estimates = new Estimates {
+ // Size getters adapted from
+ // https://github.com/apache/hive/blob/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/SizeBasedBigTableSelectorForAutoSMJ.java
+ override lazy val size: Long =
+ maybeGetSize(hiveConf, hiveQlTable.getProperty("totalSize"), path)
+
+ private[this] def maybeGetSize(conf: HiveConf, size: String, path: Path): Long = {
+ val res = try { Some(size.toLong) } catch { case _: Exception => None }
+ res.getOrElse { path.getFileSystem(conf).getContentSummary(path).getLength }
--- End diff --
Does this get the size after partition pruning? If not, add a TODO here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47987156
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47292134
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48954264
QA results for PR 1238:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16632/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48570611
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14273582
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala ---
@@ -72,8 +72,9 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
condition,
left,
right @ PhysicalOperation(_, _, b: BaseRelation))
- if broadcastTables.contains(b.tableName) =>
- broadcastHashJoin(leftKeys, rightKeys, left, right, condition, BuildRight)
+ if broadcastTables.contains(b.tableName)
--- End diff --
remove ```broadcastTables.contains(b.tableName)``` and then your if can fit in a single line, and you don't need to indent the next line ...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47989913
Jenkins, retest this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48570696
QA tests have started for PR 1238. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16492/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47292249
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16174/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47292247
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49222667
Jenkins, test this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-50531985
QA results for PR 1238:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17369/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47986687
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15024063
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,28 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ // TODO: handle overflow?
+ /**
+ * Estimates of various statistics. The default estimation logic simply sums up the corresponding
+ * statistic produced by the children. To override this behavior, override `statistics` and
+ * assign it a overriden version of `Statistics`.
+ */
+ case class Statistics(
+ /**
+ * Number of output tuples. For leaf operators this defaults to 1, otherwise it is set to the
+ * product of children's `numTuples`.
+ */
+ numTuples: Long = childrenStats.map(_.numTuples).product,
+
+ /**
+ * Physical size in bytes. For leaf operators this defaults to 1, otherwise it is set to the
+ * product of children's `sizeInBytes`.
+ */
+ sizeInBytes: Long = childrenStats.map(_.sizeInBytes).product
+ )
+ lazy val statistics: Statistics = new Statistics
+ lazy val childrenStats = children.map(_.statistics)
--- End diff --
Thanks for the note -- I wasn't well aware of the memory/time overhead of lazy vals. Since we have one statistic at the moment, `childrenStats` is used only once for a logical node, so I went ahead and removed it.
However, memoizing `statistics` itself probably would save some planning time, as it will get called multiple times during planning (e.g. different cases in the HashJoin pattern match). If we were to use a `def` it is going to be evaluated in the quadratic order.
Using `val` only will fire off the computation of the stats for all logical nodes right away. I think this might be less desirable to only evaluating it for everything under a Join when needed by a Strategy. Also, we'd have very few operators in a typical query anyway.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14857026
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -21,16 +21,27 @@ import java.util.Properties
import scala.collection.JavaConverters._
+private object SQLConf {
+ @transient protected[spark] val confSettings = java.util.Collections.synchronizedMap(
--- End diff --
Let's remove singletons.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14857044
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala ---
@@ -47,6 +47,13 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
/**
* Uses the ExtractEquiJoinKeys pattern to find joins where at least some of the predicates can be
* evaluated by matching hash keys.
+ *
+ * This strategy applies a simple optimization based on the estimates of the physical sizes of
+ * the two join sides. When planning a [[execution.BroadcastHashJoin]], if one side has an
+ * estimated physical size smaller than the user-settable threshold
+ * `spark.sql.auto.convert.join.size`, the planner would mark it as the ''build'' relation and
--- End diff --
Document it in more explicit ways that a table will get broadcasted (instead of just saying "build"; you can still "build" in shuffle)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47990859
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14273632
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/JoinSuite.scala ---
@@ -28,6 +29,14 @@ class JoinSuite extends QueryTest {
// Ensures tables are loaded.
TestData
+ test("parquet") {
--- End diff --
this doesn't really belong in JoinSuite, does it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14273901
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,18 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
--- End diff --
How is cardinality different from numTuples here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47993164
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16317/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49219540
QA tests have started for PR 1238. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16743/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14273707
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -245,7 +246,11 @@ object HiveMetastoreTypes extends RegexParsers {
private[hive] case class MetastoreRelation
(databaseName: String, tableName: String, alias: Option[String])
(val table: TTable, val partitions: Seq[TPartition])
+ (@transient hiveConf: HiveConf, @transient path: Path)
--- End diff --
does the 3 level currying buy us anything? (I'm not sure even why we curry it in the first place ...)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48546717
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16480/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14538726
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,18 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
--- End diff --
Resolved.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48540239
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49223292
QA tests have started for PR 1238. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16745/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48538197
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14573140
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala ---
@@ -61,8 +61,6 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
condition.map(Filter(_, broadcastHashJoin)).getOrElse(broadcastHashJoin) :: Nil
}
- def broadcastTables: Seq[String] = sqlContext.joinBroadcastTables.split(",").toBuffer
-
def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
--- End diff --
For the config option to control the size-based conversion: what do people think about this? I'd note that Hive does have such a switch, yet if users want to turn off the conversion, setting the threshold to zero effectively achieves the same thing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15243385
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala ---
@@ -66,8 +66,8 @@ abstract class SparkPlan extends QueryPlan[SparkPlan] with Logging {
* linking.
*/
@DeveloperApi
-case class SparkLogicalPlan(alreadyPlanned: SparkPlan, tableName: String = "SparkLogicalPlan")
- extends BaseRelation with MultiInstanceRelation {
+case class SparkLogicalPlan(alreadyPlanned: SparkPlan)(@transient sqlContext: SQLContext)
--- End diff --
https://github.com/marmbrus/spark/commit/d2ad5c54c4c309fda49ece99d744839693d6f879
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47990860
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16319/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14902232
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -263,6 +268,19 @@ private[hive] case class MetastoreRelation
new Partition(hiveQlTable, p)
}
+ // TODO: are there any stats in hiveQlTable.getSkewedInfo that we can use?
+ @transient override lazy val statistics = new Statistics {
--- End diff --
Well, that's because we need a helper method `maybeGetSize()` here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48570625
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47987610
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16316/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49785142
QA tests have started for PR 1238. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16981/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47986699
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49796558
QA results for PR 1238:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16981/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48648099
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16505/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48530050
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49233766
QA results for PR 1238:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16745/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47990092
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47993473
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14273785
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -258,6 +263,19 @@ private[hive] case class MetastoreRelation
new Partition(hiveQlTable, p)
}
+ // TODO: are there any stats in hiveQlTable.getSkewedInfo that we can use?
+ @transient override lazy val estimates = new Estimates {
+ // Size getters adapted from
+ // https://github.com/apache/hive/blob/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/SizeBasedBigTableSelectorForAutoSMJ.java
--- End diff --
Also it is very long :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47996531
Cool, I will check this out. Please create a JIRA.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47987148
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48941276
Jenkins, test this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15205473
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -22,15 +22,21 @@ import java.util.Properties
import scala.collection.JavaConverters._
/**
- * SQLConf holds mutable config parameters and hints. These can be set and
- * queried either by passing SET commands into Spark SQL's DSL
- * functions (sql(), hql(), etc.), or by programmatically using setters and
- * getters of this class.
+ * A trait that enables the setting and getting of mutable config parameters/hints. The central
+ * location for storing them is uniquely located in the same-name private companion object.
+ * Therefore, all classes that mix in this trait share all the hints.
--- End diff --
Is this comment correct? I don't see a companion object.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48546714
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14902341
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -92,6 +114,8 @@ abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
abstract class LeafNode extends LogicalPlan with trees.LeafNode[LogicalPlan] {
self: Product =>
+ override lazy val statistics = Statistics(numTuples = 1L, sizeInBytes = 1L)
--- End diff --
Why are we defaulting to such small values here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48645064
To handle potential overflow (one last TODO), I think there are a couple alternatives:
- A: Throw exceptions for overflowing operations. Similar to [1].
- B: Use [1], but replace the overflow situations with a Top and a Bottom that absorb/saturate things correspondingly. Similar concepts here [2].
- C: Use [3] (or reimplement parts of it), which just carry out an overflowing operation in the lifted BigInt counterparts.
I think Approach A is bad as when dealing with big data we'd almost certainly run into this case in the future. Approach B is reasonable in that whenever we see a Top/Bottom, we could just disable/special-case the cost estimation. Approach C looks okay too but may be too heavy.
Let me know what do you guys think should go into this PR.
[1] https://github.com/twitter/util/blob/master/util-core/src/main/scala/com/twitter/util/LongOverflowArith.scala
[2] https://github.com/twitter/util/blob/master/util-core/src/main/scala/com/twitter/util/Duration.scala
[3] https://github.com/non/spire/blob/master/core/src/main/scala/spire/math/SafeLong.scala
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14677118
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala ---
@@ -61,8 +61,6 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
condition.map(Filter(_, broadcastHashJoin)).getOrElse(broadcastHashJoin) :: Nil
}
- def broadcastTables: Seq[String] = sqlContext.joinBroadcastTables.split(",").toBuffer
-
def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
--- End diff --
I don't see a need for two config options here. -1 as the threshold works as a perfectly reasonable way to turn off auto-broadcasting.
I agree that it would be a good idea to set the build side for hash joins to the relation with the smaller estimated size. If its not too hard we should just do that in this PR too.
We should add comments to the scala doc that explain the estimation magic that is going on here now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48648098
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15199553
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -92,6 +112,8 @@ abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
abstract class LeafNode extends LogicalPlan with trees.LeafNode[LogicalPlan] {
self: Product =>
+ override lazy val statistics = Statistics(sizeInBytes = 1L)
--- End diff --
Why is this `1L` and not `statsDefaultSizeInBytes`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48648084
QA results for PR 1238:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16505/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47990105
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14676669
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,17 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
+ lazy val sizeInBytes: Long = childrenEstimations.map(_.sizeInBytes).sum
--- End diff --
A few notes here:
- I think the most important thing about any default values that we set is that they work together correctly. For example, we probably want the default size for RDDs to be large enough that they do not get broadcast using the default value. For this reason we probably want to have one central location where defaults are set (probably in SQLConf).
- Since RDDs are immutable, it may be possible to augment things so that we gather simple statistics the first time the RDD is queried, perhaps using an accumulator. This however does not need to be done in this PR. I would make a note of it though.
- Regarding joins: for inner joins if one side is actually zero then the join will yield no results. For outer joins the maximum possible output cardinality is the cardinality of the non-empty relation. However, zero is kind of a weird estimate as if it is wrong it will really throw things off. For this reason maybe we should consider 1 to be the smallest possible cardinality estimate.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by chhao01 <gi...@git.apache.org>.
Github user chhao01 commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14581617
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala ---
@@ -61,8 +61,6 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
condition.map(Filter(_, broadcastHashJoin)).getOrElse(broadcastHashJoin) :: Nil
}
- def broadcastTables: Seq[String] = sqlContext.joinBroadcastTables.split(",").toBuffer
-
def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
--- End diff --
I agree. I'd like to add 2 more concerns :)
* zero maybe means the zero record of an input table, we probably need to take some other default value, or like Hive does, to add an new option like "set hive.auto.convert.join=false;"
* size estimates interface is very helpful for cost based join optimization (join reordering), besides the BroadcastNestedLoopJoin, we can also use that for equi-inner-join(we always prefer the bigger table as streaming table and the smaller one as build table). just make sure you didn't misunderstanding what I suggested in the top comment of this thread.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14676932
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,17 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
--- End diff --
- I think `Statistics` might be a better name than `Estimates`. This is because eventually I think this class will also contain things like per-column information like # of distinct elements, or even histograms.
- What do you think about making this a case class? I think there are two advantages here over the current scheme:
- Two levels of lazyness might be overkill.
- With case classes, propagation could be done using a copy constructor, changing only the required values. I think this might become more important as we add more fields to this class.
- It would prevent a proliferation of anonymous classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14901837
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,28 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ // TODO: handle overflow?
+ /**
+ * Estimates of various statistics. The default estimation logic simply sums up the corresponding
+ * statistic produced by the children. To override this behavior, override `statistics` and
+ * assign it a overriden version of `Statistics`.
+ */
+ case class Statistics(
+ /**
+ * Number of output tuples. For leaf operators this defaults to 1, otherwise it is set to the
--- End diff --
These should probably be `@param` in the scala doc above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49210466
Jenkins, test this please.
I think I have addressed the latest round of review comments, where the biggest changes being:
- Remove statistics estimates from ParquetRelation for now due to performance considerations.
- When estimating sizes for MetastoreRelation, instead of making potentially expensive Hadoop FileSystem calls, we peek into the Hive Metastore for populated information.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14544542
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala ---
@@ -61,8 +61,6 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
condition.map(Filter(_, broadcastHashJoin)).getOrElse(broadcastHashJoin) :: Nil
}
- def broadcastTables: Seq[String] = sqlContext.joinBroadcastTables.split(",").toBuffer
-
def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
--- End diff --
I think this probably has 2 rules for size estimated JOIN if I understand correctly:
* if size estimates less than a threshold, we can convert it into a BroadcastNestedLoopJoin
* In equi-join(InnerJoin), if the left estimates size less than right, we should `BuildLeft`, otherwise `BuildRight`
We have the first implemented, but we probably need second also. And should we add an option to disable the size estimates based strategy?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48530251
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16477/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14902100
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,28 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ // TODO: handle overflow?
+ /**
+ * Estimates of various statistics. The default estimation logic simply sums up the corresponding
--- End diff --
This comment says `sum`, but the implementation below looks like `product`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15492240
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -39,29 +43,34 @@ trait SQLConf {
/**
* Upper bound on the sizes (in bytes) of the tables qualified for the auto conversion to
- * a broadcast value during the physical executions of join operations. Setting this to 0
+ * a broadcast value during the physical executions of join operations. Setting this to -1
* effectively disables auto conversion.
- * Hive setting: hive.auto.convert.join.noconditionaltask.size.
+ *
+ * Hive setting: hive.auto.convert.join.noconditionaltask.size, whose default value is also 10000.
*/
private[spark] def autoConvertJoinSize: Int =
get("spark.sql.auto.convert.join.size", "10000").toInt
- /** A comma-separated list of table names marked to be broadcasted during joins. */
- private[spark] def joinBroadcastTables: String = get("spark.sql.join.broadcastTables", "")
+ /**
+ * The default size in bytes to assign to a logical operator's estimation statistics. By default,
+ * it is set to a larger value than `autoConvertJoinSize`, hence any logical operator without a
+ * properly implemented estimation of this statistic will not be incorrectly broadcasted in joins.
+ */
+ private[spark] def statsDefaultSizeInBytes: Long =
+ getOption("spark.sql.catalyst.stats.sizeInBytes").map(_.toLong)
--- End diff --
`defaultSizeInBytes`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15206160
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala ---
@@ -66,8 +66,8 @@ abstract class SparkPlan extends QueryPlan[SparkPlan] with Logging {
* linking.
*/
@DeveloperApi
-case class SparkLogicalPlan(alreadyPlanned: SparkPlan, tableName: String = "SparkLogicalPlan")
- extends BaseRelation with MultiInstanceRelation {
+case class SparkLogicalPlan(alreadyPlanned: SparkPlan)(@transient sqlContext: SQLContext)
--- End diff --
FYI: I'm considering adding a way for all physical plans to automatically have a handle to the sqlContext. This will probably happen in the code gen patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-50522265
Rebased & addressed review comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48540226
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14737891
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala ---
@@ -61,8 +61,6 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
condition.map(Filter(_, broadcastHashJoin)).getOrElse(broadcastHashJoin) :: Nil
}
- def broadcastTables: Seq[String] = sqlContext.joinBroadcastTables.split(",").toBuffer
-
def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
--- End diff --
@chenghao-intel @chhao01 @marmbrus I added the optimization for both ShuffledHashJoin (in HashJoin) and BroadcastNestedLoopJoin.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48636336
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1238
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14581868
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,17 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
+ lazy val sizeInBytes: Long = childrenEstimations.map(_.sizeInBytes).sum
--- End diff --
@concretevitamin maybe there is some misunderstanding, I mean we should set a bigger default value for sizeInBytes, like Long.MAX_VALUE or something? 0 is actually a meaningful value, like for an empty table.
BTW, I don't see where we use the `cardinality` of `Estimates`, is it for the future?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48530035
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14544706
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,17 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
+ lazy val sizeInBytes: Long = childrenEstimations.map(_.sizeInBytes).sum
--- End diff --
What if people register the existed RDD as table input, not from Hive Table or Parquet file, should we set a bigger default value?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15492206
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -39,29 +43,34 @@ trait SQLConf {
/**
* Upper bound on the sizes (in bytes) of the tables qualified for the auto conversion to
- * a broadcast value during the physical executions of join operations. Setting this to 0
+ * a broadcast value during the physical executions of join operations. Setting this to -1
* effectively disables auto conversion.
- * Hive setting: hive.auto.convert.join.noconditionaltask.size.
+ *
+ * Hive setting: hive.auto.convert.join.noconditionaltask.size, whose default value is also 10000.
*/
private[spark] def autoConvertJoinSize: Int =
get("spark.sql.auto.convert.join.size", "10000").toInt
--- End diff --
What do you think about avoiding `.`s when there isn't hierarchy. So maybe `spark.sql.autoBroadcastJoinThreshold`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14857013
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -21,16 +21,27 @@ import java.util.Properties
import scala.collection.JavaConverters._
+private object SQLConf {
+ @transient protected[spark] val confSettings = java.util.Collections.synchronizedMap(
--- End diff --
Why singleton?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15243796
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,26 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ // TODO: handle overflow?
--- End diff --
BigInt somehow completely eluded me before. Let me try to make the changes now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14273776
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -258,6 +263,19 @@ private[hive] case class MetastoreRelation
new Partition(hiveQlTable, p)
}
+ // TODO: are there any stats in hiveQlTable.getSkewedInfo that we can use?
+ @transient override lazy val estimates = new Estimates {
+ // Size getters adapted from
+ // https://github.com/apache/hive/blob/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/SizeBasedBigTableSelectorForAutoSMJ.java
--- End diff --
Maybe just say "See SizeBasedBigTableSelectorForAutoSMJ.java in Hive (version 0.13)"
The github link could disappear in the future when that file gets moved.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14857017
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -21,16 +21,27 @@ import java.util.Properties
import scala.collection.JavaConverters._
+private object SQLConf {
+ @transient protected[spark] val confSettings = java.util.Collections.synchronizedMap(
--- End diff --
In one of the earlier commits, I tried the approach of making an operator mixing in SQLConf -- hence that necessitated a singleton object to hold the settings. Now I just have that operator take a SQLContext in order to get access to the conf, so if you want I could remove this singleton.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47993459
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49785200
I have addressed the latest round of review comments & rebased onto latest master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14738387
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,17 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
+ lazy val sizeInBytes: Long = childrenEstimations.map(_.sizeInBytes).sum
--- End diff --
@chenghao-intel Yes, `cardinality` (now renamed to `numTuples`) is not used in this PR and any optimizations utilizing it are intended for future work.
@marmbrus I don't understand the argument of using 1 for cardinality (numTuples) here? It seems to me in either an inner join or an outer join, if one side has numTuples equal to 0, the summation still provides a correct overestimate. Additionally, numTuples is not used in this patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47998295
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16321/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-49219898
QA results for PR 1238:<br>- This patch FAILED unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16743/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48547545
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16481/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47291825
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47996391
This is no longer WIP -- tests added & previous review comments addressed.
+@marmbrus let me know what you think is missing that should also go into this first draft, thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47993162
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15206167
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -250,7 +251,11 @@ object HiveMetastoreTypes extends RegexParsers {
private[hive] case class MetastoreRelation
(databaseName: String, tableName: String, alias: Option[String])
(val table: TTable, val partitions: Seq[TPartition])
- extends BaseRelation {
+ (@transient hiveConf: HiveConf, @transient path: Path)
--- End diff --
Are we still using the hiveConf and path?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48630836
@rxin sure, done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48636351
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47292447
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16173/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48941686
QA tests have started for PR 1238. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16632/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] Prototype implementation of size estimat...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14573147
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,17 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
+ lazy val sizeInBytes: Long = childrenEstimations.map(_.sizeInBytes).sum
--- End diff --
We should definitely decide how much of the estimates framework we'd like to implement in this PR. Existing RDDs do not have meaningful estimates in the current version of this patch.
As for what default value to use, I think 10000 is what Hive is using (see [here](https://cwiki.apache.org/confluence/display/Hive/LanguageManual+JoinOptimization)).
Lastly, we should also think about the default Estimates: currently it is a sum of children's estimates, but for joins we'd like it to be a product -- yet we have to be careful when 0 appears in a child's estimate.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14739228
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,17 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
+ lazy val sizeInBytes: Long = childrenEstimations.map(_.sizeInBytes).sum
--- End diff --
Yeah, but the sum could also wildly underestimate compared with a product for many join cases. The 1 is only to prevent anything * 0 = 0 happening for outer joins.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14902287
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveMetastoreCatalog.scala ---
@@ -263,6 +268,19 @@ private[hive] case class MetastoreRelation
new Partition(hiveQlTable, p)
}
+ // TODO: are there any stats in hiveQlTable.getSkewedInfo that we can use?
+ @transient override lazy val statistics = new Statistics {
--- End diff --
Does that need to be in the class itself?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14683319
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,18 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ protected class Estimates {
+ lazy val childrenEstimations = children.map(_.estimates)
+ lazy val cardinality: Long = childrenEstimations.map(_.cardinality).sum
--- End diff --
We should rename cardinality because it can be ambiguous in databases. In some context cardinality means "numTuples", and in other cases people use it for "number of distinct values".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48577069
If this is no longer WIP, can you change the title to make it more descriptive? At the very least I wouldn't include the word "Prototype".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SQL] [WIP] Prototype implementation of size e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-47292122
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Prototype implementation of ...
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48570761
I have addressed most of the review comments & rebased. Please review again & take a look at the TODOs.
It might be better to leave the optimization for BroadcastNestedLoopJoin for a future PR (although I have some WIP locally), as it requires a couple changes to the class (mainly to support a BuildSide). This is the reason the tests were failing before.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r15020066
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,28 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ // TODO: handle overflow?
+ /**
+ * Estimates of various statistics. The default estimation logic simply sums up the corresponding
+ * statistic produced by the children. To override this behavior, override `statistics` and
+ * assign it a overriden version of `Statistics`.
+ */
+ case class Statistics(
+ /**
+ * Number of output tuples. For leaf operators this defaults to 1, otherwise it is set to the
+ * product of children's `numTuples`.
+ */
+ numTuples: Long = childrenStats.map(_.numTuples).product,
+
+ /**
+ * Physical size in bytes. For leaf operators this defaults to 1, otherwise it is set to the
+ * product of children's `sizeInBytes`.
+ */
+ sizeInBytes: Long = childrenStats.map(_.sizeInBytes).product
+ )
+ lazy val statistics: Statistics = new Statistics
+ lazy val childrenStats = children.map(_.statistics)
--- End diff --
I was suggesting we turn it into a def, remove it entirely, or stop memoizing statistics. Double memoization seems unnecessarily expensive memory wise.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1238#issuecomment-48636504
QA tests have started for PR 1238. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16505/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2393][SQL] Cost estimation optimization...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/1238#discussion_r14902549
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -26,6 +26,28 @@ import org.apache.spark.sql.catalyst.trees
abstract class LogicalPlan extends QueryPlan[LogicalPlan] {
self: Product =>
+ // TODO: handle overflow?
+ /**
+ * Estimates of various statistics. The default estimation logic simply sums up the corresponding
+ * statistic produced by the children. To override this behavior, override `statistics` and
+ * assign it a overriden version of `Statistics`.
+ */
+ case class Statistics(
+ /**
+ * Number of output tuples. For leaf operators this defaults to 1, otherwise it is set to the
+ * product of children's `numTuples`.
+ */
+ numTuples: Long = childrenStats.map(_.numTuples).product,
--- End diff --
perhaps we should consider removing this since right now the values it returns will always be garbage.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---