[WIP][Spark-SQL] Optimize the Constant Folding for Expression #482

chenghao-intel · 2014-04-22T08:54:50Z

Currently, expression does not support the "constant null" well in constant folding.
e.g. Sum(a, 0) actually always produces Literal(0, NumericType) in runtime.

For example:

explain select isnull(key+null)  from src;
== Logical Plan ==
Project [HiveGenericUdf#isnull((key#30 + CAST(null, IntegerType))) AS c_0#28]
 MetastoreRelation default, src, None

== Optimized Logical Plan ==
Project [true AS c_0#28]
 MetastoreRelation default, src, None

== Physical Plan ==
Project [true AS c_0#28]
 HiveTableScan [], (MetastoreRelation default, src, None), None

I've create a new Optimization rule called NullPropagation for such kind of constant folding.

AmplabJenkins · 2014-04-22T08:57:55Z

Can one of the admins verify this patch?

AndreSchumacher · 2014-04-23T19:22:41Z

Jenkins, test this please.

AmplabJenkins · 2014-04-23T19:22:58Z

Merged build triggered.

AmplabJenkins · 2014-04-23T19:23:06Z

Merged build started.

AmplabJenkins · 2014-04-23T19:24:37Z

Merged build finished.

AmplabJenkins · 2014-04-23T19:24:37Z

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14385/

mateiz · 2014-04-23T22:43:01Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

+// always be null
+object alwaysNull extends Nullability(-1)
+// never be null
+object neverNull extends Nullability(1)


As a naming issue, objects like this should be capitalized (PossibleNull, AlwaysNull, NeverNull). It might also be good to rename PossibleNull to PossiblyNull.

marmbrus · 2014-04-23T23:37:45Z

I'm not sure if we want to overload the nullability interface in this way. This nullability bit currently is mostly intended for use when determining storage layout of tuples (i.e. do we need to keep a null bit around or can we operate on primitives). What is going on here is something like constraint propagation + null domination, which we might be able to accomplish more generally.

Also, I'm not sure if its a good idea to provide a default implementation of nullability as there are quite a few operators whose nullability isn't defined by their children (many aggregate operators for example).

Perhaps there is a simpler way to do this using an improved rule in the optimizer.

chenghao-intel · 2014-04-24T01:05:41Z

@marmbrus I will review the nullability for aggregate operators etc. and it also may could be folded by statically analyze the nullability. (for example "sum(a)" but "a" could be an expression "Always Be Null").

I was thinking not to extend the original nullability of expression, however only the "Never Be Null" and "Always Be Null" will be helpful in constant folding rule, but the boolean value of nullability is only represented as "Never Be Null" or "Possibly Be Null".

I think we need to find some other way to show "Always Be Null" state for expression if we don't want to extend the nullability.

chenghao-intel · 2014-04-24T02:03:25Z

I have 2 options for the changes:
Option 1) is add an new method in expression, which tells the optimize rule, what it produces if part of its arguments are "always be null" / "possibly be null" / "never be null". the rule may test every expressions.
Option 2) the rule itself enumerates all of the expressions, and understand what if part of the arguments is "always be null" etc., like the BooleanSimplification does.

But still seems the "constraint propagation" is the best. Not sure if you have better idea on this.

marmbrus · 2014-04-24T02:12:28Z

I like the option of doing it similar to BooleanSimplification. That seems nice and explicit. I also think this rule will play nicely with more general constraint propagation when we add that in the future.

chenghao-intel · 2014-04-28T06:14:35Z

@marmbrus , can you please review this again? I've rewritten the code as rule-based.

marmbrus · 2014-04-28T06:36:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

      // Skip redundant folding of literals.
      case l: Literal => l
+      // if it's foldable


I think we can omit this comment since the line beneath reads if e.foldable.

marmbrus · 2014-04-28T06:53:04Z

Thanks for working on this! I think the rule based version is a lot clearer.

chenghao-intel · 2014-04-28T08:36:36Z

Thank you @marmbrus , I've updated the code a little bit.

I've created a new rule "NullPropagation", it transforms the expression tree from bottom to top, and keep unchanged for the rule constant folding.

marmbrus · 2014-04-28T17:48:58Z

test this please

AmplabJenkins · 2014-04-28T17:52:57Z

Merged build triggered.

AmplabJenkins · 2014-04-28T17:53:03Z

Merged build started.

AmplabJenkins · 2014-04-28T17:54:26Z

Merged build finished.

AmplabJenkins · 2014-04-28T17:54:27Z

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14534/

marmbrus · 2014-04-28T17:55:32Z

Can you please fix the style issues.

AmplabJenkins · 2014-05-04T01:33:15Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-05-04T01:33:15Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14637/

marmbrus · 2014-05-04T18:08:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala

@@ -114,37 +114,37 @@ package object dsl {
      def attr = analysis.UnresolvedAttribute(s)

      /** Creates a new AttributeReference of type boolean */
-      def boolean = AttributeReference(s, BooleanType, nullable = false)()


What is the rationale for changing all of these?

I think "def boolean" / "def string" should return an Attribute with nullability = true, that does not harm for the correctness, otherwise, it may bring a wrong hint for the optimization of rule NullPropagation.

For example:

val row = new GenericRow(Array[Any] ("a", null)) val c1 = 'a.string.at(0) val c2 = 'b.string.at(1) // nullable should be true, otherwise does't reflect the real situation. assert(evaluate(IsNull(c2), row) == true)

Good point.

chenghao-intel · 2014-05-05T01:06:05Z

@marmbrus I have updated the code as your comment, can you review/retest it?

marmbrus · 2014-05-05T01:15:53Z

test this please

AmplabJenkins · 2014-05-05T01:17:59Z

Merged build triggered.

AmplabJenkins · 2014-05-05T01:18:07Z

Merged build started.

AmplabJenkins · 2014-05-05T02:33:25Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-05-05T02:33:25Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14652/

marmbrus · 2014-05-05T18:40:41Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypes.scala

-        baseValue(o)
-      }
+    val value = child.eval(input)
+    if(value == null) {


Space after if, below too.

marmbrus · 2014-05-05T18:41:42Z

Other than a very minor style issue, this LGTM.

@pwendell or @rxin please merge when ready.

chenghao-intel · 2014-05-06T00:41:26Z

Fixed. Can you retest this please?

marmbrus · 2014-05-06T00:41:54Z

test this please.

AmplabJenkins · 2014-05-06T00:42:58Z

Merged build triggered.

AmplabJenkins · 2014-05-06T00:43:06Z

Merged build started.

AmplabJenkins · 2014-05-06T01:51:08Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-05-06T01:51:09Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14692/

marmbrus · 2014-05-07T00:43:59Z

Thanks for doing this :)

@pwendell, can you please merge?

rxin · 2014-05-07T07:36:59Z

I've merged this. Thanks!

Currently, expression does not support the "constant null" well in constant folding. e.g. Sum(a, 0) actually always produces Literal(0, NumericType) in runtime. For example: ``` explain select isnull(key+null) from src; == Logical Plan == Project [HiveGenericUdf#isnull((key#30 + CAST(null, IntegerType))) AS c_0#28] MetastoreRelation default, src, None == Optimized Logical Plan == Project [true AS c_0#28] MetastoreRelation default, src, None == Physical Plan == Project [true AS c_0#28] HiveTableScan [], (MetastoreRelation default, src, None), None ``` I've create a new Optimization rule called NullPropagation for such kind of constant folding. Author: Cheng Hao <hao.cheng@intel.com> Author: Michael Armbrust <michael@databricks.com> Closes #482 from chenghao-intel/optimize_constant_folding and squashes the following commits: 2f14b50 [Cheng Hao] Fix code style issues 68b9fad [Cheng Hao] Remove the Literal pattern matching for NullPropagation 29c8166 [Cheng Hao] Update the code for feedback of code review 50444cc [Cheng Hao] Remove the unnecessary null checking 80f9f18 [Cheng Hao] Update the UnitTest for aggregation constant folding 27ea3d7 [Cheng Hao] Fix Constant Folding Bugs & Add More Unittests b28e03a [Cheng Hao] Merge pull request #1 from marmbrus/pr/482 9ccefdb [Michael Armbrust] Add tests for optimized expression evaluation. 543ef9d [Cheng Hao] fix code style issues 9cf0396 [Cheng Hao] update code according to the code review comment 536c005 [Cheng Hao] Add Exceptional case for constant folding 3c045c7 [Cheng Hao] Optimize the Constant Folding by adding more rules 2645d4f [Cheng Hao] Constant Folding(null propagation) (cherry picked from commit 3eb53bd) Signed-off-by: Reynold Xin <rxin@apache.org>

Added StreamingContext.awaitTermination to streaming examples StreamingContext.start() currently starts a non-daemon thread which prevents termination of a Spark Streaming program even if main function has exited. Since the expected behavior of a streaming program is to run until explicitly killed, this was sort of fine when spark streaming applications are launched from the command line. However, when launched in Yarn-standalone mode, this did not work as the driver effectively got terminated when the main function exits. So SparkStreaming examples did not work on Yarn. This addition to the examples ensures that the examples work on Yarn and also ensures that everyone learns that StreamingContext.awaitTermination() being necessary for SparkStreaming programs to wait. The true bug-fix of making sure all threads by Spark Streaming are daemon threads is left for post-0.9.

Currently, expression does not support the "constant null" well in constant folding. e.g. Sum(a, 0) actually always produces Literal(0, NumericType) in runtime. For example: ``` explain select isnull(key+null) from src; == Logical Plan == Project [HiveGenericUdf#isnull((key#30 + CAST(null, IntegerType))) AS c_0#28] MetastoreRelation default, src, None == Optimized Logical Plan == Project [true AS c_0#28] MetastoreRelation default, src, None == Physical Plan == Project [true AS c_0#28] HiveTableScan [], (MetastoreRelation default, src, None), None ``` I've create a new Optimization rule called NullPropagation for such kind of constant folding. Author: Cheng Hao <hao.cheng@intel.com> Author: Michael Armbrust <michael@databricks.com> Closes apache#482 from chenghao-intel/optimize_constant_folding and squashes the following commits: 2f14b50 [Cheng Hao] Fix code style issues 68b9fad [Cheng Hao] Remove the Literal pattern matching for NullPropagation 29c8166 [Cheng Hao] Update the code for feedback of code review 50444cc [Cheng Hao] Remove the unnecessary null checking 80f9f18 [Cheng Hao] Update the UnitTest for aggregation constant folding 27ea3d7 [Cheng Hao] Fix Constant Folding Bugs & Add More Unittests b28e03a [Cheng Hao] Merge pull request apache#1 from marmbrus/pr/482 9ccefdb [Michael Armbrust] Add tests for optimized expression evaluation. 543ef9d [Cheng Hao] fix code style issues 9cf0396 [Cheng Hao] update code according to the code review comment 536c005 [Cheng Hao] Add Exceptional case for constant folding 3c045c7 [Cheng Hao] Optimize the Constant Folding by adding more rules 2645d4f [Cheng Hao] Constant Folding(null propagation)

Added StreamingContext.awaitTermination to streaming examples StreamingContext.start() currently starts a non-daemon thread which prevents termination of a Spark Streaming program even if main function has exited. Since the expected behavior of a streaming program is to run until explicitly killed, this was sort of fine when spark streaming applications are launched from the command line. However, when launched in Yarn-standalone mode, this did not work as the driver effectively got terminated when the main function exits. So SparkStreaming examples did not work on Yarn. This addition to the examples ensures that the examples work on Yarn and also ensures that everyone learns that StreamingContext.awaitTermination() being necessary for SparkStreaming programs to wait. The true bug-fix of making sure all threads by Spark Streaming are daemon threads is left for post-0.9. (cherry picked from commit 0367981) Signed-off-by: Patrick Wendell <pwendell@gmail.com>

AndreSchumacher mentioned this pull request Apr 23, 2014

SPARK-1487 [SQL] Support record filtering via predicate pushdown in Parquet #511

Closed

mateiz reviewed Apr 23, 2014
View reviewed changes

chenghao-intel added 3 commits April 28, 2014 10:45

Constant Folding(null propagation)

2645d4f

Optimize the Constant Folding by adding more rules

3c045c7

Add Exceptional case for constant folding

536c005

marmbrus reviewed Apr 28, 2014
View reviewed changes

update code according to the code review comment

9cf0396

marmbrus reviewed May 4, 2014
View reviewed changes

Update the code for feedback of code review

29c8166

Remove the Literal pattern matching for NullPropagation

68b9fad

marmbrus reviewed May 5, 2014
View reviewed changes

Fix code style issues

2f14b50

asfgit closed this in 3eb53bd May 7, 2014

chenghao-intel deleted the optimize_constant_folding branch May 28, 2014 02:36

[WIP][Spark-SQL] Optimize the Constant Folding for Expression #482

[WIP][Spark-SQL] Optimize the Constant Folding for Expression #482

Conversation

chenghao-intel commented Apr 22, 2014

AmplabJenkins commented Apr 22, 2014

AndreSchumacher commented Apr 23, 2014

AmplabJenkins commented Apr 23, 2014

AmplabJenkins commented Apr 23, 2014

AmplabJenkins commented Apr 23, 2014

AmplabJenkins commented Apr 23, 2014

mateiz Apr 23, 2014

Choose a reason for hiding this comment

marmbrus commented Apr 23, 2014

chenghao-intel commented Apr 24, 2014

chenghao-intel commented Apr 24, 2014

marmbrus commented Apr 24, 2014

chenghao-intel commented Apr 28, 2014

marmbrus Apr 28, 2014

Choose a reason for hiding this comment

marmbrus commented Apr 28, 2014

chenghao-intel commented Apr 28, 2014

marmbrus commented Apr 28, 2014

AmplabJenkins commented Apr 28, 2014

AmplabJenkins commented Apr 28, 2014

AmplabJenkins commented Apr 28, 2014

AmplabJenkins commented Apr 28, 2014

marmbrus commented Apr 28, 2014

AmplabJenkins commented May 4, 2014

AmplabJenkins commented May 4, 2014

marmbrus May 4, 2014

Choose a reason for hiding this comment

chenghao-intel May 5, 2014

Choose a reason for hiding this comment

marmbrus May 5, 2014

Choose a reason for hiding this comment

chenghao-intel commented May 5, 2014

marmbrus commented May 5, 2014

AmplabJenkins commented May 5, 2014

AmplabJenkins commented May 5, 2014

AmplabJenkins commented May 5, 2014

AmplabJenkins commented May 5, 2014

marmbrus May 5, 2014

Choose a reason for hiding this comment

marmbrus commented May 5, 2014

chenghao-intel commented May 6, 2014

marmbrus commented May 6, 2014

AmplabJenkins commented May 6, 2014

AmplabJenkins commented May 6, 2014

AmplabJenkins commented May 6, 2014

AmplabJenkins commented May 6, 2014

marmbrus commented May 7, 2014

rxin commented May 7, 2014