[SPARK-8945][SQL] Add add and subtract expressions for IntervalType by viirya · Pull Request #7398 · apache/spark

viirya · 2015-07-14T17:14:12Z

JIRA: https://issues.apache.org/jira/browse/SPARK-8945

Add add and subtract expressions for IntervalType.

SparkQA · 2015-07-14T17:20:11Z

Test build #37240 has finished for PR 7398 at commit d3e9d0e.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2015-07-15T02:37:03Z

this seems excessive, since it won't work for multiply or division.

rxin · 2015-07-15T02:38:15Z

Let's revisit this patch once #7348 is merged.

SparkQA · 2015-07-15T04:28:04Z

Test build #37301 has finished for PR 7398 at commit acfe1ab.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2015-07-15T04:51:45Z

A failure about modified error message. I will update soon.

SparkQA · 2015-07-15T05:27:31Z

Test build #37316 has finished for PR 7398 at commit 83ec129.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2015-07-15T06:38:07Z

There are some conflicts since #7348 is merged. I will update later.

rxin · 2015-07-15T06:47:09Z

Now #7348 is in, just create a TypeCollection that contains all the numeric types as well as interval type.

ok. I will update again.

…ract Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TypeUtils.scala

rxin · 2015-07-15T06:49:50Z

i don't think you want this -- it is super slow. Just remove this function, and define codegen in Add / Subtract, rather than relying on what BinaryArithmetic provides.

ok. I wanted to avoid defining codegen in two operations. Update later together.

…ract Conflicts: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ExpressionTypeCheckingSuite.scala

rxin · 2015-07-15T07:51:48Z

just inline $decimalMethod here; don't define the function anymore.

rxin · 2015-07-15T07:53:04Z

LGTM other than that.

rxin · 2015-07-15T08:15:14Z

LGTM

SparkQA · 2015-07-15T08:43:19Z

Test build #37330 has finished for PR 7398 at commit 13a2fc5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- trait ImplicitCastInputTypes extends ExpectsInputTypes
- abstract class BinaryOperator extends BinaryExpression with ExpectsInputTypes
- case class UnaryMinus(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class UnaryPositive(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class Abs(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class BitwiseNot(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class Factorial(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Hex(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Unhex(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Md5(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Sha1(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Crc32(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Not(child: Expression)
- case class And(left: Expression, right: Expression) extends BinaryOperator with Predicate
- case class Or(left: Expression, right: Expression) extends BinaryOperator with Predicate
- trait StringRegexExpression extends ImplicitCastInputTypes
- trait String2StringExpression extends ImplicitCastInputTypes
- trait StringComparison extends ImplicitCastInputTypes
- case class StringSpace(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class StringLength(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Ascii(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Base64(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class UnBase64(child: Expression) extends UnaryExpression with ImplicitCastInputTypes

SparkQA · 2015-07-15T09:51:51Z

Test build #37337 has finished for PR 7398 at commit dbe3906.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- trait ImplicitCastInputTypes extends ExpectsInputTypes
- abstract class BinaryOperator extends BinaryExpression with ExpectsInputTypes
- case class UnaryMinus(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class UnaryPositive(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class Abs(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class BitwiseNot(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class Factorial(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Hex(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Unhex(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Md5(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Sha1(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Crc32(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Not(child: Expression)
- case class And(left: Expression, right: Expression) extends BinaryOperator with Predicate
- case class Or(left: Expression, right: Expression) extends BinaryOperator with Predicate
- trait StringRegexExpression extends ImplicitCastInputTypes
- trait String2StringExpression extends ImplicitCastInputTypes
- trait StringComparison extends ImplicitCastInputTypes
- case class StringSpace(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class StringLength(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Ascii(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Base64(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class UnBase64(child: Expression) extends UnaryExpression with ImplicitCastInputTypes

SparkQA · 2015-07-15T10:11:41Z

Test build #37342 has finished for PR 7398 at commit 5abae28.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class LDAModel(JavaModelWrapper):
- class LDA(object):
- case class Round(child: Expression, scale: Expression)

viirya · 2015-07-15T10:16:25Z

Looks like an unrelated failure.

SparkQA · 2015-07-15T10:26:05Z

Test build #37339 has finished for PR 7398 at commit 6f5b72e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class LDAModel(JavaModelWrapper):
- class LDA(object):
- trait ImplicitCastInputTypes extends ExpectsInputTypes
- abstract class BinaryOperator extends BinaryExpression with ExpectsInputTypes
- case class UnaryMinus(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class UnaryPositive(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class Abs(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class BitwiseNot(child: Expression) extends UnaryExpression with ExpectsInputTypes
- case class Factorial(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Hex(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Unhex(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Round(child: Expression, scale: Expression)
- case class Md5(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Sha1(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Crc32(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Not(child: Expression)
- case class And(left: Expression, right: Expression) extends BinaryOperator with Predicate
- case class Or(left: Expression, right: Expression) extends BinaryOperator with Predicate
- trait StringRegexExpression extends ImplicitCastInputTypes
- trait String2StringExpression extends ImplicitCastInputTypes
- trait StringComparison extends ImplicitCastInputTypes
- case class StringSpace(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class StringLength(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Ascii(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class Base64(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
- case class UnBase64(child: Expression) extends UnaryExpression with ImplicitCastInputTypes

cloud-fan · 2015-07-15T16:07:01Z

can we simplify it to TypeCollection(NumericType, IntervalType)?

Because TypeCollection also specifies precedence for types, I am not sure if it works when we use NumericType here?

Actually we should use NumericType here.
Think about 2 + "2", before this PR, we will first cast "2" to 2.0(double is the default for numeric type), and then the result should be double 4.0. After this PR, we will first cast "2" to 2(byte is the default for your type collection), and thus make the result of type int.

OK. Another problem that might be minor is that the casting error message will become argument 1 is expected to be of type (numeric or interval), instead of ...type (tinyint or smallint or int or bigint or float or double or decimal or interval....
Is it still informative? Because supposed that NumericType should be internal? Will users know what type is numeric meaning?

I saw there are already use cases for directly showing numeric in error message. So I updated this too.

cloud-fan · 2015-07-15T16:07:58Z

lgtm

…ract Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/types/AbstractDataType.scala

Add add and subtract expressions for IntervalType.
d3e9d0e

Fix scala style.
acfe1ab

rxin reviewed Jul 15, 2015
View reviewed changes

Remove intervalMethod.
83ec129

rxin reviewed Jul 15, 2015
View reviewed changes

Merge remote-tracking branch 'upstream/master' into interval_add_subt…
13a2fc5
…ract Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TypeUtils.scala

rxin reviewed Jul 15, 2015
View reviewed changes

viirya added 2 commits July 15, 2015 15:43

For comments.
dbe3906

Merge remote-tracking branch 'upstream/master' into interval_add_subt…
6f5b72e
…ract Conflicts: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ExpressionTypeCheckingSuite.scala

rxin reviewed Jul 15, 2015
View reviewed changes

For comments.
5abae28

cloud-fan reviewed Jul 15, 2015
View reviewed changes

Merge remote-tracking branch 'upstream/master' into interval_add_subt…
acd1f1e
…ract Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/types/AbstractDataType.scala

Uh oh!

Conversation

viirya commented Jul 14, 2015

Uh oh!

SparkQA commented Jul 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Jul 15, 2015

Uh oh!

SparkQA commented Jul 15, 2015

Uh oh!

viirya commented Jul 15, 2015

Uh oh!

SparkQA commented Jul 15, 2015

Uh oh!

viirya commented Jul 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Jul 15, 2015

Uh oh!

rxin commented Jul 15, 2015

Uh oh!

SparkQA commented Jul 15, 2015

Uh oh!

SparkQA commented Jul 15, 2015

Uh oh!

SparkQA commented Jul 15, 2015

Uh oh!

viirya commented Jul 15, 2015

Uh oh!

SparkQA commented Jul 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jul 15, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants