-
Notifications
You must be signed in to change notification settings - Fork 29.1k
[SPARK-19435][SQL] Type coercion between ArrayTypes #16777
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,8 @@ class TypeCoercionSuite extends PlanTest { | |
| // | NullType | ByteType | ShortType | IntegerType | LongType | DoubleType | FloatType | Dec(10, 2) | BinaryType | BooleanType | StringType | DateType | TimestampType | ArrayType | MapType | StructType | NullType | CalendarIntervalType | DecimalType(38, 18) | DoubleType | IntegerType | | ||
| // | CalendarIntervalType | X | X | X | X | X | X | X | X | X | X | X | X | X | X | X | X | CalendarIntervalType | X | X | X | | ||
| // +----------------------+----------+-----------+-------------+----------+------------+-----------+------------+------------+-------------+------------+----------+---------------+------------+----------+-------------+----------+----------------------+---------------------+-------------+--------------+ | ||
| // Note: ArrayType*, MapType*, StructType* are castable only when the internal child types also match; otherwise, not castable | ||
| // Note: MapType*, StructType* are castable only when the internal child types also match; otherwise, not castable. | ||
| // Note: ArrayType* is castable when the element type is castable according to the table. | ||
|
||
| // scalastyle:on line.size.limit | ||
|
|
||
| private def shouldCast(from: DataType, to: AbstractDataType, expected: DataType): Unit = { | ||
|
|
@@ -125,6 +126,20 @@ class TypeCoercionSuite extends PlanTest { | |
| } | ||
| } | ||
|
|
||
| private def checkWidenType( | ||
| widenFunc: (DataType, DataType) => Option[DataType], | ||
| t1: DataType, | ||
| t2: DataType, | ||
| expected: Option[DataType]): Unit = { | ||
| var found = widenFunc(t1, t2) | ||
| assert(found == expected, | ||
| s"Expected $expected as wider common type for $t1 and $t2, found $found") | ||
| // Test both directions to make sure the widening is symmetric. | ||
| found = widenFunc(t2, t1) | ||
| assert(found == expected, | ||
| s"Expected $expected as wider common type for $t2 and $t1, found $found") | ||
| } | ||
|
|
||
| test("implicit type cast - ByteType") { | ||
| val checkedType = ByteType | ||
| checkTypeCasting(checkedType, castableTypes = numericTypes ++ Seq(StringType)) | ||
|
|
@@ -308,15 +323,8 @@ class TypeCoercionSuite extends PlanTest { | |
| } | ||
|
|
||
| test("tightest common bound for types") { | ||
| def widenTest(t1: DataType, t2: DataType, tightestCommon: Option[DataType]) { | ||
| var found = TypeCoercion.findTightestCommonType(t1, t2) | ||
| assert(found == tightestCommon, | ||
| s"Expected $tightestCommon as tightest common type for $t1 and $t2, found $found") | ||
| // Test both directions to make sure the widening is symmetric. | ||
| found = TypeCoercion.findTightestCommonType(t2, t1) | ||
| assert(found == tightestCommon, | ||
| s"Expected $tightestCommon as tightest common type for $t2 and $t1, found $found") | ||
| } | ||
| def widenTest(t1: DataType, t2: DataType, expected: Option[DataType]): Unit = | ||
| checkWidenType(TypeCoercion.findTightestCommonType, t1, t2, expected) | ||
|
|
||
| // Null | ||
| widenTest(NullType, NullType, Some(NullType)) | ||
|
|
@@ -355,7 +363,6 @@ class TypeCoercionSuite extends PlanTest { | |
| widenTest(DecimalType(2, 1), DoubleType, None) | ||
| widenTest(DecimalType(2, 1), IntegerType, None) | ||
| widenTest(DoubleType, DecimalType(2, 1), None) | ||
| widenTest(IntegerType, DecimalType(2, 1), None) | ||
|
|
||
| // StringType | ||
| widenTest(NullType, StringType, Some(StringType)) | ||
|
|
@@ -379,6 +386,60 @@ class TypeCoercionSuite extends PlanTest { | |
| widenTest(ArrayType(IntegerType), StructType(Seq()), None) | ||
| } | ||
|
|
||
| test("wider common type for decimal and array") { | ||
| def widenTestWithStringPromotion( | ||
| t1: DataType, | ||
| t2: DataType, | ||
| expected: Option[DataType]): Unit = { | ||
| checkWidenType(TypeCoercion.findWiderTypeForTwo, t1, t2, expected) | ||
| } | ||
|
|
||
| def widenTestWithoutStringPromotion( | ||
| t1: DataType, | ||
| t2: DataType, | ||
| expected: Option[DataType]): Unit = { | ||
| checkWidenType(TypeCoercion.findWiderTypeWithoutStringPromotionForTwo, t1, t2, expected) | ||
| } | ||
|
|
||
| // Decimal | ||
| widenTestWithStringPromotion( | ||
| DecimalType(2, 1), DecimalType(3, 2), Some(DecimalType(3, 2))) | ||
| widenTestWithStringPromotion( | ||
| DecimalType(2, 1), DoubleType, Some(DoubleType)) | ||
| widenTestWithStringPromotion( | ||
| DecimalType(2, 1), IntegerType, Some(DecimalType(11, 1))) | ||
| widenTestWithStringPromotion( | ||
| DecimalType(2, 1), LongType, Some(DecimalType(21, 1))) | ||
|
|
||
| // ArrayType | ||
| widenTestWithStringPromotion( | ||
| ArrayType(ShortType, containsNull = true), | ||
| ArrayType(DoubleType, containsNull = false), | ||
| Some(ArrayType(DoubleType, containsNull = true))) | ||
| widenTestWithStringPromotion( | ||
| ArrayType(TimestampType, containsNull = false), | ||
| ArrayType(StringType, containsNull = true), | ||
| Some(ArrayType(StringType, containsNull = true))) | ||
| widenTestWithStringPromotion( | ||
| ArrayType(ArrayType(IntegerType), containsNull = false), | ||
| ArrayType(ArrayType(LongType), containsNull = false), | ||
| Some(ArrayType(ArrayType(LongType), containsNull = false))) | ||
|
|
||
| // Without string promotion | ||
| widenTestWithoutStringPromotion(IntegerType, StringType, None) | ||
|
||
| widenTestWithoutStringPromotion(StringType, TimestampType, None) | ||
| widenTestWithoutStringPromotion(ArrayType(LongType), ArrayType(StringType), None) | ||
| widenTestWithoutStringPromotion(ArrayType(StringType), ArrayType(TimestampType), None) | ||
|
|
||
| // String promotion | ||
| widenTestWithStringPromotion(IntegerType, StringType, Some(StringType)) | ||
| widenTestWithStringPromotion(StringType, TimestampType, Some(StringType)) | ||
| widenTestWithStringPromotion( | ||
| ArrayType(LongType), ArrayType(StringType), Some(ArrayType(StringType))) | ||
| widenTestWithStringPromotion( | ||
| ArrayType(StringType), ArrayType(TimestampType), Some(ArrayType(StringType))) | ||
| } | ||
|
|
||
| private def ruleTest(rule: Rule[LogicalPlan], initial: Expression, transformed: Expression) { | ||
| ruleTest(Seq(rule), initial, transformed) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea we changed the order, but looks like it won't change the result.
findWiderTypeForDecimalwill always return a result for decimal type and numeric type, and iffindTightestCommonTypecan return a result,findWiderTypeForDecimalwill return the same result. So it doesn't matter if we runfindTightestCommonTypebefore or after it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Integer will be promoted to a wider Decimal anyway.