Skip to content

[SPARK-45905][SQL] Least common type between decimal types should retain integral digits first #43781

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cloud-fan wants to merge 9 commits into from
Closed

[SPARK-45905][SQL] Least common type between decimal types should retain integral digits first #43781

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
70affef
least common type between decimal types should retain integral digits…
cloud-fan Nov 13, 2023
41e0a3c
Update docs/sql-ref-ansi-compliance.md
cloud-fan Nov 13, 2023
cf5cb7b
Update docs/sql-ref-ansi-compliance.md
cloud-fan Nov 13, 2023
d61da82
fix
cloud-fan Nov 14, 2023
5426242
fix golden
cloud-fan Nov 14, 2023
f75fe5d
fix
cloud-fan Nov 15, 2023
c6f4295
Update docs/sql-ref-ansi-compliance.md
cloud-fan Nov 15, 2023
0400e8c
Update docs/sql-ref-ansi-compliance.md
cloud-fan Nov 15, 2023
42477eb
Update sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLC…
cloud-fan Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions docs/sql-ref-ansi-compliance.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,25 @@ The least common type resolution is used to:
- Derive the result type for expressions such as the case expression.
- Derive the element, key, or value types for array and map constructors.
Special rules are applied if the least common type resolves to FLOAT. With float type values, if any of the types is INT, BIGINT, or DECIMAL the least common type is pushed to DOUBLE to avoid potential loss of digits.

Decimal type is a bit more complicated here, as it's not a simple type but has parameters: precision and scale.
A `decimal(precision, scale)` means the value can has at most `precision - scale` digits in the integral part and `scale` digits in the fractional part.
A least common type between decimal types should have enough digits in both integral and fractional parts to represent all values.
More precisely, a least common type between `decimal(p1, s1)` and `decimal(p2, s2)` has the scale of `max(s1, s2)` and precision of `max(s1, s2) + max(p1 - s1, p2 - s2)`.
However, decimal types in Spark has a maximum precision: 38. If the final decimal type needs more precision, we must do truncation.
Since the digits in the integral part are more significant, Spark truncates the digits in the fractional part first. For example, `decimal(48, 20)` will be reduced to `decimal(38, 10)`.

Note, arithmetic operations have special rules to calculate the least common type for decimal inputs:

| Operation | Result precision | Result scale |
|------------|------------------------------------------|---------------------|
| e1 + e2 | max(s1, s2) + max(p1 - s1, p2 - s2) + 1 | max(s1, s2) |
| e1 - e2 | max(s1, s2) + max(p1 - s1, p2 - s2) + 1 | max(s1, s2) |
| e1 * e2 | p1 + p2 + 1 | s1 + s2 |
| e1 / e2 | p1 - s1 + s2 + max(6, s1 + p2 + 1) | max(6, s1 + p2 + 1) |
| e1 % e2 | min(p1 - s1, p2 - s2) + max(s1, s2) | max(s1, s2) |
Copy link
Contributor

@beliefer beliefer Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, the arithmetic operations did not strictly follow this rule.

Copy link
Contributor Author

@cloud-fan cloud-fan Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which one does not follow? The final decimal type can be different as there is one more truncation step.

Copy link
Contributor

@beliefer beliefer Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* AND /.
For example:

val a = Decimal(100) // p: 10, s: 0
val b= Decimal(-100) // p: 10, s: 0
val c = a * b  // Decimal(-10000) p: 5, s: 0

Copy link
Contributor Author

@cloud-fan cloud-fan Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the Spark SQL multiple. Please take a look at Multiple#resultDecimalType


The truncation rule is also different for arithmetic operations: they retain at least 6 digits in the fractional part, which means we can only reduce `scale` to 6.
Copy link
Member

@viirya viirya Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention what happens if we cannot truncate fractional part to make it fit into maximum precision? Overflow?


```sql
-- The coalesce function accepts any set of argument types as long as they share a least common type.
Expand Down
12 changes: 12 additions & 0 deletions sql/api/src/main/scala/org/apache/spark/sql/types/DecimalType.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,18 @@ object DecimalType extends AbstractDataType {
DecimalType(min(precision, MAX_PRECISION), min(scale, MAX_SCALE))
}

private[sql] def boundedPreferIntegralDigits(precision: Int, scale: Int): DecimalType = {
if (precision <= MAX_PRECISION) {
DecimalType(precision, scale)
} else {
// If we have to reduce the precision, we should retain the digits in the integral part first,
// as they are more significant to the value. Here we reduce the scale as well to drop the
// digits in the fractional part.
Copy link
Contributor

@beliefer beliefer Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

val diff = precision - MAX_PRECISION
DecimalType(MAX_PRECISION, math.max(0, scale - diff))
}
}

private[sql] def checkNegativeScale(scale: Int): Unit = {
if (scale < 0 && !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
throw DataTypeErrors.negativeScaleNotAllowedError(scale)
Expand Down
7 changes: 6 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/DecimalPrecision.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package org.apache.spark.sql.catalyst.analysis
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.Literal._
import org.apache.spark.sql.catalyst.types.DataTypeUtils
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._


Expand Down Expand Up @@ -64,7 +65,11 @@ object DecimalPrecision extends TypeCoercionRule {
def widerDecimalType(p1: Int, s1: Int, p2: Int, s2: Int): DecimalType = {
val scale = max(s1, s2)
val range = max(p1 - s1, p2 - s2)
DecimalType.bounded(range + scale, scale)
if (conf.getConf(SQLConf.LEGACY_RETAIN_FRACTION_DIGITS_FIRST)) {
DecimalType.bounded(range + scale, scale)
Copy link
Member

@gengliangwang gengliangwang Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many usages of DecimalType.bounded.
Why we only change the behavior here?

Copy link
Contributor Author

@cloud-fan cloud-fan Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To limit the scope to type coercion only. Some arithmetic operations also call it to determine the result decimal type and I don't want to change that part.

} else {
DecimalType.boundedPreferIntegralDigits(range + scale, scale)
}
}

override def transform: PartialFunction[Expression, Expression] = {
Expand Down
11 changes: 10 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4541,6 +4541,15 @@ object SQLConf {
.booleanConf
.createWithDefault(false)

val LEGACY_RETAIN_FRACTION_DIGITS_FIRST =
buildConf("spark.sql.legacy.decimalLeastCommonType.retainFractionDigitsFirst")
Copy link
Member

@gengliangwang gengliangwang Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH the conf is a bit long.
How about spark.sql.legacy.decimal.retainFractionDigitsOnTruncate. But the naming really depends on the scrope of the behavior change as I asked in https://github.com/apache/spark/pull/43781/files#r1393331222

.internal()
.doc("When set to true, we will try to retain the fraction digits first rather than " +
"integral digits, when getting a least common type between decimal types, and the " +
"result decimal precision exceeds the max precision.")
Copy link
Member

@viirya viirya Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.doc("When set to true, we will try to retain the fraction digits first rather than " +
"integral digits, when getting a least common type between decimal types, and the " +
"result decimal precision exceeds the max precision.")
.doc("When set to true, we will try to retain the fraction digits first rather than " +
"integral digits as prior Spark 4.0, when getting a least common type between decimal types, and the " +
"result decimal precision exceeds the max precision.")

.booleanConf
.createWithDefault(false)

/**
* Holds information about keys that have been deprecated.
*
Expand Down Expand Up @@ -5425,7 +5434,7 @@ class SQLConf extends Serializable with Logging with SqlApiConf {
}

def legacyRaiseErrorWithoutErrorClass: Boolean =
getConf(SQLConf.LEGACY_RAISE_ERROR_WITHOUT_ERROR_CLASS)
getConf(SQLConf.LEGACY_RAISE_ERROR_WITHOUT_ERROR_CLASS)
Copy link
Contributor

@ryan-johnson-databricks ryan-johnson-databricks Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noise? (whitespace changes best made in a non-bugfix PR?)

Copy link
Contributor Author

@cloud-fan cloud-fan Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since I touched this file, I just fixed the wrong indentation.


/** ********************** SQLConf functionality methods ************ */

Expand Down
10 changes: 5 additions & 5 deletions ...atalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnsiTypeCoercionSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,9 +481,9 @@ class AnsiTypeCoercionSuite extends TypeCoercionSuiteBase {
:: Literal.create(null, DecimalType(22, 10))
:: Literal.create(null, DecimalType(38, 38))
:: Nil),
CreateArray(Literal.create(null, DecimalType(5, 3)).cast(DecimalType(38, 38))
:: Literal.create(null, DecimalType(22, 10)).cast(DecimalType(38, 38))
:: Literal.create(null, DecimalType(38, 38))
CreateArray(Literal.create(null, DecimalType(5, 3)).cast(DecimalType(38, 26))
:: Literal.create(null, DecimalType(22, 10)).cast(DecimalType(38, 26))
:: Literal.create(null, DecimalType(38, 38)).cast(DecimalType(38, 26))
:: Nil))
}

Expand Down Expand Up @@ -530,9 +530,9 @@ class AnsiTypeCoercionSuite extends TypeCoercionSuiteBase {
:: Literal.create(null, DecimalType(38, 38))
:: Nil),
CreateMap(Literal(1)
:: Literal.create(null, DecimalType(38, 0)).cast(DecimalType(38, 38))
:: Literal.create(null, DecimalType(38, 0))
:: Literal(2)
:: Literal.create(null, DecimalType(38, 38))
:: Literal.create(null, DecimalType(38, 38)).cast(DecimalType(38, 0))
:: Nil))
// type coercion for both map keys and values
ruleTest(AnsiTypeCoercion.FunctionArgumentConversion,
Expand Down
18 changes: 9 additions & 9 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,7 @@ class TypeCoercionSuite extends TypeCoercionSuiteBase {
widenTestWithStringPromotion(
ArrayType(DecimalType(36, 0), containsNull = false),
ArrayType(DecimalType(36, 35), containsNull = false),
Some(ArrayType(DecimalType(38, 35), containsNull = true)))
Some(ArrayType(DecimalType(38, 2), containsNull = false)))

// MapType
widenTestWithStringPromotion(
Expand All @@ -808,15 +808,15 @@ class TypeCoercionSuite extends TypeCoercionSuiteBase {
widenTestWithStringPromotion(
MapType(StringType, DecimalType(36, 0), valueContainsNull = false),
MapType(StringType, DecimalType(36, 35), valueContainsNull = false),
Some(MapType(StringType, DecimalType(38, 35), valueContainsNull = true)))
Some(MapType(StringType, DecimalType(38, 2), valueContainsNull = false)))
widenTestWithStringPromotion(
MapType(IntegerType, StringType, valueContainsNull = false),
MapType(DecimalType.IntDecimal, StringType, valueContainsNull = false),
Some(MapType(DecimalType.IntDecimal, StringType, valueContainsNull = false)))
widenTestWithStringPromotion(
MapType(DecimalType(36, 0), StringType, valueContainsNull = false),
MapType(DecimalType(36, 35), StringType, valueContainsNull = false),
None)
Some(MapType(DecimalType(38, 2), StringType, valueContainsNull = false)))

// StructType
widenTestWithStringPromotion(
Expand Down Expand Up @@ -847,7 +847,7 @@ class TypeCoercionSuite extends TypeCoercionSuiteBase {
widenTestWithStringPromotion(
new StructType().add("num", DecimalType(36, 0), nullable = false),
new StructType().add("num", DecimalType(36, 35), nullable = false),
Some(new StructType().add("num", DecimalType(38, 35), nullable = true)))
Some(new StructType().add("num", DecimalType(38, 2), nullable = false)))

widenTestWithStringPromotion(
new StructType().add("num", IntegerType),
Expand Down Expand Up @@ -1046,9 +1046,9 @@ class TypeCoercionSuite extends TypeCoercionSuiteBase {
:: Literal.create(null, DecimalType(22, 10))
:: Literal.create(null, DecimalType(38, 38))
:: Nil),
CreateArray(Literal.create(null, DecimalType(5, 3)).cast(DecimalType(38, 38))
:: Literal.create(null, DecimalType(22, 10)).cast(DecimalType(38, 38))
:: Literal.create(null, DecimalType(38, 38))
CreateArray(Literal.create(null, DecimalType(5, 3)).cast(DecimalType(38, 26))
:: Literal.create(null, DecimalType(22, 10)).cast(DecimalType(38, 26))
:: Literal.create(null, DecimalType(38, 38)).cast(DecimalType(38, 26))
:: Nil))
}

Expand Down Expand Up @@ -1095,9 +1095,9 @@ class TypeCoercionSuite extends TypeCoercionSuiteBase {
:: Literal.create(null, DecimalType(38, 38))
:: Nil),
CreateMap(Literal(1)
:: Literal.create(null, DecimalType(38, 0)).cast(DecimalType(38, 38))
:: Literal.create(null, DecimalType(38, 0))
:: Literal(2)
:: Literal.create(null, DecimalType(38, 38))
:: Literal.create(null, DecimalType(38, 38)).cast(DecimalType(38, 0))
:: Nil))
// type coercion for both map keys and values
ruleTest(TypeCoercion.FunctionArgumentConversion,
Expand Down