[SQL] SPARK-6981: Factor out SparkPlanner and QueryExecution from SQLContext #6356

evacchi · 2015-05-22T13:53:24Z

Alternative to PR #6122; in this case the refactored out classes are replaced by inner classes with the same name for backwards binary compatibility

process in a lighter-weight, backwards-compatible way

* process in a lighter-weight, backwards-compatible way

rxin · 2015-05-22T18:19:53Z

I don't think we need to care about the binary compatibility for this case. Maybe just add the exclude rules to MimaExcludes file.

rxin · 2015-05-22T18:21:33Z

sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala

+}
+
+
+class SparkPlanner(val sqlContext: SQLContext) extends org.apache.spark.sql.execution.SparkStrategies {


can we move this into the execution package?

chenghao-intel · 2015-05-25T05:17:07Z

In order to make a minimal change (To keep the HiveContext / TestHive unchanged), keeping the nested classes QueryExcution and SparkPlanner seems a good idea for me, Or as @rxin suggested, we should do all of change in a single PR.
Thoughts?

evacchi · 2015-05-25T07:29:17Z

@chenghao-intel to me both solutions are fine, see PR #6122; however, this solution would retain binary compatibility

rxin · 2015-07-18T19:24:16Z

@evacchi mind bringing this up to date so we can merge it for 1.5?

evacchi · 2015-07-20T07:31:00Z

ok, will check today

evacchi · 2015-07-20T11:41:53Z

@rxin merge conflicts fixed; should pass tests as well

SparkQA · 2015-07-20T16:43:44Z

Test build #1123 has finished for PR 6356 at commit b01244b.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[sql] class SparkPlanner extends sparkexecution.SparkPlanner(this)
- protected[sql] class QueryExecution(logical: LogicalPlan) extends sparkexecution.QueryExecution(this, logical)
- class QueryExecution(val sqlContext: SQLContext, val logical: LogicalPlan)
- class ShuffledRowRDD(
- class SparkPlanner(val sqlContext: SQLContext) extends org.apache.spark.sql.execution.SparkStrategies

rxin · 2015-07-20T18:08:49Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlanner.scala

+      Project(projectList, filterCondition.map(Filter(_, scan)).getOrElse(scan))
+    }
+  }
+}


i think u need to add a new line here to not fail scalastyle

there might be more so u should check the output from jenkins

evacchi · 2015-07-21T07:47:05Z

@rxin weird, didn't get those during local testing. BTW, they should pass now

rxin · 2015-07-21T07:48:46Z

Jenkins, test this please.

SparkQA · 2015-07-21T10:06:30Z

Test build #1146 has finished for PR 6356 at commit 253c15e.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- protected[sql] class SparkPlanner extends sparkexecution.SparkPlanner(this)
- protected[sql] class QueryExecution(logical: LogicalPlan)
- class QueryExecution(val sqlContext: SQLContext, val logical: LogicalPlan)
- class SparkPlanner(val sqlContext: SQLContext) extends SparkStrategies

evacchi · 2015-07-23T07:39:52Z

@rxin I fixed today's merge conflicts; I would advise doing a merged build once again and then (if, of course, the patch is good enough) to go ahead and merge it ASAP. Since this is a refactoring it will obviously break every day until it will be merged ! :)

rxin · 2015-07-23T07:41:41Z

Jenkins, test this please.

evacchi · 2015-07-24T08:00:45Z

test build has not started

SparkQA · 2015-07-24T08:13:07Z

Test build #1194 has finished for PR 6356 at commit 0089435.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[sql] class SparkPlanner extends sparkexecution.SparkPlanner(this)
- protected[sql] class QueryExecution(logical: LogicalPlan)
- class QueryExecution(val sqlContext: SQLContext, val logical: LogicalPlan)
- class SparkPlanner(val sqlContext: SQLContext) extends SparkStrategies

evacchi · 2015-07-24T14:30:19Z

tests should pass now

marmbrus · 2015-09-03T20:50:55Z

@evacchi, sorry for letting this go stale again. Can we close this issue until you have time to bring it up to date?

Conflicts: sql/core/src/main/scala/org/apache/spark/sql/DataFrame.scala sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala

evacchi · 2015-09-04T12:02:31Z

@marmbrus I have brought this up to date. Might need fixes in order to merge cleanly, though

marmbrus · 2015-09-08T21:53:42Z

ok to test

marmbrus · 2015-09-08T22:03:43Z

sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala

-      }
-    }
-  }
+  @deprecated("use org.apache.spark.sql.SparkPlanner", "1.5.0")


marmbrus · 2015-09-08T22:07:45Z

Minor comments, otherwise LGTM

SparkQA · 2015-09-09T00:06:15Z

Test build #42156 has finished for PR 6356 at commit 3bf710b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[sql] class SparkPlanner extends sparkexecution.SparkPlanner(this)
- protected[sql] class QueryExecution(logical: LogicalPlan)
- class QueryExecution(val sqlContext: SQLContext, val logical: LogicalPlan)
- class SparkPlanner(val sqlContext: SQLContext) extends SparkStrategies

evacchi · 2015-09-10T07:04:22Z

Hope this still merges cleanly, I'm writing this from my phone. Otherwise it'll have to wait till I'm back from holidays

SparkQA · 2015-09-10T07:16:38Z

Test build #42246 has finished for PR 6356 at commit 47685cc.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[sql] class SparkPlanner extends sparkexecution.SparkPlanner(this)
- protected[sql] class QueryExecution(logical: LogicalPlan)
- class QueryExecution(val sqlContext: SQLContext, val logical: LogicalPlan)
- class SparkPlanner(val sqlContext: SQLContext) extends SparkStrategies

evacchi · 2015-09-11T16:24:25Z

Rerun tests?

marmbrus · 2015-09-11T21:49:24Z

ok to test

SparkQA · 2015-09-11T23:57:07Z

Test build #1746 has finished for PR 6356 at commit 22a01ae.

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

SparkQA · 2015-09-12T00:10:25Z

Test build #42359 has finished for PR 6356 at commit 22a01ae.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[sql] class SparkPlanner extends sparkexecution.SparkPlanner(this)
- protected[sql] class QueryExecution(logical: LogicalPlan)
- class QueryExecution(val sqlContext: SQLContext, val logical: LogicalPlan)
- class SparkPlanner(val sqlContext: SQLContext) extends SparkStrategies

marmbrus · 2015-09-14T21:56:01Z

Thanks, merging to master!

yhuai · 2015-10-03T01:06:47Z

sql/core/src/main/scala/org/apache/spark/sql/DataFrame.scala

@@ -114,7 +114,7 @@ private[sql] object DataFrame {
 @Experimental
 class DataFrame private[sql](
    @transient val sqlContext: SQLContext,
-    @DeveloperApi @transient val queryExecution: SQLContext#QueryExecution) extends Serializable {
+    @DeveloperApi @transient val queryExecution: QueryExecution) extends Serializable {


@marmbrus This changes the signature of queryExecution. Is there anyway that we can preserve the binary compatibility?

Should we preserve binary compatibility for developer API? At least MIMA doesn't complain about it...

QueryExecution extends SQLContext#QueryExecution so binary compatibility should be preserved

The signature of the function to retrieve it has changed though. I think that we should rename this parameter and add a method that constructs a new instance of the inner subclass to return with the name queryExecution. I think its worth rerunning the optimizer to not have to recompile all our perf tools.

Refactor out QueryExecution, SparkPlanner

b087770

* process in a lighter-weight, backwards-compatible way

evacchi mentioned this pull request May 22, 2015

[SQL] SPARK-6981: Factor out SparkPlanner and QueryExecution from SQLContext #6122

Closed

rxin reviewed May 22, 2015
View reviewed changes

Move QueryExecution, SparkPlanner to execution package

c22aa54

Ported to Spark 1.5

b01244b

rxin reviewed Jul 20, 2015
View reviewed changes

Fix scala style

253c15e

Fix merge conflicts

0089435

SparkPlanner: Re-align imports to master

24819e5

Merge branch 'master' into sqlctx-refactoring-lite

3bf710b

Conflicts: sql/core/src/main/scala/org/apache/spark/sql/DataFrame.scala sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala

marmbrus reviewed Sep 8, 2015
View reviewed changes

Update SQLContext.scala

47685cc

Scala style

22a01ae

asfgit closed this in 64f0415 Sep 14, 2015

yhuai reviewed Oct 3, 2015
View reviewed changes

		}


		class SparkPlanner(val sqlContext: SQLContext) extends org.apache.spark.sql.execution.SparkStrategies {

[SQL] SPARK-6981: Factor out SparkPlanner and QueryExecution from SQLContext #6356

[SQL] SPARK-6981: Factor out SparkPlanner and QueryExecution from SQLContext #6356

Conversation

evacchi commented May 22, 2015

rxin commented May 22, 2015

Choose a reason for hiding this comment

chenghao-intel commented May 25, 2015

evacchi commented May 25, 2015

rxin commented Jul 18, 2015

evacchi commented Jul 20, 2015

evacchi commented Jul 20, 2015

SparkQA commented Jul 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evacchi commented Jul 21, 2015

rxin commented Jul 21, 2015

SparkQA commented Jul 21, 2015

evacchi commented Jul 23, 2015

rxin commented Jul 23, 2015

evacchi commented Jul 24, 2015

SparkQA commented Jul 24, 2015

evacchi commented Jul 24, 2015

marmbrus commented Sep 3, 2015

evacchi commented Sep 4, 2015

marmbrus commented Sep 8, 2015

Choose a reason for hiding this comment

marmbrus commented Sep 8, 2015

SparkQA commented Sep 9, 2015

evacchi commented Sep 10, 2015

SparkQA commented Sep 10, 2015

evacchi commented Sep 11, 2015

marmbrus commented Sep 11, 2015

SparkQA commented Sep 11, 2015

SparkQA commented Sep 12, 2015

marmbrus commented Sep 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment