[SPARK-8221][SQL]Add pmod function #6783

zhichao-li · 2015-06-12T07:05:48Z

https://issues.apache.org/jira/browse/SPARK-8221

One concern is the result would be negative if the divisor is not positive( i.e pmod(7, -3) ), but the behavior is the same as hive.

SparkQA · 2015-06-12T07:10:17Z

Test build #34760 has finished for PR 6783 at commit b18a861.

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

chenghao-intel · 2015-06-12T08:29:54Z

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

We need to support Decimal type as well.

Byte, Long etc.

Sorry, I've just checked the Hive doc, seems only Int and Double types are accepted, let's do the same thing.

It's important that we should not generate a result different from hive, but I think we are free to extend when hive would throw an exception, only if we consider it a reasonable change.

however I think it is ok to leave Decimal unsupported here.:P

according to hive, I think we need to support Decimal...

Hmm, it's not the same as the doc described, https://cwiki.apache.org/confluence/display/Hive/LanguageManual+UDF
The reason for this is conform to the BinaryMathExpression, which supposed to be DoubleType, just the like pow etc.

SparkQA · 2015-06-15T02:21:35Z

Test build #34900 has finished for PR 6783 at commit 01a0f3b.

This patch fails Scala style tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

SparkQA · 2015-06-15T03:29:18Z

Test build #34903 has finished for PR 6783 at commit ca9cc53.

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

SparkQA · 2015-06-15T07:17:08Z

Test build #34912 has finished for PR 6783 at commit 90e588c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

SparkQA · 2015-06-16T03:01:30Z

Test build #34963 has finished for PR 6783 at commit 88c63ab.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

adrian-wang · 2015-06-16T03:05:41Z

LGTM except the full type APIs

zhichao-li · 2015-06-16T05:55:25Z

@adrian-wang @chenghao-intel , wondering what type of api we should reserve regarding to this kind of api def pmod(leftName: String, r: Integer): Column ? I guess it would be a good thing that enable such input: select pmod(columName, 10) .. from ...

adrian-wang · 2015-06-16T07:04:19Z

@zhichao-li if you are using sql(), even without those APIs, the query will still work. It is just for dataframe api

zhichao-li · 2015-06-16T07:45:28Z

hum... right . It just give us a convenient way to enabledf.select(pmod($"a", 10)) without using verbose literal wrapper. I'll remove it if not too much benefit we can gain from those.

chenghao-intel · 2015-06-16T08:44:53Z

Adding API is easier, but remove the published API is difficult, let's keep the DF API simple as much as possible.

SparkQA · 2015-06-16T10:33:28Z

Test build #34975 has finished for PR 6783 at commit b5233a2.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

cloud-fan · 2015-06-16T15:46:42Z

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

How about follow Remainder to use Integral.rem to do the computation? That will save a lot of code here...

Good idea , but seems like the clac result would be a little bit different with the expected result. pmod(7.2, 4.1), actual: 3.099999999999999, expected: 3.1000000000000005 . I'm keen on keeping it this way if not obvious downside.

private lazy val integral = dataType match { case i: IntegralType => i.integral.asInstanceOf[Integral[Any]] case i: FractionalType => i.asIntegral.asInstanceOf[Integral[Any]] case i: DecimalType => i.asIntegral.asInstanceOf[Integral[Any]] } protected override def evalInternal(a: Any, n: Any) = integral.rem(integral.plus(integral.rem(a, n), n), n)

Ok maybe you can send another PR to fix Divide in this way too, as it's result is different from hive's.

SparkQA · 2015-06-18T10:20:43Z

Test build #35116 has finished for PR 6783 at commit 372b66c.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

SparkQA · 2015-06-19T01:18:03Z

Test build #35203 has finished for PR 6783 at commit 9be7e4c.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

chenghao-intel · 2015-06-19T01:37:18Z

retest this please.

zhichao-li · 2015-06-19T01:40:06Z

retest this please.

SparkQA · 2015-06-19T01:59:50Z

Test build #35209 has finished for PR 6783 at commit 9be7e4c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

rxin · 2015-06-19T06:29:16Z

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

i don't think you need to override this

Seems like it's a must since symbol is abstract in BinaryOperator

zhichao-li · 2015-07-14T06:32:24Z

retest this please. Fail by timeout after 1 minute

SparkQA · 2015-07-14T06:51:40Z

Test build #9 has finished for PR 6783 at commit 0493dc4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

SparkQA · 2015-07-14T08:03:03Z

Test build #11 has finished for PR 6783 at commit 0493dc4.

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

zhichao-li · 2015-07-14T08:07:02Z

retest this please.

SparkQA · 2015-07-14T08:13:21Z

Test build #37208 has finished for PR 6783 at commit 0493dc4.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

SparkQA · 2015-07-14T09:26:41Z

Test build #37219 has finished for PR 6783 at commit 0493dc4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

SparkQA · 2015-07-14T09:27:38Z

Test build #13 has finished for PR 6783 at commit 0493dc4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

zhichao-li · 2015-07-15T00:54:27Z

retest this please.

SparkQA · 2015-07-15T02:27:59Z

Test build #18 has finished for PR 6783 at commit 0493dc4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

SparkQA · 2015-07-15T02:59:56Z

Test build #37291 has finished for PR 6783 at commit 0493dc4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

zhichao-li · 2015-07-15T05:41:32Z

retest this please.

rxin · 2015-07-15T06:38:41Z

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

remove this -- and after #7348 is merged, you'd need to define inputType to be NumericType

rxin · 2015-07-15T06:40:40Z

@zhichao-li this looks pretty good now. Just need to update it to work with the latest type checking framework.

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

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

we should override toString to be s"pmod($left, $right)"

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

Test build #37332 has finished for PR 6783 at commit 0493dc4.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

rxin · 2015-07-15T07:12:57Z

Jenkins, retest this please.

SparkQA · 2015-07-15T07:23:37Z

Test build #20 has finished for PR 6783 at commit 0493dc4.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

SparkQA · 2015-07-15T07:23:39Z

Test build #37321 has finished for PR 6783 at commit 0493dc4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

SparkQA · 2015-07-15T08:21:12Z

Test build #37334 has finished for PR 6783 at commit 7083eb9.

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 Pmod(left: Expression, right: Expression) extends BinaryArithmetic
- 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

SparkQA · 2015-07-15T08:42:06Z

Test build #37335 has finished for PR 6783 at commit 7083eb9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic

cloud-fan · 2015-07-15T15:53:24Z

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

Actually this symbol is not needed as you have override toString, however we have to implement it here as it's defined abstract in BinaryOperator. We may need to fix it in the future.

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

lgtm

rxin · 2015-07-15T17:43:29Z

Alright I'm merging this. Thanks a lot for working on it.

rxin · 2016-07-11T19:57:13Z

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

+        case dt: DecimalType =>
+          val decimalAdd = "$plus"
+          s"""
+            ${ctx.javaType(dataType)} r = $eval1.remainder($eval2);


fyi there is a bug here -- if we use pmod twice, this will fail codegen because r is not unique.

chenghao-intel reviewed Jun 12, 2015
View reviewed changes

zhichao-li force-pushed the pmod2 branch from 01a0f3b to ca9cc53 Compare June 15, 2015 03:07

cloud-fan reviewed Jun 16, 2015
View reviewed changes

zhichao-li force-pushed the pmod2 branch from 372b66c to 9be7e4c Compare June 19, 2015 01:06

rxin reviewed Jun 19, 2015
View reviewed changes

rxin reviewed Jul 15, 2015
View reviewed changes

zhichao-li added 2 commits July 15, 2015 14:45

add pmod

d26dba7

update to the latest type checking

7083eb9

zhichao-li force-pushed the pmod2 branch from 0493dc4 to 7083eb9 Compare July 15, 2015 06:59

cloud-fan reviewed Jul 15, 2015
View reviewed changes

asfgit closed this in a938527 Jul 15, 2015

zhichao-li deleted the pmod2 branch September 9, 2015 06:06

rxin reviewed Jul 11, 2016
View reviewed changes

[SPARK-8221][SQL]Add pmod function #6783

[SPARK-8221][SQL]Add pmod function #6783

Uh oh!

Conversation

zhichao-li commented Jun 12, 2015

Uh oh!

SparkQA commented Jun 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 16, 2015

Uh oh!

adrian-wang commented Jun 16, 2015

Uh oh!

zhichao-li commented Jun 16, 2015

Uh oh!

adrian-wang commented Jun 16, 2015

Uh oh!

zhichao-li commented Jun 16, 2015

Uh oh!

chenghao-intel commented Jun 16, 2015

Uh oh!

SparkQA commented Jun 16, 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!

SparkQA commented Jun 18, 2015

Uh oh!

SparkQA commented Jun 19, 2015

Uh oh!

chenghao-intel commented Jun 19, 2015

Uh oh!

zhichao-li commented Jun 19, 2015

Uh oh!

SparkQA commented Jun 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhichao-li commented Jul 14, 2015

Uh oh!

SparkQA commented Jul 14, 2015

Uh oh!

SparkQA commented Jul 14, 2015

Uh oh!

zhichao-li commented Jul 14, 2015

Uh oh!

SparkQA commented Jul 14, 2015

Uh oh!

SparkQA commented Jul 14, 2015

Uh oh!

SparkQA commented Jul 14, 2015

Uh oh!

zhichao-li commented Jul 15, 2015

Uh oh!

SparkQA commented Jul 15, 2015

Uh oh!