[Spark-1461] Deferred Expression Evaluation (short-circuit evaluation) #446

chenghao-intel · 2014-04-18T06:38:46Z

This patch unify the foldable & nullable interface for Expression.

Deterministic-less UDF (like Rand()) can not be folded.
Short-circut will significantly improves the performance in Expression Evaluation, however, the stateful UDF should not be ignored in a short-circuit evaluation(e.g. in expression: col1 > 0 and row_sequence() < 1000, row_sequence() can not be ignored even if col1 > 0 is false)

I brought an concept of DeferredObject from Hive, which has 2 kinds of children classes (EagerResult / DeferredResult), the former requires triggering the evaluation before it's created, while the later trigger the evaluation when first called its get() method.

AmplabJenkins · 2014-04-18T06:43:11Z

Can one of the admins verify this patch?

marmbrus · 2014-04-18T23:52:21Z

A few high-level comments:

I'm not sure if stateful UDFs are actually something we want to support. The semantics for them are not well defined in partitioned systems, especially where the optimizer decides the partitioning. If you want things like row id there are already ways to do this with map partitions with index.
The deferred evaluation class seems like a complicated way to get short circuit evaluation. In a lot of cases can't we just change the ordering of calling the existing eval method? Adding a new interface complicates things, and in some simple benchmarks that I ran this code is actually slower than what was there before (probably because of the extra object allocations).
There are a lot of unrelated changes here also. While fixing a minor spelling error or something is okay, making a whole bunch of unrelated changes makes reviewing the PR more difficult for us. For example, maybe you can do the data type additions for Hive UDFs in their own PR.

chenghao-intel · 2014-04-19T03:29:42Z

Thank you @marmbrus , if we are not planning to support the stateful UDFs, the deferred evaluation can be removed, too, and we can just change the ordering of calling the eval method for the existed expressions for short-circuit evaluation.

chenghao-intel · 2014-04-21T04:49:16Z

@marmbrus , I've removed the unrelated changes from this PR, and this PR only for the deferred expression evaluation.

marmbrus · 2014-04-21T16:32:07Z

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

@@ -146,7 +158,7 @@ case class If(predicate: Expression, trueValue: Expression, falseValue: Expressi
    extends Expression {

  def children = predicate :: trueValue :: falseValue :: Nil
-  def nullable = trueValue.nullable || falseValue.nullable
+  override def nullable = predicate.nullable || (trueValue.nullable && falseValue.nullable)


I think this was correct before. The nullability of the predicate does not affect the nullability of the output since a null predicate will just cause the falseValue to be output, not null.

marmbrus · 2014-04-21T16:32:27Z

ok to test

AmplabJenkins · 2014-04-21T16:32:56Z

Merged build triggered.

AmplabJenkins · 2014-04-21T16:33:03Z

Merged build started.

AmplabJenkins · 2014-04-21T18:01:37Z

Merged build finished.

AmplabJenkins · 2014-04-21T18:01:37Z

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

marmbrus · 2014-04-21T18:19:08Z

@andrewor14, any idea what is up with org.apache.spark.ui.UISuite?

andrewor14 · 2014-04-21T18:52:19Z

It has the same problem as streaming.UISuite, which was disabled recently. We should do the same for ui.UISuite

#446 faced a connection refused exception from these tests, causing them to timeout and fail after a long time. For now, let's disable these tests. (We recently disabled the corresponding test in streaming in 7863ecc. These tests are very similar). Author: Andrew Or <andrewor14@gmail.com> Closes #466 from andrewor14/ignore-ui-tests and squashes the following commits: 6f5a362 [Andrew Or] Ignore org.apache.spark.ui.UISuite tests (cherry picked from commit af46f1f) Signed-off-by: Patrick Wendell <pwendell@gmail.com>

AmplabJenkins · 2014-04-22T00:42:56Z

Merged build triggered.

AmplabJenkins · 2014-04-22T00:43:03Z

Merged build started.

AmplabJenkins · 2014-04-22T01:55:28Z

Merged build finished.

AmplabJenkins · 2014-04-22T01:55:29Z

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

AmplabJenkins · 2014-04-22T02:52:55Z

Merged build triggered.

AmplabJenkins · 2014-04-22T02:53:01Z

Merged build started.

AmplabJenkins · 2014-04-22T04:23:49Z

Merged build finished.

AmplabJenkins · 2014-04-22T04:23:49Z

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

marmbrus · 2014-05-09T21:02:19Z

test this please.

AmplabJenkins · 2014-05-09T21:02:59Z

Build triggered.

AmplabJenkins · 2014-05-09T21:03:07Z

Build started.

AmplabJenkins · 2014-05-09T22:06:39Z

Build finished. All automated tests passed.

AmplabJenkins · 2014-05-09T22:06:39Z

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

marmbrus · 2014-05-09T22:31:48Z

LGTM

@pwendell can you please merge?

pwendell · 2014-05-10T19:03:04Z

@chenghao-intel this has some merge conflicts - mind updating it?

AmplabJenkins · 2014-05-12T08:07:58Z

Merged build triggered.

AmplabJenkins · 2014-05-12T20:07:59Z

Merged build triggered.

AmplabJenkins · 2014-05-12T20:08:08Z

Merged build started.

AmplabJenkins · 2014-05-12T21:23:39Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-05-12T21:23:40Z

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

rxin · 2014-05-15T19:03:41Z

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

-      false
-    } else if (l == null || r == null ) {
-      null
+    if(l == false) {


mind adding a space here?

chenghao-intel · 2014-05-16T01:47:25Z

Thanks @rxin.
It's done, can you re-test it?

AmplabJenkins · 2014-05-16T01:47:57Z

Merged build triggered.

AmplabJenkins · 2014-05-16T01:48:06Z

Merged build started.

rxin · 2014-05-16T01:50:36Z

LGTM. I will merge it once Jenkins comes back green.

AmplabJenkins · 2014-05-16T03:07:05Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-05-16T03:07:06Z

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

rxin · 2014-05-16T05:12:18Z

I've merged this. Thanks a lot!

This patch unify the foldable & nullable interface for Expression. 1) Deterministic-less UDF (like Rand()) can not be folded. 2) Short-circut will significantly improves the performance in Expression Evaluation, however, the stateful UDF should not be ignored in a short-circuit evaluation(e.g. in expression: col1 > 0 and row_sequence() < 1000, row_sequence() can not be ignored even if col1 > 0 is false) I brought an concept of DeferredObject from Hive, which has 2 kinds of children classes (EagerResult / DeferredResult), the former requires triggering the evaluation before it's created, while the later trigger the evaluation when first called its get() method. Author: Cheng Hao <hao.cheng@intel.com> Closes #446 from chenghao-intel/expression_deferred_evaluation and squashes the following commits: d2729de [Cheng Hao] Fix the codestyle issues a08f09c [Cheng Hao] fix bug in or/and short-circuit evaluation af2236b [Cheng Hao] revert the short-circuit expression evaluation for IF b7861d2 [Cheng Hao] Add Support for Deferred Expression Evaluation (cherry picked from commit a20fea9) Signed-off-by: Reynold Xin <rxin@apache.org>

apache#446 faced a connection refused exception from these tests, causing them to timeout and fail after a long time. For now, let's disable these tests. (We recently disabled the corresponding test in streaming in 7863ecc. These tests are very similar). Author: Andrew Or <andrewor14@gmail.com> Closes apache#466 from andrewor14/ignore-ui-tests and squashes the following commits: 6f5a362 [Andrew Or] Ignore org.apache.spark.ui.UISuite tests

This patch unify the foldable & nullable interface for Expression. 1) Deterministic-less UDF (like Rand()) can not be folded. 2) Short-circut will significantly improves the performance in Expression Evaluation, however, the stateful UDF should not be ignored in a short-circuit evaluation(e.g. in expression: col1 > 0 and row_sequence() < 1000, row_sequence() can not be ignored even if col1 > 0 is false) I brought an concept of DeferredObject from Hive, which has 2 kinds of children classes (EagerResult / DeferredResult), the former requires triggering the evaluation before it's created, while the later trigger the evaluation when first called its get() method. Author: Cheng Hao <hao.cheng@intel.com> Closes apache#446 from chenghao-intel/expression_deferred_evaluation and squashes the following commits: d2729de [Cheng Hao] Fix the codestyle issues a08f09c [Cheng Hao] fix bug in or/and short-circuit evaluation af2236b [Cheng Hao] revert the short-circuit expression evaluation for IF b7861d2 [Cheng Hao] Add Support for Deferred Expression Evaluation

marmbrus reviewed Apr 21, 2014
View reviewed changes

andrewor14 mentioned this pull request Apr 21, 2014

[Hot Fix] Ignore org.apache.spark.ui.UISuite tests #466

Closed

rxin reviewed May 15, 2014
View reviewed changes

chenghao-intel added 4 commits May 16, 2014 09:35

Add Support for Deferred Expression Evaluation

b7861d2

revert the short-circuit expression evaluation for IF

af2236b

fix bug in or/and short-circuit evaluation

a08f09c

Fix the codestyle issues

d2729de

asfgit closed this in a20fea9 May 16, 2014

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

markhamstra pushed a commit to markhamstra/spark that referenced this pull request Nov 7, 2017

Scalastyle (apache#446)

f8cf9db

j-esse pushed a commit to j-esse/spark that referenced this pull request Jan 24, 2019

Track active shuffles by stage (apache#446)

92fe984

[Spark-1461] Deferred Expression Evaluation (short-circuit evaluation) #446

[Spark-1461] Deferred Expression Evaluation (short-circuit evaluation) #446

Conversation

chenghao-intel commented Apr 18, 2014

AmplabJenkins commented Apr 18, 2014

marmbrus commented Apr 18, 2014

chenghao-intel commented Apr 19, 2014

chenghao-intel commented Apr 21, 2014

marmbrus Apr 21, 2014

Choose a reason for hiding this comment

marmbrus commented Apr 21, 2014

AmplabJenkins commented Apr 21, 2014

AmplabJenkins commented Apr 21, 2014

AmplabJenkins commented Apr 21, 2014

AmplabJenkins commented Apr 21, 2014

marmbrus commented Apr 21, 2014

andrewor14 commented Apr 21, 2014

AmplabJenkins commented Apr 22, 2014

AmplabJenkins commented Apr 22, 2014

AmplabJenkins commented Apr 22, 2014

AmplabJenkins commented Apr 22, 2014

AmplabJenkins commented Apr 22, 2014

AmplabJenkins commented Apr 22, 2014

AmplabJenkins commented Apr 22, 2014

AmplabJenkins commented Apr 22, 2014

marmbrus commented May 9, 2014

AmplabJenkins commented May 9, 2014

AmplabJenkins commented May 9, 2014

AmplabJenkins commented May 9, 2014

AmplabJenkins commented May 9, 2014

marmbrus commented May 9, 2014

pwendell commented May 10, 2014

AmplabJenkins commented May 12, 2014

AmplabJenkins commented May 12, 2014

AmplabJenkins commented May 12, 2014

AmplabJenkins commented May 12, 2014

AmplabJenkins commented May 12, 2014

rxin May 15, 2014

Choose a reason for hiding this comment

chenghao-intel commented May 16, 2014

AmplabJenkins commented May 16, 2014

AmplabJenkins commented May 16, 2014

rxin commented May 16, 2014

AmplabJenkins commented May 16, 2014

AmplabJenkins commented May 16, 2014

rxin commented May 16, 2014