You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mgaido91 <gi...@git.apache.org> on 2018/09/11 15:57:18 UTC
[GitHub] spark pull request #22395: [SPARK-16323][SQ] Add IntegralDivide expression
GitHub user mgaido91 opened a pull request:
https://github.com/apache/spark/pull/22395
[SPARK-16323][SQ] Add IntegralDivide expression
## What changes were proposed in this pull request?
The PR takes over #14036 and it introduces a new expression `IntegralDivide` in order to avoid the several unneded cast added previously.
In order to prove the performance gain, the following benchmark has been run:
```
test("Benchmark IntegralDivide") {
val r = new scala.util.Random(91)
val nData = 1000000
val testDataInt = (1 to nData).map(_ => (r.nextInt(), r.nextInt()))
val testDataLong = (1 to nData).map(_ => (r.nextLong(), r.nextLong()))
val testDataShort = (1 to nData).map(_ => (r.nextInt().toShort, r.nextInt().toShort))
// old code
val oldExprsInt = testDataInt.map(x =>
Cast(Divide(Cast(Literal(x._1), DoubleType), Cast(Literal(x._2), DoubleType)), LongType))
val oldExprsLong = testDataLong.map(x =>
Cast(Divide(Cast(Literal(x._1), DoubleType), Cast(Literal(x._2), DoubleType)), LongType))
val oldExprsShort = testDataShort.map(x =>
Cast(Divide(Cast(Literal(x._1), DoubleType), Cast(Literal(x._2), DoubleType)), LongType))
// new code
val newExprsInt = testDataInt.map(x => IntegralDivide(x._1, x._2))
val newExprsLong = testDataLong.map(x => IntegralDivide(x._1, x._2))
val newExprsShort = testDataShort.map(x => IntegralDivide(x._1, x._2))
Seq(("Long", "old", oldExprsLong),
("Long", "new", newExprsLong),
("Int", "old", oldExprsInt),
("Int", "new", newExprsShort),
("Short", "old", oldExprsShort),
("Short", "new", oldExprsShort)).foreach { case (dt, t, ds) =>
val start = System.nanoTime()
ds.foreach(e => e.eval(EmptyRow))
val endNoCodegen = System.nanoTime()
println(s"Running $nData op with $t code on $dt (no-codegen): ${(endNoCodegen - start) / 1000000} ms")
}
}
```
The results on my laptop are:
```
Running 1000000 op with old code on Long (no-codegen): 600 ms
Running 1000000 op with new code on Long (no-codegen): 112 ms
Running 1000000 op with old code on Int (no-codegen): 560 ms
Running 1000000 op with new code on Int (no-codegen): 135 ms
Running 1000000 op with old code on Short (no-codegen): 317 ms
Running 1000000 op with new code on Short (no-codegen): 153 ms
```
Showing a 2-5X improvement. The benchmark doesn't include code generation as it is pretty hard to test the performance there as for such simple operations the most of the time is spent in the code generation/compilation process.
## How was this patch tested?
added UTs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mgaido91/spark SPARK-16323
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22395.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #22395
----
commit 649b45875e81e47ba3282150b669f766fbe806ba
Author: Marco Gaido <ma...@...>
Date: 2018-09-11T15:50:56Z
[SPARK-16323][SQ] Add IntegerDivide expression
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQ] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/22395
cc @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95954/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96128 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96128/testReport)** for PR 22395 at commit [`c471bef`](https://github.com/apache/spark/commit/c471bef5292016e942ffe2788760eda08e48783e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96079 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96079/testReport)** for PR 22395 at commit [`71255a1`](https://github.com/apache/spark/commit/71255a1787012baf2d5188991421e8197ec44733).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95995/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217935398
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala ---
@@ -143,16 +143,14 @@ class ArithmeticExpressionSuite extends SparkFunSuite with ExpressionEvalHelper
}
}
- // By fixing SPARK-15776, Divide's inputType is required to be DoubleType of DecimalType.
- // TODO: in future release, we should add a IntegerDivide to support integral types.
- ignore("/ (Divide) for integral type") {
- checkEvaluation(Divide(Literal(1.toByte), Literal(2.toByte)), 0.toByte)
- checkEvaluation(Divide(Literal(1.toShort), Literal(2.toShort)), 0.toShort)
- checkEvaluation(Divide(Literal(1), Literal(2)), 0)
- checkEvaluation(Divide(Literal(1.toLong), Literal(2.toLong)), 0.toLong)
- checkEvaluation(Divide(positiveShortLit, negativeShortLit), 0.toShort)
- checkEvaluation(Divide(positiveIntLit, negativeIntLit), 0)
- checkEvaluation(Divide(positiveLongLit, negativeLongLit), 0L)
+ test("/ (Divide) for integral type") {
+ checkEvaluation(IntegralDivide(Literal(1.toByte), Literal(2.toByte)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toShort), Literal(2.toShort)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1), Literal(2)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toLong), Literal(2.toLong)), 0L)
+ checkEvaluation(IntegralDivide(positiveShortLit, negativeShortLit), 0L)
+ checkEvaluation(IntegralDivide(positiveIntLit, negativeIntLit), 0L)
+ checkEvaluation(IntegralDivide(positiveLongLit, negativeLongLit), 0L)
--- End diff --
Could you add a test case for `divide by zero` like `test("/ (Divide) basic")`?
For now, this PR seems to follow the behavior of Spark `/` instead of Hive `div`. We had better be clear on our decision and prevent future unintended behavior changes.
```scala
scala> sql("select 2 / 0, 2 div 0").show()
+---------------------------------------+---------+
|(CAST(2 AS DOUBLE) / CAST(0 AS DOUBLE))|(2 div 0)|
+---------------------------------------+---------+
| null| null|
+---------------------------------------+---------+
```
```sql
0: jdbc:hive2://ctr-e138-1518143905142-477481> select 2 / 0;
+-------+
| _c0 |
+-------+
| NULL |
+-------+
0: jdbc:hive2://ctr-e138-1518143905142-477481> select 2 div 0;
Error: Error while compiling statement: FAILED:
SemanticException [Error 10014]: Line 1:7 Wrong arguments '0':
org.apache.hadoop.hive.ql.metadata.HiveException: Unable to execute method public org.apache.hadoop.io.LongWritable org.apache.hadoop.hive.ql.udf.UDFOPLongDivide.evaluate(org.apache.hadoop.io.LongWritable,org.apache.hadoop.io.LongWritable) with arguments {2,0}:/ by zero (state=42000,code=10014)
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r216854603
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala ---
@@ -72,6 +72,7 @@ package object dsl {
def - (other: Expression): Expression = Subtract(expr, other)
def * (other: Expression): Expression = Multiply(expr, other)
def / (other: Expression): Expression = Divide(expr, other)
+ def div (other: Expression): Expression = IntegralDivide(expr, other)
--- End diff --
The failure looks like relevant.
```scala
org.scalatest.exceptions.TestFailedException:
Expected "struct<[CAST((CAST(5 AS DOUBLE) / CAST(2 AS DOUBLE)) AS BIGINT):big]int>",
but got "struct<[(5 div 2):]int>" Schema did not match for query #19 select 5 div 2
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96045 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96045/testReport)** for PR 22395 at commit [`02a2369`](https://github.com/apache/spark/commit/02a2369967b3b842b85cbf4ae1ce2bbbfb8817da).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217942351
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala ---
@@ -143,16 +143,14 @@ class ArithmeticExpressionSuite extends SparkFunSuite with ExpressionEvalHelper
}
}
- // By fixing SPARK-15776, Divide's inputType is required to be DoubleType of DecimalType.
- // TODO: in future release, we should add a IntegerDivide to support integral types.
- ignore("/ (Divide) for integral type") {
- checkEvaluation(Divide(Literal(1.toByte), Literal(2.toByte)), 0.toByte)
- checkEvaluation(Divide(Literal(1.toShort), Literal(2.toShort)), 0.toShort)
- checkEvaluation(Divide(Literal(1), Literal(2)), 0)
- checkEvaluation(Divide(Literal(1.toLong), Literal(2.toLong)), 0.toLong)
- checkEvaluation(Divide(positiveShortLit, negativeShortLit), 0.toShort)
- checkEvaluation(Divide(positiveIntLit, negativeIntLit), 0)
- checkEvaluation(Divide(positiveLongLit, negativeLongLit), 0L)
+ test("/ (Divide) for integral type") {
+ checkEvaluation(IntegralDivide(Literal(1.toByte), Literal(2.toByte)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toShort), Literal(2.toShort)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1), Literal(2)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toLong), Literal(2.toLong)), 0L)
+ checkEvaluation(IntegralDivide(positiveShortLit, negativeShortLit), 0L)
+ checkEvaluation(IntegralDivide(positiveIntLit, negativeIntLit), 0L)
+ checkEvaluation(IntegralDivide(positiveLongLit, negativeLongLit), 0L)
--- End diff --
good catch! We should clearly define the behavior in the doc string too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217087471
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "a _FUNC_ b - Divides a by b.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
--- End diff --
Shall we add this to `FunctionRegistry`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3162/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96040/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3151/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96141 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96141/testReport)** for PR 22395 at commit [`3550d29`](https://github.com/apache/spark/commit/3550d29f74e437ebc57b846d7b22f0cea254bd18).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96040 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96040/testReport)** for PR 22395 at commit [`02a2369`](https://github.com/apache/spark/commit/02a2369967b3b842b85cbf4ae1ce2bbbfb8817da).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3040/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217631634
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "expr1 _FUNC_ expr2 - Returns `expr1`/`expr2`. It performs integral division.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+
+ override def inputType: AbstractDataType = IntegralType
+
+ override def symbol: String = "/"
+ override def sqlOperator: String = "div"
+
+ private lazy val div: (Any, Any) => Any = dataType match {
+ case i: IntegralType => i.integral.asInstanceOf[Integral[Any]].quot
+ }
+ override def evalOperation(left: Any, right: Any): Any = div(left, right)
--- End diff --
Yeah, I think it is reasonable as that is what we defined: `Hive Long Division: 'DIV'` in `AstBuilder.scala`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/22395
Could we check the definition of div in MySQL? Is it the same as the one implemented in this PR?
https://dev.mysql.com/doc/refman/8.0/en/arithmetic-functions.html#operator_div
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3091/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/22395
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96079/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217747916
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,32 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "expr1 _FUNC_ expr2 - Returns `expr1`/`expr2`. It performs integral division.",
--- End diff --
yes, thanks @viirya, I am updating to that sentence
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22395
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217285630
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "a _FUNC_ b - Divides a by b.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
--- End diff --
@dongjoon-hyun because if we add it there, we can write: `select div(3, 2)`, which is not supported by Hive.
```
hive> select div(3, 2);
NoViableAltException(13@[])
at org.apache.hadoop.hive.ql.parse.HiveParser_SelectClauseParser.selectClause(HiveParser_SelectClauseParser.java:964)
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217177287
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "a _FUNC_ b - Divides a by b.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
--- End diff --
Ur, sorry, but why not? As @viirya suggested, without that, the description added here is not meaningless.
```scala
spark-sql> describe function 'div';
Function: div not found.
Time taken: 0.016 seconds, Fetched 1 row(s)
```
Also, Hive accepts that like the following.
```scala
0: jdbc:hive2://ctr-e138-1518143905142-429335> describe function div;
+----------------------------------------------------+
| tab_name |
+----------------------------------------------------+
| a div b - Divide a by b rounded to the long integer |
+----------------------------------------------------+
0: jdbc:hive2://ctr-e138-1518143905142-429335> select 3 / 2, 3 div 2, `/`(3,2), `div`(3,2);
+------+------+------+------+
| _c0 | _c1 | _c2 | _c3 |
+------+------+------+------+
| 1.5 | 1 | 1.5 | 1 |
+------+------+------+------+
1 row selected (0.288 seconds)
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/22395
thank you all for the reviews
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22395
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22395
Merged to the master.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #95995 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95995/testReport)** for PR 22395 at commit [`315bb86`](https://github.com/apache/spark/commit/315bb86dcbd97bd4b1371896f213b52cf7aee16f).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96114 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96114/testReport)** for PR 22395 at commit [`71255a1`](https://github.com/apache/spark/commit/71255a1787012baf2d5188991421e8197ec44733).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217098043
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "a _FUNC_ b - Divides a by b.",
--- End diff --
yes, thanks, I am very bad at descriptions.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96045/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #95954 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95954/testReport)** for PR 22395 at commit [`649b458`](https://github.com/apache/spark/commit/649b45875e81e47ba3282150b669f766fbe806ba).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class IntegralDivide(left: Expression, right: Expression) extends DivModLike `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22395
Retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/22395
@gatorsmile I checked on MySQL 5.6 and there are 2 differences between MySQL's `div` and the current implementation:
- MySQL returns an `int` if the div operands are integers, `bigint` if they are bigint, etc (as reported for Postgres and SQLServer in https://github.com/apache/spark/pull/22395#discussion_r217289728);
- MySQL accepts as operands also decimal, double, etc. (it is the only DB doing so).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/22395
Looks like a use case for a legacy config.
On Mon, Sep 17, 2018 at 6:41 PM Wenchen Fan <no...@github.com>
wrote:
> To clarify, it's not following hive, but following the behavior of
> previous Spark versions, which is same as hive.
>
> I also think returning left operand's type is more reasonable, but we
> should do it in another PR since it's a behavior change, and we should also
> add migration guide for it.
>
> @mgaido91 <https://github.com/mgaido91> do you have time to do this
> change? Thanks!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/spark/pull/22395#issuecomment-422225132>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AATvPDeW3F4Jsc-gS6CFrrGZY_lFXGxbks5ucE9WgaJpZM4Wjmfh>
> .
>
--
--
excuse the brevity and lower case due to wrist injury
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217388045
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "a _FUNC_ b - Divides a by b.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
--- End diff --
@mgaido91 . I gave the example of Hive in the above. :)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQ] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #95954 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95954/testReport)** for PR 22395 at commit [`649b458`](https://github.com/apache/spark/commit/649b45875e81e47ba3282150b669f766fbe806ba).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #95982 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95982/testReport)** for PR 22395 at commit [`a0c0849`](https://github.com/apache/spark/commit/a0c0849aaf9b1fd0a3f3e5fa079c51d89005f68c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217289728
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "expr1 _FUNC_ expr2 - Returns `expr1`/`expr2`. It performs integral division.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+
+ override def inputType: AbstractDataType = IntegralType
+
+ override def symbol: String = "/"
+ override def sqlOperator: String = "div"
+
+ private lazy val div: (Any, Any) => Any = dataType match {
+ case i: IntegralType => i.integral.asInstanceOf[Integral[Any]].quot
+ }
+ override def evalOperation(left: Any, right: Any): Any = div(left, right)
--- End diff --
Sure, so:
- Hive returns always long;
- Postgres and SQLServer don't have a `div` operator but they perform integral division when the operands are integrals and return the datatype of the operands (eg. `select 3 / 2` returns an integer);
- Oracle doesn't support it.
So the behavior is not homogeneous among the RDBMs
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22395
Retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQ] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22395
`[SQ]` -> `[SQL]` in the title?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217626193
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "expr1 _FUNC_ expr2 - Returns `expr1`/`expr2`. It performs integral division.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+
+ override def inputType: AbstractDataType = IntegralType
+
+ override def symbol: String = "/"
+ override def sqlOperator: String = "div"
+
+ private lazy val div: (Any, Any) => Any = dataType match {
+ case i: IntegralType => i.integral.asInstanceOf[Integral[Any]].quot
+ }
+ override def evalOperation(left: Any, right: Any): Any = div(left, right)
--- End diff --
+1 for @cloud-fan 's suggestion.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217977048
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala ---
@@ -143,16 +143,14 @@ class ArithmeticExpressionSuite extends SparkFunSuite with ExpressionEvalHelper
}
}
- // By fixing SPARK-15776, Divide's inputType is required to be DoubleType of DecimalType.
- // TODO: in future release, we should add a IntegerDivide to support integral types.
- ignore("/ (Divide) for integral type") {
- checkEvaluation(Divide(Literal(1.toByte), Literal(2.toByte)), 0.toByte)
- checkEvaluation(Divide(Literal(1.toShort), Literal(2.toShort)), 0.toShort)
- checkEvaluation(Divide(Literal(1), Literal(2)), 0)
- checkEvaluation(Divide(Literal(1.toLong), Literal(2.toLong)), 0.toLong)
- checkEvaluation(Divide(positiveShortLit, negativeShortLit), 0.toShort)
- checkEvaluation(Divide(positiveIntLit, negativeIntLit), 0)
- checkEvaluation(Divide(positiveLongLit, negativeLongLit), 0L)
+ test("/ (Divide) for integral type") {
+ checkEvaluation(IntegralDivide(Literal(1.toByte), Literal(2.toByte)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toShort), Literal(2.toShort)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1), Literal(2)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toLong), Literal(2.toLong)), 0L)
+ checkEvaluation(IntegralDivide(positiveShortLit, negativeShortLit), 0L)
+ checkEvaluation(IntegralDivide(positiveIntLit, negativeIntLit), 0L)
+ checkEvaluation(IntegralDivide(positiveLongLit, negativeLongLit), 0L)
--- End diff --
The test for this case is present in `operators.sql` (anyway, if you prefer me to add a case here too, just let me know and I'll add it). And since we already have this function in our code indeed - it is just translated to a normal divide + a cast - currently we are returning `null` and throwing an exception for it would be a behavior change (and a quite disruptive too). Do we really want to follow Hive's behavior on this?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #95982 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95982/testReport)** for PR 22395 at commit [`a0c0849`](https://github.com/apache/spark/commit/a0c0849aaf9b1fd0a3f3e5fa079c51d89005f68c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96045 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96045/testReport)** for PR 22395 at commit [`02a2369`](https://github.com/apache/spark/commit/02a2369967b3b842b85cbf4ae1ce2bbbfb8817da).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96114/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96128 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96128/testReport)** for PR 22395 at commit [`c471bef`](https://github.com/apache/spark/commit/c471bef5292016e942ffe2788760eda08e48783e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217981686
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala ---
@@ -143,16 +143,14 @@ class ArithmeticExpressionSuite extends SparkFunSuite with ExpressionEvalHelper
}
}
- // By fixing SPARK-15776, Divide's inputType is required to be DoubleType of DecimalType.
- // TODO: in future release, we should add a IntegerDivide to support integral types.
- ignore("/ (Divide) for integral type") {
- checkEvaluation(Divide(Literal(1.toByte), Literal(2.toByte)), 0.toByte)
- checkEvaluation(Divide(Literal(1.toShort), Literal(2.toShort)), 0.toShort)
- checkEvaluation(Divide(Literal(1), Literal(2)), 0)
- checkEvaluation(Divide(Literal(1.toLong), Literal(2.toLong)), 0.toLong)
- checkEvaluation(Divide(positiveShortLit, negativeShortLit), 0.toShort)
- checkEvaluation(Divide(positiveIntLit, negativeIntLit), 0)
- checkEvaluation(Divide(positiveLongLit, negativeLongLit), 0L)
+ test("/ (Divide) for integral type") {
+ checkEvaluation(IntegralDivide(Literal(1.toByte), Literal(2.toByte)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toShort), Literal(2.toShort)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1), Literal(2)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toLong), Literal(2.toLong)), 0L)
+ checkEvaluation(IntegralDivide(positiveShortLit, negativeShortLit), 0L)
+ checkEvaluation(IntegralDivide(positiveIntLit, negativeIntLit), 0L)
+ checkEvaluation(IntegralDivide(positiveLongLit, negativeLongLit), 0L)
--- End diff --
I think we don't really need to change current behavior, but it is worth describing this in the doc string.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96040 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96040/testReport)** for PR 22395 at commit [`02a2369`](https://github.com/apache/spark/commit/02a2369967b3b842b85cbf4ae1ce2bbbfb8817da).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #95995 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95995/testReport)** for PR 22395 at commit [`315bb86`](https://github.com/apache/spark/commit/315bb86dcbd97bd4b1371896f213b52cf7aee16f).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3050/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217747363
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,32 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "expr1 _FUNC_ expr2 - Returns `expr1`/`expr2`. It performs integral division.",
--- End diff --
`Divide a by b rounded to the long integer`, this is Hive's `div` document.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96141 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96141/testReport)** for PR 22395 at commit [`3550d29`](https://github.com/apache/spark/commit/3550d29f74e437ebc57b846d7b22f0cea254bd18).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96114 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96114/testReport)** for PR 22395 at commit [`71255a1`](https://github.com/apache/spark/commit/71255a1787012baf2d5188991421e8197ec44733).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22395
LGTM except one comment
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217576598
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "expr1 _FUNC_ expr2 - Returns `expr1`/`expr2`. It performs integral division.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+
+ override def inputType: AbstractDataType = IntegralType
+
+ override def symbol: String = "/"
+ override def sqlOperator: String = "div"
+
+ private lazy val div: (Any, Any) => Any = dataType match {
+ case i: IntegralType => i.integral.asInstanceOf[Integral[Any]].quot
+ }
+ override def evalOperation(left: Any, right: Any): Any = div(left, right)
--- End diff --
Then I'd prefer always returning long, since it was the behavior before. We can consider changing the behavior in another PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22395
LGTM, cc @viirya @gatorsmile
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217982791
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala ---
@@ -143,16 +143,14 @@ class ArithmeticExpressionSuite extends SparkFunSuite with ExpressionEvalHelper
}
}
- // By fixing SPARK-15776, Divide's inputType is required to be DoubleType of DecimalType.
- // TODO: in future release, we should add a IntegerDivide to support integral types.
- ignore("/ (Divide) for integral type") {
- checkEvaluation(Divide(Literal(1.toByte), Literal(2.toByte)), 0.toByte)
- checkEvaluation(Divide(Literal(1.toShort), Literal(2.toShort)), 0.toShort)
- checkEvaluation(Divide(Literal(1), Literal(2)), 0)
- checkEvaluation(Divide(Literal(1.toLong), Literal(2.toLong)), 0.toLong)
- checkEvaluation(Divide(positiveShortLit, negativeShortLit), 0.toShort)
- checkEvaluation(Divide(positiveIntLit, negativeIntLit), 0)
- checkEvaluation(Divide(positiveLongLit, negativeLongLit), 0L)
+ test("/ (Divide) for integral type") {
+ checkEvaluation(IntegralDivide(Literal(1.toByte), Literal(2.toByte)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toShort), Literal(2.toShort)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1), Literal(2)), 0L)
+ checkEvaluation(IntegralDivide(Literal(1.toLong), Literal(2.toLong)), 0L)
+ checkEvaluation(IntegralDivide(positiveShortLit, negativeShortLit), 0L)
+ checkEvaluation(IntegralDivide(positiveIntLit, negativeIntLit), 0L)
+ checkEvaluation(IntegralDivide(positiveLongLit, negativeLongLit), 0L)
--- End diff --
I agree with you @viirya. I updated the doc string with the current behavior. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22395
Thank you, @mgaido91 !
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22395
@rxin . We made a decision to follow Hive behavior [here](https://github.com/apache/spark/pull/22395#discussion_r217289728) .
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3140/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217390828
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "a _FUNC_ b - Divides a by b.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
--- End diff --
ah, sorry I missed the back-ticks. I am adding it, sorry. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217088230
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "a _FUNC_ b - Divides a by b.",
--- End diff --
nit: explicitly say this is integral divide?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r218055871
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,34 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "expr1 _FUNC_ expr2 - Divide `expr1` by `expr2` rounded to the long integer. It returns NULL if an operand is NULL or `expr2` is 0.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
--- End diff --
the next version will be 2.5.0 AFAIK.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3117/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96072/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3113/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3088/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96141/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22395
LGTM, cc @viirya @gatorsmile
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96128/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQ] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3020/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96079 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96079/testReport)** for PR 22395 at commit [`71255a1`](https://github.com/apache/spark/commit/71255a1787012baf2d5188991421e8197ec44733).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/22395
Sure @cloud-fan, I'll create a JIRA and submit a PR for it.
> Looks like a use case for a legacy config.
Yes, thanks for the suggestion @rxin, I agree.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96072 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96072/testReport)** for PR 22395 at commit [`fca5e62`](https://github.com/apache/spark/commit/fca5e62d5b0b8c7a9123c8bab638f955a754e197).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22395
To clarify, it's not following hive, but following the behavior of previous Spark versions, which is same as hive.
I also think returning left operand's type is more reasonable, but we should do it in another PR since it's a behavior change, and we should also add migration guide for it.
@mgaido91 do you have time to do this change? Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217743678
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,32 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "expr1 _FUNC_ expr2 - Returns `expr1`/`expr2`. It performs integral division.",
--- End diff --
Let's mention that it always return long. Maybe we can take a look at how Hive document it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95982/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22395
**[Test build #96072 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96072/testReport)** for PR 22395 at commit [`fca5e62`](https://github.com/apache/spark/commit/fca5e62d5b0b8c7a9123c8bab638f955a754e197).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQ] Add IntegralDivide expression
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217233033
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "expr1 _FUNC_ expr2 - Returns `expr1`/`expr2`. It performs integral division.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+
+ override def inputType: AbstractDataType = IntegralType
+
+ override def symbol: String = "/"
+ override def sqlOperator: String = "div"
+
+ private lazy val div: (Any, Any) => Any = dataType match {
+ case i: IntegralType => i.integral.asInstanceOf[Integral[Any]].quot
+ }
+ override def evalOperation(left: Any, right: Any): Any = div(left, right)
--- End diff --
Sorry I may not recall it very clearly. Can you check Hive and other databases and see if the result type of `div` is always long?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/22395
why are we always returning long type here? shouldn't they be the same as the left expr's type? see mysql
```mysql> create temporary table rxin_temp select 4 div 2, 123456789124 div 2, 4 / 2, 123456789124 / 2;
Query OK, 1 row affected (0.02 sec)
Records: 1 Duplicates: 0 Warnings: 0
mysql> describe rxin_temp;
+--------------------+---------------+------+-----+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+--------------------+---------------+------+-----+---------+-------+
| 4 div 2 | int(1) | YES | | NULL | |
| 123456789124 div 2 | bigint(12) | YES | | NULL | |
| 4 / 2 | decimal(5,4) | YES | | NULL | |
| 123456789124 / 2 | decimal(16,4) | YES | | NULL | |
+--------------------+---------------+------+-----+---------+-------+
4 rows in set (0.01 sec)```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22395: [SPARK-16323][SQL] Add IntegralDivide expression
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22395#discussion_r217097186
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -314,6 +314,27 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
override def evalOperation(left: Any, right: Any): Any = div(left, right)
}
+@ExpressionDescription(
+ usage = "a _FUNC_ b - Divides a by b.",
+ examples = """
+ Examples:
+ > SELECT 3 _FUNC_ 2;
+ 1
+ """,
+ since = "3.0.0")
+case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
--- End diff --
I don't think so, please see the discussion at https://github.com/apache/spark/pull/14036#discussion_r69392061
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org