[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/13651 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r67225998
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala
---
@@ -199,9 +201,18 @@ class TypeCoercionSuite extends PlanTest {
}
private def ruleTest(rule: Rule[LogicalPlan], initial: Expression,
transformed: Expression) {
+ruleTest(Seq(rule), initial, transformed)
+ }
+
+ private def ruleTest(
+ rules: Seq[Rule[LogicalPlan]], initial: Expression, transformed:
Expression): Unit = {
--- End diff --
NIT: one argument per line
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r67223826
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala
---
@@ -345,4 +345,42 @@ class AnalysisSuite extends AnalysisTest {
assertAnalysisSuccess(query)
}
+
+ test("SPARK-15776: test whether Divide expression's data type can be
deduced correctly by " +
+"analyzer") {
+val testRelation = LocalRelation(AttributeReference("a",
IntegerType)())
+assertSameSchema(
--- End diff --
How about defining a tester function, and use that for the rest of the
testing? Something like:
```scala
def assertDivideReturnType(left: Expression, right: Expression, dt:
DataType): Unit = {
assertSameSchema(
Project(Seq(Alias(sum(Divide(left, right)), "a")()), OneRowRelation),
StructType(Seq(StructField("a", dt, true
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r67203936
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala
---
@@ -128,12 +133,27 @@ class ArithmeticExpressionSuite extends SparkFunSuite
with ExpressionEvalHelper
checkEvaluation(Divide(left, Literal(convert(0))), null) // divide
by zero
}
-DataTypeTestUtils.numericTypeWithoutDecimal.foreach { tpe =>
+Seq(DoubleType, DecimalType.SYSTEM_DEFAULT).foreach { tpe =>
checkConsistencyBetweenInterpretedAndCodegen(Divide, tpe, tpe)
}
}
- test("/ (Divide) for integral type") {
+ test("SPARK-15776: Divide report unresolved when children's data type is
" +
--- End diff --
This suite is to test the correctness, not resolution.
BTW, the resolution logic is defined by `ExpectedInputTypes`, I think we
don't need to test it again for divide.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r67052047
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala
---
@@ -630,6 +640,25 @@ class TypeCoercionSuite extends PlanTest {
Seq(Cast(Literal(1), StringType), Cast(Literal("b"), StringType)))
)
}
+
+ test("SPARK-15776 Divide expression's dataType should be casted to
Double or Decimal") {
+val analyzer = new RuleExecutor[LogicalPlan] {
+ override val batches =
+Seq(Batch("Resolution", FixedPoint(10),
FunctionArgumentConversion, Division))
+}
+
+// Cast integer to double
+ruleTest(analyzer, sum(Divide(4, 3)), sum(Divide(Cast(4, DoubleType),
Cast(3, DoubleType
+// left expression is already Double, skip
--- End diff --
what if left expression is int and right expression is double?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user yhuai commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r67011472
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -213,7 +213,7 @@ case class Multiply(left: Expression, right: Expression)
case class Divide(left: Expression, right: Expression)
extends BinaryArithmetic with NullIntolerant {
- override def inputType: AbstractDataType = NumericType
--- End diff --
Looks like div is from mysql (see
http://dev.mysql.com/doc/refman/5.7/en/arithmetic-functions.html).
```
DIV Integer division
/ Division operator
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user yhuai commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r67008784
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
---
@@ -2847,4 +2847,15 @@ class SQLQuerySuite extends QueryTest with
SharedSQLContext {
test("SPARK-15887: hive-site.xml should be loaded") {
assert(spark.sessionState.newHadoopConf().get("hive.in.test") ==
"true")
}
+
+ test("SPARK-15776 Divide expression inside an Aggregation function
should not " +
--- End diff --
TypeCoercionSuite
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user yhuai commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r67008385
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala
---
@@ -525,7 +525,7 @@ object TypeCoercion {
def apply(plan: LogicalPlan): LogicalPlan = plan resolveExpressions {
// Skip nodes who has not been resolved yet,
// as this is an extra rule which should be applied at last.
- case e if !e.resolved => e
+ case e if !e.childrenResolved => e
// Decimal and Double remain the same
--- End diff --
Seems `case d: Divide if d.dataType.isInstanceOf[IntegralType]` is not
equivalent with the code that we have at here?
(also , let's avoid of unnecessary changes)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user clockfly commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66885516
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -213,7 +213,7 @@ case class Multiply(left: Expression, right: Expression)
case class Divide(left: Expression, right: Expression)
extends BinaryArithmetic with NullIntolerant {
- override def inputType: AbstractDataType = NumericType
--- End diff --
@hvanhovell Thanks for the catch. I will try to test hive behavior more,
like how `select 5 div 3.0` behaves in Hive.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66883329
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala
---
@@ -525,7 +525,7 @@ object TypeCoercion {
def apply(plan: LogicalPlan): LogicalPlan = plan resolveExpressions {
// Skip nodes who has not been resolved yet,
// as this is an extra rule which should be applied at last.
- case e if !e.resolved => e
+ case e if !e.childrenResolved => e
// Decimal and Double remain the same
--- End diff --
or we can remove this rule entirely, the `ImplicitTypeCasts` can already
handle this
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user clockfly commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66882617
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
---
@@ -2847,4 +2847,15 @@ class SQLQuerySuite extends QueryTest with
SharedSQLContext {
test("SPARK-15887: hive-site.xml should be loaded") {
assert(spark.sessionState.newHadoopConf().get("hive.in.test") ==
"true")
}
+
+ test("SPARK-15776 Divide expression inside an Aggregation function
should not " +
--- End diff --
@cloud-fan
It involves Analyzer, low level unit test is hard to cover that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66882340
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -213,7 +213,7 @@ case class Multiply(left: Expression, right: Expression)
case class Divide(left: Expression, right: Expression)
extends BinaryArithmetic with NullIntolerant {
- override def inputType: AbstractDataType = NumericType
--- End diff --
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user clockfly commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66881813
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -213,7 +213,7 @@ case class Multiply(left: Expression, right: Expression)
case class Divide(left: Expression, right: Expression)
extends BinaryArithmetic with NullIntolerant {
- override def inputType: AbstractDataType = NumericType
+ override def inputType: AbstractDataType = TypeCollection(DoubleType,
DecimalType)
--- End diff --
Float is casted to Double.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66881454
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -213,7 +213,7 @@ case class Multiply(left: Expression, right: Expression)
case class Divide(left: Expression, right: Expression)
extends BinaryArithmetic with NullIntolerant {
- override def inputType: AbstractDataType = NumericType
--- End diff --
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66881195
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -213,7 +213,7 @@ case class Multiply(left: Expression, right: Expression)
case class Divide(left: Expression, right: Expression)
extends BinaryArithmetic with NullIntolerant {
- override def inputType: AbstractDataType = NumericType
+ override def inputType: AbstractDataType = TypeCollection(DoubleType,
DecimalType)
--- End diff --
Shall we use `FractionalType`, which also includes `FloatType` here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66880904
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -213,7 +213,7 @@ case class Multiply(left: Expression, right: Expression)
case class Divide(left: Expression, right: Expression)
extends BinaryArithmetic with NullIntolerant {
- override def inputType: AbstractDataType = NumericType
--- End diff --
Shouldn't we add a dedicated expression for integral division? We currently
support `DIV` in our parser:
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala#L954
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66880282
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala
---
@@ -525,7 +525,7 @@ object TypeCoercion {
def apply(plan: LogicalPlan): LogicalPlan = plan resolveExpressions {
// Skip nodes who has not been resolved yet,
// as this is an extra rule which should be applied at last.
- case e if !e.resolved => e
+ case e if !e.childrenResolved => e
// Decimal and Double remain the same
--- End diff --
We can simplify this:
```
case e if !e.childrenResolved => e
case d: Divide if d.dataType.isInstanceOf[IntegralType] => ...
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66879827
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
---
@@ -213,7 +213,7 @@ case class Multiply(left: Expression, right: Expression)
case class Divide(left: Expression, right: Expression)
extends BinaryArithmetic with NullIntolerant {
- override def inputType: AbstractDataType = NumericType
--- End diff --
we should also cleanup the `divide` expression to remove code for integral
division.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/13651#discussion_r66879738
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
---
@@ -2847,4 +2847,15 @@ class SQLQuerySuite extends QueryTest with
SharedSQLContext {
test("SPARK-15887: hive-site.xml should be loaded") {
assert(spark.sessionState.newHadoopConf().get("hive.in.test") ==
"true")
}
+
+ test("SPARK-15776 Divide expression inside an Aggregation function
should not " +
--- End diff --
I think we need some low level unit test instead of end-to-end test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #13651: [SPARK-15776][SQL] Divide Expression inside Aggre...
GitHub user clockfly opened a pull request:
https://github.com/apache/spark/pull/13651
[SPARK-15776][SQL] Divide Expression inside Aggregation function is casted
to wrong type
## What changes were proposed in this pull request?
This PR fixes the problem that Divide Expression inside Aggregation
function is casted to wrong type. After the fix, the behavior is consistent
with Hive.
**Before the change:**
```
scala> sql("select sum(1 / 2) as a").schema
res4: org.apache.spark.sql.types.StructType =
StructType(StructField(a,LongType,true))
scala> sql("select sum(1 / 2) as a").show()
+---+
| a|
+---+
|0 |
+---+
```
**After the change:**
```
scala> sql("select sum(1 / 2) as a").schema
res4: org.apache.spark.sql.types.StructType =
StructType(StructField(a,DoubleType,true))
scala> sql("select sum(1 / 2) as a").show()
+---+
| a|
+---+
|0.5|
+---+
```
## How was this patch tested?
Unit test.
This PR is based on https://github.com/apache/spark/pull/13524 by
@Sephiroth-Lin
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/clockfly/spark SPARK-15776
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/13651.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 #13651
commit df08eeacd85187ca5a71463fc5d25f63426ebe84
Author: Sean Zhong
Date: 2016-06-13T22:09:20Z
SPARK-15776 Divide Expression inside an Aggregation function is casted to
wrong type
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
