[SPARK-8759][SQL] add default eval to binary and unary expression according to default behavior of nullable #7157

cloud-fan · 2015-07-01T11:39:13Z

We have nullSafeCodeGen to provide default code generation for binary and unary expression, and we can do the same thing for eval.

AmplabJenkins · 2015-07-01T11:43:10Z

Merged build triggered.

cloud-fan · 2015-07-01T11:44:19Z

cc @rxin

AmplabJenkins · 2015-07-01T12:08:11Z

Merged build triggered.

AmplabJenkins · 2015-07-01T12:13:11Z

Merged build triggered.

AmplabJenkins · 2015-07-01T12:20:06Z

Merged build started.

AmplabJenkins · 2015-07-01T12:20:06Z

Merged build started.

AmplabJenkins · 2015-07-01T12:20:06Z

Merged build started.

SparkQA · 2015-07-01T12:21:29Z

Test build #36255 has started for PR 7157 at commit 24cf8db.

SparkQA · 2015-07-01T12:23:24Z

Test build #36252 has started for PR 7157 at commit 070e2cb.

SparkQA · 2015-07-01T12:23:25Z

Test build #36256 has started for PR 7157 at commit ffa742c.

SparkQA · 2015-07-01T12:38:55Z

Test build #36255 has finished for PR 7157 at commit 24cf8db.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-07-01T12:39:00Z

Merged build finished. Test FAILed.

SparkQA · 2015-07-01T14:02:15Z

Test build #36256 has finished for PR 7157 at commit ffa742c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class UnaryMinus(child: Expression) extends UnaryExpression
- case class UnaryPositive(child: Expression) extends UnaryExpression
- case class Abs(child: Expression) extends UnaryExpression
- case class BitwiseNot(child: Expression) extends UnaryExpression
- case class InSet(child: Expression, hset: Set[Any])
- case class Contains(left: Expression, right: Expression) extends StringPredicate
- case class StartsWith(left: Expression, right: Expression) extends StringPredicate
- case class EndsWith(left: Expression, right: Expression) extends StringPredicate

AmplabJenkins · 2015-07-01T14:02:48Z

Merged build finished. Test PASSed.

SparkQA · 2015-07-01T14:05:04Z

Test build #36252 has finished for PR 7157 at commit 070e2cb.

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

AmplabJenkins · 2015-07-01T14:05:50Z

Merged build finished. Test PASSed.

cloud-fan · 2015-07-02T12:23:34Z

waiting for #7175

yhuai · 2015-07-02T16:01:34Z

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

Can you add scala doc?

will add it during rebase, thanks for reminding!

Why not just leave it as unimplemented, so developer will notice that during coding, not runtime.

There maybe exceptional cases that user need to override eval, at that time they don't need to implement nullSafeEval.

chenghao-intel · 2015-07-03T12:57:50Z

Thank you for working on this @cloud-fan.This PR definitely will save the code, but I have some concerns on the interface updating, not about what you did in this PR, but in general:

Currently, too many callback functions need to implement when inherit from different abstract expression classes, it will be the barrier to who are new in the expression framework. From the SPI design princple point of view, we'd better keep the interface the simpler the better.
Default behavor is ok for most of cases, but there are always exceptional cases, expression developer needs more freedom.
It's not a big deal if we leave some duplicated code in expressions, as each of them are quite independent, that would be more straightforward for people to read / review it. (we don't want to jump back and forward while reading the code from web, right?)

cloud-fan · 2015-07-03T14:02:00Z

hi @chenghao-intel , thanks for looking into this! I agree with your opinions, but have some other thoughts:

In this PR I added a new nullSafeEval for binary and unary expressions, with a default implementation: throw exception. So if new developers don't know this, it's OK for them to just override eval with some duplicated code. For someone is familiar with expression framework, they can use this shortcut to save code.
For exceptional cases, developers can still override eval and no need to care about nullSafeEval.
Duplicated code is not a big deal, but this pattern is so common and I think it's good to abstract it.

AmplabJenkins · 2015-07-06T06:38:11Z

Build triggered.

AmplabJenkins · 2015-07-06T06:38:17Z

Build started.

SparkQA · 2015-07-06T06:41:12Z

Test build #36562 has started for PR 7157 at commit 2876cbd.

SparkQA · 2015-07-06T08:14:47Z

Test build #36562 has finished for PR 7157 at commit 2876cbd.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- case class InSet(child: Expression, hset: Set[Any])
- case class Decode(bin: Expression, charset: Expression)

AmplabJenkins · 2015-07-06T08:15:16Z

Build finished. Test PASSed.

AmplabJenkins · 2015-07-06T11:08:10Z

Merged build triggered.

AmplabJenkins · 2015-07-06T11:08:19Z

Merged build started.

SparkQA · 2015-07-06T11:12:03Z

Test build #36571 has started for PR 7157 at commit a0a3b96.

SparkQA · 2015-07-06T12:44:06Z

Test build #36571 has finished for PR 7157 at commit a0a3b96.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class InSet(child: Expression, hset: Set[Any])

AmplabJenkins · 2015-07-06T12:44:38Z

Merged build finished. Test PASSed.

cloud-fan · 2015-07-06T13:24:11Z

Hi @rxin , can you review it please? It's very easy to get conflict and I have to rebase it again and again...

rxin · 2015-07-06T21:59:15Z

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

can you update the javadoc above to include param doc for f, defining what the input / output is?

rxin · 2015-07-06T22:16:00Z

Looks good to me. Some comments about naming.

AmplabJenkins · 2015-07-07T03:23:10Z

Merged build triggered.

AmplabJenkins · 2015-07-07T03:23:19Z

Merged build started.

SparkQA · 2015-07-07T03:26:35Z

Test build #36635 has started for PR 7157 at commit f3987c6.

SparkQA · 2015-07-07T04:59:06Z

Test build #36635 has finished for PR 7157 at commit f3987c6.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class InSet(child: Expression, hset: Set[Any])

AmplabJenkins · 2015-07-07T04:59:40Z

Merged build finished. Test PASSed.

rxin · 2015-07-07T05:13:45Z

Thanks. I've merged this.

cloud-fan force-pushed the refactor branch from 441e697 to 070e2cb Compare July 1, 2015 11:40

cloud-fan force-pushed the refactor branch from 070e2cb to 24cf8db Compare July 1, 2015 12:07

cloud-fan force-pushed the refactor branch from 24cf8db to ffa742c Compare July 1, 2015 12:11

yhuai reviewed Jul 2, 2015
View reviewed changes

chenghao-intel mentioned this pull request Jul 5, 2015

[SPARK-8238][SPARK-8239][SPARK-8242][SPARK-8243][SPARK-8268][SQL]Add ascii/base64/unbase64/encode/decode functions #6843

Closed

cloud-fan force-pushed the refactor branch from ffa742c to 2876cbd Compare July 6, 2015 06:34

cloud-fan force-pushed the refactor branch from 2876cbd to a0a3b96 Compare July 6, 2015 11:07

rxin reviewed Jul 6, 2015
View reviewed changes

refactor Expression

f3987c6

cloud-fan force-pushed the refactor branch from a0a3b96 to f3987c6 Compare July 7, 2015 03:18

asfgit closed this in c46aaf4 Jul 7, 2015

[SPARK-8759][SQL] add default eval to binary and unary expression according to default behavior of nullable #7157

[SPARK-8759][SQL] add default eval to binary and unary expression according to default behavior of nullable #7157

Uh oh!

Conversation

cloud-fan commented Jul 1, 2015

Uh oh!

AmplabJenkins commented Jul 1, 2015

Uh oh!

cloud-fan commented Jul 1, 2015

Uh oh!

AmplabJenkins commented Jul 1, 2015

Uh oh!

AmplabJenkins commented Jul 1, 2015

Uh oh!

AmplabJenkins commented Jul 1, 2015

Uh oh!

AmplabJenkins commented Jul 1, 2015

Uh oh!

AmplabJenkins commented Jul 1, 2015

Uh oh!

SparkQA commented Jul 1, 2015

Uh oh!

SparkQA commented Jul 1, 2015

Uh oh!

SparkQA commented Jul 1, 2015

Uh oh!

SparkQA commented Jul 1, 2015

Uh oh!

AmplabJenkins commented Jul 1, 2015

Uh oh!

SparkQA commented Jul 1, 2015

Uh oh!

AmplabJenkins commented Jul 1, 2015

Uh oh!

SparkQA commented Jul 1, 2015

Uh oh!

AmplabJenkins commented Jul 1, 2015

Uh oh!

cloud-fan commented Jul 2, 2015

Uh oh!

yhuai Jul 2, 2015

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 2, 2015

Choose a reason for hiding this comment

Uh oh!

chenghao-intel Jul 3, 2015

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 3, 2015

Choose a reason for hiding this comment

Uh oh!

chenghao-intel commented Jul 3, 2015

Uh oh!

cloud-fan commented Jul 3, 2015

Uh oh!

AmplabJenkins commented Jul 6, 2015

Uh oh!

AmplabJenkins commented Jul 6, 2015

Uh oh!

SparkQA commented Jul 6, 2015

Uh oh!

SparkQA commented Jul 6, 2015

Uh oh!

AmplabJenkins commented Jul 6, 2015

Uh oh!

AmplabJenkins commented Jul 6, 2015

Uh oh!

AmplabJenkins commented Jul 6, 2015

Uh oh!

SparkQA commented Jul 6, 2015

Uh oh!

SparkQA commented Jul 6, 2015

Uh oh!

AmplabJenkins commented Jul 6, 2015

Uh oh!

cloud-fan commented Jul 6, 2015

Uh oh!

rxin Jul 6, 2015

Choose a reason for hiding this comment