You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/02/18 20:40:38 UTC
[GitHub] [spark] skambha opened a new pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
skambha opened a new pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627
### What changes were proposed in this pull request?
JIRA SPARK-28067: Wrong results are returned for aggregate sum with decimals with whole stage codegen enabled
**Repro:**
WholeStage enabled enabled -> Wrong results
WholeStage disabled -> Returns exception Decimal precision 39 exceeds max precision 38
**Issues:**
1. Wrong results are returned which is bad
2. Inconsistency between whole stage enabled and disabled.
**Cause:**
Sum does not take care of possibility of overflow for the intermediate steps. ie the updateExpressions and mergeExpressions.
This PR makes the following changes:
- Add changes to check if overflow occurs for decimal in aggregate Sum and if there is an overflow, it will return null for the Sum operation when spark.sql.ansi.enabled is false.
- When spark.sql.ansi.enabled is true, then the sum operation will return an exception if an overflow occurs for the decimal operation in Sum.
- This is keeping it consistent with the behavior defined in spark.sql.ansi.enabled property
**Before the fix: Scenario 1:** - WRONG RESULTS
```
scala> val df = Seq(
| (BigDecimal("10000000000000000000"), 1),
| (BigDecimal("10000000000000000000"), 1),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2)).toDF("decNum", "intNum")
df: org.apache.spark.sql.DataFrame = [decNum: decimal(38,18), intNum: int]
scala> val df2 = df.withColumnRenamed("decNum", "decNum2").join(df, "intNum").agg(sum("decNum"))
df2: org.apache.spark.sql.DataFrame = [sum(decNum): decimal(38,18)]
scala> df2.show(40,false)
+---------------------------------------+
|sum(decNum) |
+---------------------------------------+
|20000000000000000000.000000000000000000|
+---------------------------------------+
```
--
**Before fix: Scenario2: Setting spark.sql.ansi.enabled to true** - WRONG RESULTS
```
scala> spark.conf.set("spark.sql.ansi.enabled", "true")
scala> val df = Seq(
| (BigDecimal("10000000000000000000"), 1),
| (BigDecimal("10000000000000000000"), 1),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2)).toDF("decNum", "intNum")
df: org.apache.spark.sql.DataFrame = [decNum: decimal(38,18), intNum: int]
scala> val df2 = df.withColumnRenamed("decNum", "decNum2").join(df, "intNum").agg(sum("decNum"))
df2: org.apache.spark.sql.DataFrame = [sum(decNum): decimal(38,18)]
scala> df2.show(40,false)
+---------------------------------------+
|sum(decNum) |
+---------------------------------------+
|20000000000000000000.000000000000000000|
+---------------------------------------+
```
**After the fix: Scenario1:**
```
scala> val df = Seq(
| (BigDecimal("10000000000000000000"), 1),
| (BigDecimal("10000000000000000000"), 1),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2)).toDF("decNum", "intNum")
df: org.apache.spark.sql.DataFrame = [decNum: decimal(38,18), intNum: int]
scala> val df2 = df.withColumnRenamed("decNum", "decNum2").join(df, "intNum").agg(sum("decNum"))
df2: org.apache.spark.sql.DataFrame = [sum(decNum): decimal(38,18)]
scala> df2.show(40,false)
+-----------+
|sum(decNum)|
+-----------+
|null |
+-----------+
```
**After fix: Scenario2: Setting the spark.sql.ansi.enabled to true:**
```
scala> spark.conf.set("spark.sql.ansi.enabled", "true")
scala> val df = Seq(
| (BigDecimal("10000000000000000000"), 1),
| (BigDecimal("10000000000000000000"), 1),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2),
| (BigDecimal("10000000000000000000"), 2)).toDF("decNum", "intNum")
df: org.apache.spark.sql.DataFrame = [decNum: decimal(38,18), intNum: int]
scala> val df2 = df.withColumnRenamed("decNum", "decNum2").join(df, "intNum").agg(sum("decNum"))
df2: org.apache.spark.sql.DataFrame = [sum(decNum): decimal(38,18)]
scala> df2.show(40,false)
20/02/18 10:57:43 ERROR Executor: Exception in task 5.0 in stage 4.0 (TID 30)
java.lang.ArithmeticException: Decimal(expanded,100000000000000000000.000000000000000000,39,18}) cannot be represented as Decimal(38, 18).
```
### Why are the changes needed?
The changes are needed in order to fix the wrong results that are returned for decimal aggregate sum.
### Does this PR introduce any user-facing change?
User would see wrong results on aggregate sum that involved decimal overflow prior to this change, but now the user will see null. But if user enables the spark.sql.ansi.enabled flag to true, then the user will see an exception and not incorrect results.
### How was this patch tested?
New test has been added and existing tests for sql, catalyst and hive suites were run ok.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r408623119
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/DecimalSum.scala
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions.aggregate
+
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types._
+
+case class DecimalSum(child: Expression) extends DeclarativeAggregate with ImplicitCastInputTypes {
Review comment:
Please see the comment https://github.com/apache/spark/pull/27627#discussion_r408622772
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613859694
@maropu , There is more work happening now for decimal type with the checks. Yes, let me run some tests to measure the impact.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611367094
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r406019307
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/DecimalSum.scala
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions.aggregate
+
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types._
+
+case class DecimalSum(child: Expression) extends DeclarativeAggregate with ImplicitCastInputTypes {
+
+ override def children: Seq[Expression] = child :: Nil
+
+ override def nullable: Boolean = true
+
+ // Return data type.
+ override def dataType: DataType = resultType
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(NumericType)
+
+ private lazy val resultType = child.dataType match {
+ case DecimalType.Fixed(precision, scale) =>
+ DecimalType.bounded(precision + 10, scale)
+ case _: IntegralType => LongType
+ case _ => DoubleType
+ }
+
+ private lazy val sumDataType = resultType
+
+ private lazy val sum = AttributeReference("sum", sumDataType)()
+ private lazy val overflow = AttributeReference("overflow", BooleanType, false)()
+
+ private lazy val zero = Literal.default(resultType)
+
+ override lazy val aggBufferAttributes = sum :: overflow :: Nil
+
+ override lazy val initialValues: Seq[Expression] = Seq(
+ /* sum = */ Literal.create(null, sumDataType),
+ /* overflow = */ Literal.create(false, BooleanType)
+ )
+
+ override lazy val updateExpressions: Seq[Expression] = {
+ if (child.nullable) {
+ resultType match {
+ case d: DecimalType =>
+ Seq(
+ If(overflow, Literal.create(null, sumDataType), coalesce(
+ CheckOverflow(coalesce(sum, zero) + child.cast(sumDataType), d, true), sum)),
Review comment:
I don't think we should check overflow at every step. For example, we may add a big value which makes the sum overflow, then add a big negative value and we are not overflow afer that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-597997273
>How about we add a new expression DecimalSum?
Sure we can do that, if that is the preferred approach. I'll look into it
IIUC, you are proposing that we add a new DecimalSum aggregate function and this would be just for input type decimal. Is the expectation that the user would now call this from their queries for their decimal sum operations? If so, how would we like to handle the issue when user uses sum for their decimal columns.
--
>4. the mergeExpression should do:
Won't we also need to check for overflow in here as well. for e.g sum.left and sum.right are added. None of them overflowed when doing the update expressions, but when you add them during the merge the combined value overflows.
Thanks @cloud-fan
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613912451
> So if we have 2 null rows, the expected result should be null and not zero.
> We need a way to distinguish between valid null rows and null value that come because of a overflow as well.
AFAIK most aggregate functions just skip null values, and `SUM` will only have null value if: 1. overflow happens. 2: no inputs.
It's much easier to set the `isEmpty` flag, but hard to set the `isOverflow` flag (you added 2 new expressions)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611287758
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r408622772
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -240,6 +240,7 @@ class Analyzer(
ExtractWindowExpressions ::
GlobalAggregates ::
ResolveAggregateFunctions ::
+ UseDecimalSum ::
Review comment:
Thanks @maropu for your comments. Based on the discussion here https://github.com/apache/spark/pull/27627#issuecomment-611308104, the changes are now back only in Sum, which removes the necessity of this rule.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613856116
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25986/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595067541
find a way to reproduce without join
```
scala> val decimalStr = "1" + "0" * 19
decimalStr: String = 10000000000000000000
scala> val df = spark.range(0, 12, 1, 1)
df: org.apache.spark.sql.Dataset[Long] = [id: bigint]
scala> df.select(expr(s"cast('$decimalStr' as decimal (38, 18)) as d")).agg(sum($"d")).show
// This is correct
+------+
|sum(d)|
+------+
| null|
+------+
scala> val df = spark.range(0, 1, 1, 1).union(spark.range(0, 11, 1, 1))
df: org.apache.spark.sql.Dataset[Long] = [id: bigint]
scala> df.select(expr(s"cast('$decimalStr' as decimal (38, 18)) as d")).agg(sum($"d")).show
// This is wrong
+--------------------+
| sum(d)|
+--------------------+
|10000000000000000...|
+--------------------+
```
I think the root cause is, `sum` in partial aggregate overflows and write null to the unsafe row. `sum` in final aggregate reads null from the unsafe row and mistakenly think it's caused by empty data and convert it to 0.
We should create a `DecimalSum`, which use 2 buffer attributes: `sum` and `isEmpty`. Then in final aggregate we can check the `isEmpty` flag to konw if the null is caused by overflow or empty data.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-589530921
> Sum does not take care of possibility of overflow for the intermediate steps. ie the updateExpressions and mergeExpressions.
I'm a little confused. These expressions are used in non-whole-stage-codegen as well, why only whole-stage-codegen has the problem?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595906474
Yes. I agree. E.g ` " agg_value_3 = agg_value_4.$plus(agg_expr_0_0);" ` This can overflow and hold the value but not when it writes out to the UnsafeRow.
>Can someone investigate it further and see why spark.range(0, 12, 1, 1) works?
Yes. I will look into this. If someone else has suggestions please do share.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan edited a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613912451
> So if we have 2 null rows, the expected result should be null and not zero.
> We need a way to distinguish between valid null rows and null value that come because of a overflow as well.
AFAIK most aggregate functions just skip null values, and `SUM` will only return null if: 1. overflow happens. 2: no inputs.
It's much easier to set the `isEmpty` flag, but hard to set the `isOverflow` flag (you added 2 new expressions)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-596863783
I looked into why the spark.range(0,12,1,1) works and the other scenario returns wrong results
that is mentioned in https://github.com/apache/spark/pull/27627#issuecomment-595067541
**Case 1** that used spark.range(0,12,1,1) - this returns null.
Please see this file for details. https://github.com/skambha/notes/blob/master/case1_works_notes.txt
There is only one partition and 1 wholestagecodegen subtree. There is no intermediate writing out via UnsafeRowWriter. Hence all the values are added up which ends up being a value that is a overflow value and then the check overflow code kicks in, that sees that is is a overflow value and it will turn it to a null. The null then gets written out.
1. Partition is 1.
2. This has 1 WholeStageCodegen subtrees.
3. So the updateExpression codegen code will do the add ie +, for all the 12 decimal values and then it _**does not**_ write it out to UnsafeRow via UnsafeRowWriter
This will amount to `120000000000000000000.000000000000000000 `which is overflowed value.
4. This then goes to the CheckOverflow code which checks the decimal value with precision and it overflows and this call will evaluate to null.
```
/* 239 */ agg_value_1 = agg_mutableStateArray_0[0].toPrecision(
/* 240 */ 38, 18, Decimal.ROUND_HALF_UP(), true);
```
5. The null is then written out via UnsafeRowWriter.write call below.
```
/* 250 */ if (agg_isNull_1) {
/* 251 */ range_mutableStateArray_0[3].write(0, (Decimal) null, 38, 18);
```
This explains why this scenario works fine.
**Case 2** that used spark.range(0, 1, 1, 1).union(spark.range(0, 11, 1, 1)) and got wrong results
Please see the file for details:
https://github.com/skambha/notes/blob/master/case2_wrongresults.txt
1. Wholestagecodegen has 4 subtrees.
2. NumPartitions is 2
3. 11 rows in one partition and 1 row in another partition
4. For the partition with 11 rows, the 11 rows add up to a value that is `110000000000000000000.000000000000000000 `which is not containable in dec(38,18) and in UnsafeRowWriter, this will get written out as `null`.
5. For the partition that has 1 row, the sum value is `10000000000000000000.000000000000000000` which is containable.
6. Next, it does a merge of the two a) null and b) `10000000000000000000` using the coalesce and + expression in mergeExpressions and then that value is checked for overflow or not. This value is containable, so it returns the wrong result.
(Sidenote: If you split the range to 2 and 11 elements, you can see the result is `20000000000000000000.000000000000000000`)
---
We already saw that when we write out to UnsafeRow an overflow value of decimal, it will write a null value.
So, whether the wrong results repros or not is basically
- a combination of the values in each partition and when the sum of the values in that partition would become a overflow value **and** if that would end up being written out via UnsafeRowWriter, then that partition's sum will result being a **null,** **and**
- then in the subsequent execution, when the values for each partition are sum'd for merge phase, if the resultant value is a overflow value or not.
- If it is a overflow value, then due to the overflow check( coming from evaluateExpression), the resultant value will become null and then gets written out.
- If it is not a overflow value, the resultant value(which could be wrong) will get written out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-591185451
Thanks @cloud-fan and @kiszk for your comments.
> These expressions are used in non-whole-stage-codegen as well, why only whole-stage-codegen has the problem?
In case of the whole stage disabled, the exception for the error comes from here.
AggregationIterator —> .. JoinedRow —> UnsafeRow -> Decimal.set where it checks if the value can fit within the precision and scale.
```
Caused by: java.lang.ArithmeticException: Decimal precision 39 exceeds max precision 38
at org.apache.spark.sql.types.Decimal.set(Decimal.scala:122)
at org.apache.spark.sql.types.Decimal$.apply(Decimal.scala:574)
at org.apache.spark.sql.types.Decimal.apply(Decimal.scala)
at org.apache.spark.sql.catalyst.expressions.UnsafeRow.getDecimal(UnsafeRow.java:385)
at org.apache.spark.sql.catalyst.expressions.JoinedRow.getDecimal(JoinedRow.scala:95)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$SpecificMutableProjection.apply(Unknown Source)
at org.apache.spark.sql.execution.aggregate.AggregationIterator.$anonfun$generateProcessRow$7(AggregationIterator.scala:209)
at org.apache.spark.sql.execution.aggregate.AggregationIterator.$anonfun$generateProcessRow$7$adapted(AggregationIterator.scala:207)
at org.apache.spark.sql.execution.aggregate.TungstenAggregationIterator.processInputs(TungstenAggregationIterator.scala:187)
at org.apache.spark.sql.execution.aggregate.TungstenAggregationIterator.<init>(TungstenAggregationIterator.scala:362)
at org.apache.spark.sql.execution.aggregate.HashAggregateExec.$anonfun$doExecute$2(HashAggregateExec.scala:136)
at org.apache.spark.sql.execution.aggregate.HashAggregateExec.$anonfun$doExecute$2$adapted(HashAggregateExec.scala:111)
at org.apache.spark.rdd.RDD.$anonfun$mapPartitionsWithIndex$2(RDD.scala:889)
at org.apache.spark.rdd.RDD.$anonfun$mapPartitionsWithIndex$2$adapted(RDD.scala:889)
at org.apache.spark.rdd.MapPartitionsRDD.compute(MapPartitionsRDD.scala:52)
at org.apache.spark.rdd.RDD.computeOrReadCheckpoint(RDD.scala:349)
at org.apache.spark.rdd.RDD.iterator(RDD.scala:313)
at org.apache.spark.rdd.MapPartitionsRDD.compute(MapPartitionsRDD.scala:52)
at org.apache.spark.rdd.RDD.computeOrReadCheckpoint(RDD.scala:349)
at org.apache.spark.rdd.RDD.iterator(RDD.scala:313)
at org.apache.spark.shuffle.ShuffleWriteProcessor.write(ShuffleWriteProcessor.scala:59)
at org.apache.spark.scheduler.ShuffleMapTask.runTask(ShuffleMapTask.scala:99)
at org.apache.spark.scheduler.ShuffleMapTask.runTask(ShuffleMapTask.scala:52)
at org.apache.spark.scheduler.Task.run(Task.scala:127)
at org.apache.spark.executor.Executor$TaskRunner.$anonfun$run$3(Executor.scala:444)
at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:1377)
at org.apache.spark.executor.Executor$TaskRunner.run(Executor.scala:447)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
```
———
In case of the whole stage, here is the codegen that is generated for it (spark today, so no changes from this pr).
a) [codegen agg sum - all default (ansi mode false)](https://github.com/skambha/notes/blob/master/spark28067_ansifalse_wholestagetrue.txt)
b) [codegen agg sum - with ansi mode true](https://github.com/skambha/notes/blob/master/spark28067_ansitrue_wholestagetrue.txt)
In case of the whole stage codegen, you can see the decimal[ ‘+’ expressions ]( https://github.com/skambha/notes/blob/master/spark28067_ansifalse_wholestagetrue.txt#L164
)will at some point be larger than what can be contained in dec 38,18 but it gets written out as null. This messes up the end result of the sum and you get wrong results.
The decimal values computed from the + expressions are written using the UnsafeRowWriter.write https://github.com/apache/spark/blob/master/sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/UnsafeRowWriter.java#L184
Here is a snippet highlighting the behavior observed for the usecase in this issue: https://github.com/skambha/notes/blob/master/UnsafeRowWriterTestSnippet
The relevant code is here: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/UnsafeRowWriter.java#L202
This is triggered in the whole stage codegen path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r387056495
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Sum.scala
##########
@@ -60,38 +60,104 @@ case class Sum(child: Expression) extends DeclarativeAggregate with ImplicitCast
private lazy val sumDataType = resultType
private lazy val sum = AttributeReference("sum", sumDataType)()
+ private lazy val overflow = AttributeReference("overflow", BooleanType, false)()
private lazy val zero = Literal.default(resultType)
- override lazy val aggBufferAttributes = sum :: Nil
+ override lazy val aggBufferAttributes = sum :: overflow :: Nil
override lazy val initialValues: Seq[Expression] = Seq(
- /* sum = */ Literal.create(null, sumDataType)
+ /* sum = */ Literal.create(null, sumDataType),
+ /* overflow = */ Literal.create(false, BooleanType)
)
override lazy val updateExpressions: Seq[Expression] = {
- if (child.nullable) {
+ if (!SQLConf.get.ansiEnabled) {
+ if (child.nullable) {
+ Seq(
+ /* sum = */
+ coalesce(coalesce(sum, zero) + child.cast(sumDataType), sum),
+ /* overflow = */
+ resultType match {
+ case d: DecimalType =>
+ If(overflow, true,
+ HasOverflow(coalesce(sum, zero) + child.cast(sumDataType), d))
+ case _ => false
+ })
+ } else {
+ Seq(
+ /* sum = */
+ coalesce(sum, zero) + child.cast(sumDataType),
+ /* overflow = */
+ resultType match {
+ case d: DecimalType =>
+ If(overflow, true, HasOverflow(coalesce(sum, zero) + child.cast(sumDataType), d))
+ case _ => false
+ })
+ }
+ } else {
+ if (child.nullable) {
+ Seq(
+ /* sum = */
+ resultType match {
+ case d: DecimalType => coalesce(
+ CheckOverflow(
+ coalesce(sum, zero) + child.cast(sumDataType), d, !SQLConf.get.ansiEnabled), sum)
+ case _ => coalesce(coalesce(sum, zero) + child.cast(sumDataType), sum)
+ },
+ /* overflow = */
+ // overflow flag doesnt need any updates since CheckOverflow will throw exception
+ // if overflow happens
+ false
+ )
+ } else {
+ Seq(
+ /* sum = */
+ resultType match {
+ case d: DecimalType => CheckOverflow(
+ coalesce(sum, zero) + child.cast(sumDataType), d, !SQLConf.get.ansiEnabled)
+ case _ => coalesce(sum, zero) + child.cast(sumDataType)
+ },
+ /* overflow = */
+ false
+ )
+ }
+ }
+ }
+
+ override lazy val mergeExpressions: Seq[Expression] = {
+ if (!SQLConf.get.ansiEnabled) {
Seq(
/* sum = */
- coalesce(coalesce(sum, zero) + child.cast(sumDataType), sum)
+ coalesce(coalesce(sum.left, zero) + sum.right, sum.left),
+ /* overflow = */
+ resultType match {
+ case d: DecimalType =>
+ If(coalesce(overflow.left, false) || coalesce(overflow.right, false),
+ true, HasOverflow(coalesce(sum.left, zero) + sum.right, d))
+ case _ =>
+ If(coalesce(overflow.left, false) || coalesce(overflow.right, false), true, false)
+ }
)
} else {
Seq(
/* sum = */
- coalesce(sum, zero) + child.cast(sumDataType)
+ resultType match {
+ case d: DecimalType =>
+ coalesce(
+ CheckOverflow(coalesce(sum.left, zero) + sum.right, d, !SQLConf.get.ansiEnabled),
+ sum.left)
+ case _ => coalesce(coalesce(sum.left, zero) + sum.right, sum.left)
+ },
+ /* overflow = */
+ If(coalesce(overflow.left, false) || coalesce(overflow.right, false), true, false)
)
}
}
- override lazy val mergeExpressions: Seq[Expression] = {
- Seq(
- /* sum = */
- coalesce(coalesce(sum.left, zero) + sum.right, sum.left)
- )
- }
-
override lazy val evaluateExpression: Expression = resultType match {
- case d: DecimalType => CheckOverflow(sum, d, !SQLConf.get.ansiEnabled)
Review comment:
so you basically mean we should check overflow at every step during the sum calculation?
This can be super expensive, maybe we should have a special sum expression, which keeps an unlimited decimal presentation (like java BigDecimal) as buffer, and only check overflow when we need to serialize the buffer.
cc @hvanhovell @maryannxue @rednaxelafx
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595600181
> Addition of two decimal values ( expression coming from sum) that results in value that cannot be contained.
You can try `Decimal.+(Decimal)` locally, it does return a value that is not null. We can't hold an overflowed decimal value in unsafe row, but a `Decimal` object can be temporary overflowed.
In my repro, `spark.range(0, 12, 1, 1)` works fine and `spark.range(0, 1, 1, 1).union(spark.range(0, 11, 1, 1))` gives wrong result. I looked at the code again and whole-stage-codegen also stores the partial aggregate result to unsafe row. Can someone investigate it further and see why `spark.range(0, 12, 1, 1)` works?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-587819513
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-596918804
@skambha thanks for your investigation! Very helpful.
I'm trying to determine the scope of this bug. Your analysis is for global aggregate (no GROUP BY), when we only write out the sum value at the end of partial aggregate. I tried with group by
```
df.select(expr(s"cast('$decimalStr' as decimal (38, 18)) as d"), lit(1).as("key"))
.groupBy("key").agg(sum($"d")).show
java.lang.ArithmeticException: Decimal precision 39 exceeds max precision 38
at org.apache.spark.sql.types.Decimal.set(Decimal.scala:122)
at org.apache.spark.sql.types.Decimal$.apply(Decimal.scala:574)
at org.apache.spark.sql.types.Decimal.apply(Decimal.scala)
at org.apache.spark.sql.catalyst.expressions.UnsafeRow.getDecimal(UnsafeRow.java:393)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.agg_doConsume_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.agg_doAggregateWithKeys_1$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.agg_doAggregateWithKeys_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.processNext(Unknown Source)
...
```
Do you what happens there? I'd expect we write overflowed decimal as null to the aggregate hash map, but it seems not the case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595318275
Based on previous explanation, looks like the overflow happens during partial aggregate ( updateExpressions and mergeExpressions)? At final aggregate, is a check for null useful for the reported case here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613856107
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha edited a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha edited a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-615038394
@cloud-fan, This is the test case with null rows that showcases the behavior that I mentioned earlier. https://github.com/apache/spark/pull/27627/files#diff-5d2ebf4e9ca5a990136b276859769289R222-R223
--
To be able to distinguish this scenario, one alternative could be to have another flag in addition to the isEmpty flag to keep track of if we saw a non null value - but this would be adding two new flags to the buffer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595330926
Note: the semantic of most aggregate functions are skipping nulls. So sum will skip null inputs, and overflow doesn't return null (also true for decimal), so `updateExpressions` is fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611287758
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611308104
> A function implementor would need to know to use DecimalSum instead of Sum and special case it.
My main point is to avoid perf regression for other types. We can also put everything in `Sum`, and use `if (is decimal type)` to generate different update/merge expressions for decimal and other types.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan edited a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-596918804
@skambha thanks for your investigation! Very helpful.
I'm trying to determine the scope of this bug. Your analysis is for global aggregate (no GROUP BY), when we only write out the sum value at the end of partial aggregate. I tried with group by
```
df.select(expr(s"cast('$decimalStr' as decimal (38, 18)) as d"), lit(1).as("key"))
.groupBy("key").agg(sum($"d")).show
java.lang.ArithmeticException: Decimal precision 39 exceeds max precision 38
at org.apache.spark.sql.types.Decimal.set(Decimal.scala:122)
at org.apache.spark.sql.types.Decimal$.apply(Decimal.scala:574)
at org.apache.spark.sql.types.Decimal.apply(Decimal.scala)
at org.apache.spark.sql.catalyst.expressions.UnsafeRow.getDecimal(UnsafeRow.java:393)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.agg_doConsume_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.agg_doAggregateWithKeys_1$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.agg_doAggregateWithKeys_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.processNext(Unknown Source)
...
```
For aggregate with GROUP BY, we write out the sum value to unsafe row(the aggregate hash map) at each step. Do you what happens there? I'd expect we write overflowed decimal as null to the aggregate hash map, but it seems not the case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595094058
Thank you for the example and analysis, @cloud-fan .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r380939010
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Sum.scala
##########
@@ -60,38 +60,104 @@ case class Sum(child: Expression) extends DeclarativeAggregate with ImplicitCast
private lazy val sumDataType = resultType
private lazy val sum = AttributeReference("sum", sumDataType)()
+ private lazy val overflow = AttributeReference("overflow", BooleanType, false)()
private lazy val zero = Literal.default(resultType)
- override lazy val aggBufferAttributes = sum :: Nil
+ override lazy val aggBufferAttributes = sum :: overflow :: Nil
override lazy val initialValues: Seq[Expression] = Seq(
- /* sum = */ Literal.create(null, sumDataType)
+ /* sum = */ Literal.create(null, sumDataType),
+ /* overflow = */ Literal.create(false, BooleanType)
)
override lazy val updateExpressions: Seq[Expression] = {
- if (child.nullable) {
+ if (!SQLConf.get.ansiEnabled) {
+ if (child.nullable) {
+ Seq(
+ /* sum = */
+ coalesce(coalesce(sum, zero) + child.cast(sumDataType), sum),
Review comment:
The changes in the Sum are mostly to check if overflow has occurred when we do the different additions in the updateExpressions and mergeExpressions. The actual addition operations are all the same. Reading the diff may not show that easily so wanted to make a note here on that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-598002388
> Is the expectation that the user would now call this from their queries for their decimal sum operations?
No, it's just a new expression. Analyzer is responsible for converting `Sum` with decimal type input to `DecimalSum`.
> Won't we also need to check for overflow in here as well.
ah yes, thanks for catching it!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r405917785
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/DecimalSum.scala
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions.aggregate
+
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types._
+
+case class DecimalSum(child: Expression) extends DeclarativeAggregate with ImplicitCastInputTypes {
Review comment:
Could we share some code to handle overflows between `DecimalSum` and `DecimalAverage` by adding a base abstract class?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611366131
**[Test build #120991 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/120991/testReport)** for PR 27627 at commit [`a1fae7b`](https://github.com/apache/spark/commit/a1fae7ba13e6495c5008d07c4514bc3106b5735a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class DecimalSum(child: Expression) extends DeclarativeAggregate with ImplicitCastInputTypes `
* `case class OverflowException(dtype: DataType, msg: String) extends LeafExpression `
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-615038394
@cloud-fan, This is the test case with null rows that showcases the behavior that I mentioned earlier. https://github.com/apache/spark/pull/27627/files#diff-5d2ebf4e9ca5a990136b276859769289R222-R223
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-615065652
@skambha check the result in PostgreSQL:
```
cloud0fan=# create table t(d decimal(20, 10));
CREATE TABLE
cloud0fan=# insert into t values (1.1);
INSERT 0 1
cloud0fan=# insert into t values (2.2);
INSERT 0 1
cloud0fan=# insert into t values (null);
INSERT 0 1
cloud0fan=# select sum(d) from t;
sum
--------------
3.3000000000
(1 row)
```
Nulls should be skipped when calculating aggregate functions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
maropu commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595108294
For integral sum (e.g., int/long), overflow can happen in partial aggregate sides (via `Math.addExact`). We don't need to follow the behaviour in decimal sum, too, for consistency?
```
scala> sql("SET spark.sql.ansi.enabled=true")
res39: org.apache.spark.sql.DataFrame = [key: string, value: string]
scala> spark.table("t").printSchema
root
|-- v: long (nullable = true)
scala> sql("select * from t").show()
+-------------------+
| v|
+-------------------+
|9223372036854775807|
| 1|
+-------------------+
scala> sql("select sum(*) from t").show()
// Throws an exception
java.lang.ArithmeticException: long overflow
at java.lang.Math.addExact(Math.java:809)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage2.agg_doConsume_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage2.agg_doAggregateWithoutKey_0$(Unknown Source)
at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage2.processNext(Unknown Source)
at org.apache.spark.sql.execution.BufferedRowIterator.hasNext(BufferedRowIterator.java:43)
at org.apache.spark.sql.execution.WholeStageCodegenExec$$anon$1.hasNext(WholeStageCodegenExec.scala:729)
at ...
scala> sql("SET spark.sql.ansi.enabled=false")
scala> sql("select sum(*) from t").show()
// Wrong result
+--------------------+
| sum(v)|
+--------------------+
|-9223372036854775808|
+--------------------+
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611287766
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25682/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613993649
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-594644908
I tried some queries locally but can't reproduce. Is it only a problem with a join?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611289438
**[Test build #120991 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/120991/testReport)** for PR 27627 at commit [`a1fae7b`](https://github.com/apache/spark/commit/a1fae7ba13e6495c5008d07c4514bc3106b5735a).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r405917785
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/DecimalSum.scala
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions.aggregate
+
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types._
+
+case class DecimalSum(child: Expression) extends DeclarativeAggregate with ImplicitCastInputTypes {
Review comment:
Could we share some code between `DecimalSum` and `DecimalAverage` by adding a base abstract class?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613855701
**[Test build #121302 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121302/testReport)** for PR 27627 at commit [`20d1ecf`](https://github.com/apache/spark/commit/20d1ecf470e43c84dd3aa4278c010f413edff934).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r386739126
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -132,3 +132,32 @@ case class CheckOverflow(
override def sql: String = child.sql
}
+
+case class HasOverflow(
+ child: Expression,
+ inputType: DecimalType) extends UnaryExpression {
+
+ override def dataType: DataType = BooleanType
+
+ override def nullable: Boolean = true
Review comment:
Since the child can be nullable, the input value can be null. Making nullable to false in that case will not work, as it may result in npe. We can change the doGenCode() here to make the check for null for that, but since the nullSafeCodeGen in UnaryExpression already takes care of the if nullable checks, it seems there is no need to add if null checks here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-594110207
> > In case of the whole stage codegen, you can see the decimal ‘+’ expressions will at some point be larger than what can be contained in dec 38,18 but it gets written out as null. This messes up the end result of the sum and you get wrong results.
>
> Still a bit confused. If it gets written out as null, then null + decimal always return null and the final result is null?
So if we look into the aggregate Sum, we have **coalesce** in updateExpressions and mergeExpressions, so it is not purely only a null + decimal only expression. For e.g, in updateExpressions if the intermediate sum becomes null because of overflow, then in the next iteration of the updateExpressions, coalesce will be used to make the sum become 0 for that and the sum will be updated to 0 + decimal.
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Sum.scala#L76
```
override lazy val updateExpressions: Seq[Expression]
...
/* sum = */
coalesce(coalesce(sum, zero) + child.cast(sumDataType), sum)
```
Please let me know if this helps.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611165856
>>How about we add a new expression DecimalSum?
>Analyzer is responsible for converting Sum with decimal type input to DecimalSum.
@cloud-fan, Thanks. There are two things I wanted to update.
A) I looked at the idea of a new DecimalSum and implementation wise it is ok. I will push those changes right after this comment.
A few observations:
So when user calls sum function on a decimal column, we can have a rule in analyzer that converts it to decimalsum. Currently, there are some optimizer rules that work on Sum(Decimal Type), those would also require to be changed to look at DecimalSum instead.
When user calls sum function, we can use the analyzer or optimizer rule to make the necessary changes to look at decimalsum. But this itself will still not be able to cover the cases where this happens as part of the PhysicalPlan.
For e.g. Avg aggregate function as part of its implementation uses the Sum expression. So a analyzer rule will not be enough to make the change transparently. We will need to change the avg implementation.
Also for cases similar to Avg, it seems like it will be a maintenance overhead. ie. A function implementor would need to know to use DecimalSum instead of Sum and special case it. A fix inside of sum seems that it is self contained.
@cloud-fan So, I wanted your input on this: What is the rationale for preferring a DecimalSum implementation instead of fixing it in Sum?
——
B) The algorithm mentioned [here](https://github.com/apache/spark/pull/27627#issuecomment-597440834) does not cover for the scenario where we may have valid null rows. That implementation is uploaded here on my GitHub here just as an fyi.
https://github.com/skambha/notes/blob/master/DecimalSum2.scala
> initial value is [0, true]
So if we have 2 null rows, the expected result should be null and not zero.
We need a way to distinguish between valid null rows and null value that come because of a overflow as well.
--
I have a solution using a combination of approaches we have discussed so far, that solves the case for valid null rows and overflow cases for all the whole stage enabled/disabled, and for ansi enabled/disabled scenarios in consistent manner.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-593977066
OK to test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595067684
cc @viirya @maropu as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] kiszk commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r383013891
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -132,3 +132,32 @@ case class CheckOverflow(
override def sql: String = child.sql
}
+
+case class HasOverflow(
+ child: Expression,
+ inputType: DecimalType) extends UnaryExpression {
+
+ override def dataType: DataType = BooleanType
+
+ override def nullable: Boolean = true
Review comment:
Can we override `nullable` with `false` as `doGenCode()` does?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r406018460
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/DecimalSum.scala
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions.aggregate
+
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types._
+
+case class DecimalSum(child: Expression) extends DeclarativeAggregate with ImplicitCastInputTypes {
+
+ override def children: Seq[Expression] = child :: Nil
+
+ override def nullable: Boolean = true
+
+ // Return data type.
+ override def dataType: DataType = resultType
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(NumericType)
+
+ private lazy val resultType = child.dataType match {
+ case DecimalType.Fixed(precision, scale) =>
+ DecimalType.bounded(precision + 10, scale)
+ case _: IntegralType => LongType
+ case _ => DoubleType
+ }
+
+ private lazy val sumDataType = resultType
+
+ private lazy val sum = AttributeReference("sum", sumDataType)()
+ private lazy val overflow = AttributeReference("overflow", BooleanType, false)()
+
+ private lazy val zero = Literal.default(resultType)
+
+ override lazy val aggBufferAttributes = sum :: overflow :: Nil
+
+ override lazy val initialValues: Seq[Expression] = Seq(
+ /* sum = */ Literal.create(null, sumDataType),
+ /* overflow = */ Literal.create(false, BooleanType)
+ )
+
+ override lazy val updateExpressions: Seq[Expression] = {
Review comment:
can we add some comments to explain the general workflow?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] kiszk commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
kiszk commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-590179301
@skambha Should we review #27629 instead of this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
maropu commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611287252
> For e.g. Avg aggregate function as part of its implementation uses the Sum expression. So a analyzer rule will not be enough to make the change transparently. We will need to change the avg implementation.
Yea, probably, I think we need `DecimalAverage`, too, for the approach.
Btw, could you check no performance regression w/ `DecimalSum` for regular non-overflow cases?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613993020
**[Test build #121302 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121302/testReport)** for PR 27627 at commit [`20d1ecf`](https://github.com/apache/spark/commit/20d1ecf470e43c84dd3aa4278c010f413edff934).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r380941152
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Sum.scala
##########
@@ -60,38 +60,104 @@ case class Sum(child: Expression) extends DeclarativeAggregate with ImplicitCast
private lazy val sumDataType = resultType
private lazy val sum = AttributeReference("sum", sumDataType)()
+ private lazy val overflow = AttributeReference("overflow", BooleanType, false)()
private lazy val zero = Literal.default(resultType)
- override lazy val aggBufferAttributes = sum :: Nil
+ override lazy val aggBufferAttributes = sum :: overflow :: Nil
override lazy val initialValues: Seq[Expression] = Seq(
- /* sum = */ Literal.create(null, sumDataType)
+ /* sum = */ Literal.create(null, sumDataType),
+ /* overflow = */ Literal.create(false, BooleanType)
Review comment:
We keep track of overflow using this aggBufferAttributes - overflow to know if any of the intermediate add operations in updateExpressions and/or mergeExpressions overflow'd. If the overflow is true and if spark.sql.ansi.enabled flag is false, then we return null for the sum operation in evaluateExpression.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-594010285
> In case of the whole stage codegen, you can see the decimal ‘+’ expressions will at some point be larger than what can be contained in dec 38,18 but it gets written out as null. This messes up the end result of the sum and you get wrong results.
Still a bit confused. If it gets written out as null, then null + decimal always return null and the final result is null?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613855701
**[Test build #121302 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121302/testReport)** for PR 27627 at commit [`20d1ecf`](https://github.com/apache/spark/commit/20d1ecf470e43c84dd3aa4278c010f413edff934).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613993655
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/121302/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
maropu commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611287449
ok to test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-588096559
cc @mgaido91
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-587820876
Please see [some notes](https://issues.apache.org/jira/browse/SPARK-28067?focusedCommentId=17039403&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17039403) in this JIRA for the two approaches. This is a implementation for approach 2 fix. I'm marking this as WIP in order to get comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595112843
Ideally, under ANSI mode, decimal sum should also fail if overflows. It's hard to fail at the partial aggregate side, as we don't have a chance to check overflow before shuffling the aggregate buffer row. We can fail at the final aggregate side: If the value is null and `isEmpty` is false, fail with the overflow exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan edited a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595067684
cc @viirya @maropu @dongjoon-hyun as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595321878
> For e.g, in updateExpressions if the intermediate sum becomes null because of overflow, then in the next iteration of the updateExpressions, coalesce will be used to make the sum become 0 for that and the sum will be updated to 0 + decimal.
Sounds like not just a problem of `mergeExpressions`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611367098
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120991/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611289438
**[Test build #120991 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/120991/testReport)** for PR 27627 at commit [`a1fae7b`](https://github.com/apache/spark/commit/a1fae7ba13e6495c5008d07c4514bc3106b5735a).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-597440834
@skambha great analysis!
I agree with you that we need another boolean flag in the sum aggregate buffer, but I'd like to make it simpler and only change it for decimals.
How about we add a new expression `DecimalSum`? In which:
1. the buffer attributes are [sum, isEmpty]
2. initial value is [0, true]
3. the `updateExpression` should do:
3.1 update `isEmpty` to false
3.2 set `sum` to null if overflowed
3.3 do nothing if `sum` is already null.
4. the `mergeExpression` should do:
4.1 update `isEmpty` to false
4.2 if the input buffer's `isEmpty` is true, keep sum unchanged
4.3 if the input buffer's `isEmpty` is false, but `sum` is null, update its own `sum` to null
4.4 do nothing if `sum` is already null.
4.5 otherwise, add input buffer's `sum`
5. the `evaluateExpression` should do:
5.1 output null if `isEmpty` is true
5.2 fail if `sum` is null and ansi mode is on
5.3 otherwise, output the sum.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595411413
In the repro (that has the join and then the agg), we also see the null being written out via the UnsafeRowWriter that is mentioned in my earlier comment with codegen details here.https://github.com/apache/spark/pull/27627#issuecomment-591185451
Codegen and the plan details from https://github.com/apache/spark/pull/27627#issuecomment-591185451 here:
https://github.com/skambha/notes/blob/master/spark28067_ansifalse_wholestagetrue.txt
```
scala> df2.queryExecution.debug.codegen
Found 2 WholeStageCodegen subtrees.
== Subtree 1 / 2 (maxMethodCodeSize:173; maxConstantPoolSize:145(0.22% used); numInnerClasses:0) ==
*(2) HashAggregate(keys=[], functions=[sum(decNum#14)], output=[sum(decNum)#23])
+- Exchange SinglePartition, true, [id=#77]
+- *(1) HashAggregate(keys=[], functions=[partial_sum(decNum#14)], output=[sum#29])
+- *(1) Project [decNum#14]
+- *(1) BroadcastHashJoin [intNum#8], [intNum#15], Inner, BuildLeft
:- BroadcastExchange HashedRelationBroadcastMode(List(cast(input[0, int, false] as bigint))), [id=#65]
: +- LocalTableScan [intNum#8]
+- *(1) LocalTableScan [decNum#14, intNum#15]
```
The relevant code lines of interest are the following:
- Addition of two decimal values ( expression coming from sum) that results in value that cannot be contained.
- Writing a big overflow decimal using UnsafeWriter —> which will write null silently.
Codegen id1:
/* 108 */ agg_value_3 = agg_value_4.$plus(agg_expr_0_0);
/* 113 */ agg_value_2 = agg_value_3;
/* 126 */ agg_mutableStateArray_0[0] = agg_value_2;
/* 147 */ bhj_mutableStateArray_0[3].write(0, agg_mutableStateArray_0[0], 38, 18);
https://github.com/skambha/notes/blob/master/spark28067_ansifalse_wholestagetrue.txt#L336
https://github.com/skambha/notes/blob/master/spark28067_ansifalse_wholestagetrue.txt#L354
https://github.com/skambha/notes/blob/master/spark28067_ansifalse_wholestagetrue.txt#L375
Codegen id2:
/* 080 */ agg_value_5 = agg_value_6.$plus(agg_expr_0_0);
/* 091 */ agg_value_4 = agg_mutableStateArray_0[0];
/* 098 */ agg_mutableStateArray_0[0] = agg_value_4;
/* 128 */ agg_mutableStateArray_1[0].write(0, agg_value_1, 38, 18);
https://github.com/skambha/notes/blob/master/spark28067_ansifalse_wholestagetrue.txt#L164
https://github.com/skambha/notes/blob/master/spark28067_ansifalse_wholestagetrue.txt#L212
- For both partial and final stage of aggregate, we can see the same issue that can happen.
@cloud-fan, given the above pieces of code, why would the problem not exist for the partial aggregate (updateExpression) case?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611367094
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611287766
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25682/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611175398
I have pushed the changes in the latest commit. Please take a look. Thanks.
- Uses new DecimalSum implementation
- Adds a new analyzer rule and one optimizer rule is updated.
- Tests are added for the test scenarios we discussed in this pr so far, and for all the whole stage enabled/disabled, ansi enabled/disabled combinations.
_(Sidenote: For now the DecimalSum has the code to handle other types, incase we want to put this logic back into Sum after discussion, if not then we can remove that.)_
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613856116
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25986/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-587821556
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-591191022
>@skambha Should we review #27629 instead of this PR?
Thanks for checking. I was hoping to get some feedback on the preferred approach on the JIRA. So in the meantime, I have these 2 pr open. The other PR throws exception if there is overflow. This pr is to honor the ansi flag/null behavior and follows the approach in SPARK-28224. Once we make a decision I can close one out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-595319742
> looks like the overflow happens during partial aggregate ( updateExpressions and mergeExpressions)
partial aggregate does `updateExpressions` and produces buffer, final aggregate does `mergeExpressions` to merge buffers. The problem here is, `mergeExpressions` treats null as 0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] skambha commented on issue #27627: [WIP][SPARK-28067][SQL]
Fix incorrect results for decimal aggregate sum by returning null on
decimal overflow
Posted by GitBox <gi...@apache.org>.
skambha commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-597404017
>For aggregate with GROUP BY, we write out the sum value to unsafe row(the aggregate hash map) at each step. Do you what happens there?
Details are here: https://github.com/skambha/notes/blob/master/case3_exception.txt Scenario has the 2 range and union and then your query from previous comment.
Below are some brief notes:
1. In this case, we write to an aggregate HashMap using UnsafeRow. The writing of the UnsafeRow to the hash map does not use the UnsafeRowWriter that has the issue with writing null for an overflowed decimal value.
(Side note, in the whole stage execution, we will still have write using UnsafeRowWriter but that processing happens later. (ie after the hashmap is built etc). The error from the scenario mentioned is actually coming prior to that)
2. When building the hash map, it will use the _UnsafeRow.setDecimal_ call.
`agg_unsafeRowAggBuffer_0.setDecimal(0, agg_value_2, 38);`
This setDecimal method does not check if the decimal value overflowed or not.
3. As it is building the hash map, if the key already exists in the hash map, it will retrieve it and then do the + with the new input row's value. In this case, the method it uses to retrieve the decimal value from the hash map is `UnsafeRow.getDecimal`
`agg_unsafeRowAggBuffer_0.getDecimal(0, 38, 18)`
4. The UnsafeRow.getDecimal calls the `Decimal.set(decimal: BigDecimal, precision: Int, scale: Int)` via this`Decimal.apply(javaDecimal, precision, scale);`which actually does a check to see if the value is containable and if not it will throw an exception that you see.
```
if (decimalVal.precision > precision) {
throw new ArithmeticException(
s"Decimal precision ${decimalVal.precision} exceeds max precision $precision")
}
```
5. In the example scenario that has the 11 rows in one and 1 row in the other partition, and the group by key is same.
```
val decimalStr = "1" + "0" * 19
val df = spark.range(0, 1, 1, 1).union(spark.range(0, 11, 1, 1))
df.select(expr(s"cast('$decimalStr' as decimal (38, 18)) as d"), lit(1).as("key"))
.groupBy("key").agg(sum($"d")).show
```
6. When it is building the hash map, it would have successfully added up to the 10 rows, and the sum at that point is `100000000000000000000` which is a overflowed value. And when it gets to the next row (11th row), when it fetches the value from the agg hash map, using the UnsafeRow.getDecimal(..) will throw an error as its value is not containable.
`agg_unsafeRowAggBuffer_0.getDecimal(0, 38, 18)`,
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613993649
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] kiszk commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r383010088
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -132,3 +132,32 @@ case class CheckOverflow(
override def sql: String = child.sql
}
+
+case class HasOverflow(
+ child: Expression,
+ inputType: DecimalType) extends UnaryExpression {
+
+ override def dataType: DataType = BooleanType
+
+ override def nullable: Boolean = true
+
+ override def nullSafeEval(input: Any): Any =
+ !input.asInstanceOf[Decimal].changePrecision(
+ inputType.precision,
+ inputType.scale,
+ Decimal.ROUND_HALF_UP)
+
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ nullSafeCodeGen(ctx, ev, eval => {
+ s"""
+ |${ev.value} = !$eval.changePrecision(
+ | ${inputType.precision}, ${inputType.scale}, Decimal.ROUND_HALF_UP());
+ | ${ev.isNull} = false;
Review comment:
nit: indentation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#discussion_r405917396
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -240,6 +240,7 @@ class Analyzer(
ExtractWindowExpressions ::
GlobalAggregates ::
ResolveAggregateFunctions ::
+ UseDecimalSum ::
Review comment:
I think its ok to apply this rule after the resolution batch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613856107
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-611367098
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120991/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-613993655
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/121302/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-587819513
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27627:
[WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by
returning null on decimal overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27627: [WIP][SPARK-28067][SQL] Fix incorrect results for decimal aggregate sum by returning null on decimal overflow
URL: https://github.com/apache/spark/pull/27627#issuecomment-587821556
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org