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 2022/05/27 08:50:07 UTC

[GitHub] [spark] ulysses-you opened a new pull request, #36698: [SPARK-39316][SQL] Merge PromotePrecision and CheckOverflow into decimal binary arithmetic

ulysses-you opened a new pull request, #36698:
URL: https://github.com/apache/spark/pull/36698

### What changes were proposed in this pull request?

The main change:
- Add a new trait `DecimalArithmetic` for the related decimal arithmetic
- Add a new expression `DecimalAdd` for the internal decimal add, e.g. `Sum`/`Average`, the different with `Add` is:
- `DecimalAdd` does not check overflow
- `DecimalAdd` make `dataType` as its input parameter
- Merge the decimal precision code of `DecimalPrecision` into each arithmetic data type, so every arithmetic should report the accurate decimal type. And we can remove the unused expression `PromotePrecision` and related code
- Merge `CheckOverflow` iinto arithmetic eval and code-gen code path, so every arithmetic can handle the overflow case during runtime

Merge `PromotePrecision` into `dataType`, for example, `Add`:
```scala
override def dataType: DataType = (left, right) match {
case (DecimalType.Expression(p1, s1), DecimalType.Expression(p2, s2)) =>
val resultScale = max(s1, s2)
if (allowPrecisionLoss) {
DecimalType.adjustPrecisionScale(max(p1 - s1, p2 - s2) + resultScale + 1,
resultScale)
} else {
DecimalType.bounded(max(p1 - s1, p2 - s2) + resultScale + 1, resultScale)
}
case _ => super.dataType
}
```

Merge `CheckOverflow`, for example, `Add` eval:
```scala
dataType match {
case decimalType: DecimalType =>
val value = numeric.plus(input1, input2)
checkOverflow(value.asInstanceOf[Decimal], decimalType)
...
}
```

Note that, `CheckOverflow` is still useful after this pr, e.g. `RowEncoder`. We can do further in a separate pr.

### Why are the changes needed?

Fix the bug of `TypeCoercion`, for example:
```sql
SELECT CAST(1 AS DECIMAL(28, 2))
UNION ALL
SELECT CAST(1 AS DECIMAL(18, 2)) / CAST(1 AS DECIMAL(18, 2));
```

Relax the decimal precision at runtime, so we do not need redundant Cast

### Does this PR introduce _any_ user-facing change?

yes, bug fix

### How was this patch tested?

Pass exists test and add some bug fix test in `union.sql`

--
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org