Skip to content

[SPARK-24734][SQL] Fix type coercions and nullabilities of nested data types of some functions. #21704

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
ueshin wants to merge 21 commits into from
Closed

[SPARK-24734][SQL] Fix type coercions and nullabilities of nested data types of some functions. #21704

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d87a8c6
Fix containsNull of Concat for array type.
ueshin Jul 3, 2018
30d5aed
Add a test.
ueshin Jul 4, 2018
2e624df
Revert "Fix containsNull of Concat for array type."
ueshin Jul 9, 2018
fa73b32
Merge branch 'master' into issues/SPARK-24734/concat_containsnull
ueshin Jul 9, 2018
da0702b
Suppress unneeded casts.
ueshin Jul 9, 2018
b31e401
Fix `Concat`.
ueshin Jul 9, 2018
7a838b0
Fix `CreateArray`.
ueshin Jul 9, 2018
01c9ff3
Fix `CreateMap`.
ueshin Jul 9, 2018
b2ca587
Fix `Coalesce`.
ueshin Jul 9, 2018
3d8891e
Fix `Least` and `Greatest`.
ueshin Jul 9, 2018
1fa692a
Merge branch 'master' into issues/SPARK-24734/concat_containsnull
ueshin Jul 9, 2018
444383d
Fix `MapConcat`.
ueshin Jul 9, 2018
3e1f7e4
Fix udts.
ueshin Jul 10, 2018
2c54e38
Rename to `findCommonTypeDifferentOnlyInNullFlags`.
ueshin Jul 10, 2018
5f1b865
Merge branch 'master' into issues/SPARK-24734/concat_containsnull
ueshin Jul 11, 2018
2ab025f
Use `ComplexTypeMergingExpression` trait.
ueshin Jul 11, 2018
db254e5
Fix udts.
ueshin Jul 11, 2018
b412f7b
Merge branch 'master' into issues/SPARK-24734/concat_containsnull
ueshin Jul 12, 2018
f701242
Make `ArraySetLike` `ComplexTypeMergingExpression`.
ueshin Jul 12, 2018
5115961
Use `TypeCoercion.haveSameType`.
ueshin Jul 13, 2018
e489e8b
Revert "Make `ArraySetLike` `ComplexTypeMergingExpression`."
ueshin Jul 16, 2018
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
Prev Previous commit
Next Next commit
Suppress unneeded casts.
  • Loading branch information
@ueshin
ueshin committed Jul 9, 2018
commit da0702b80675e1bed74cee99cf1856ed34c10803
33 changes: 24 additions & 9 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,22 @@ object TypeCoercion {
}
}

private def haveSameType(exprs: Seq[Expression]): Boolean =
exprs.map(_.dataType).distinct.length == 1
private def haveSameType(exprs: Seq[Expression]): Boolean = {
Copy link
Contributor

@cloud-fan cloud-fan Jul 12, 2018

Choose a reason for hiding this comment

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

This is duplicated with ComplexTypeMergingExpression.areInputTypesForMergingEqual, can we unify them? We can

  1. remove hasSameType. Any expression that needs to do this check should extend ComplexTypeMergingExpression and the TypeCoercion rule should call areInputTypesForMergingEqual.
  2. remove areInputTypesForMergingEqual. ComplexTypeMergingExpression should only define the list of data types that need to be merged, and TypeCoercion rule should call hasSameType(e.inputTypesForMerging)

Copy link
Member Author

@ueshin ueshin Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Since we have CreateArray and CreateMap, we can't make all such expressions ComplexTypeMergingExpression.
I'd apply 2) approach.

if (exprs.size <= 1) {
true
} else {
val head = exprs.head.dataType
exprs.tail.forall(_.dataType.sameType(head))
}
}

private def castIfNotSameType(expr: Expression, dt: DataType): Expression = {
if (!expr.dataType.sameType(dt)) {
Cast(expr, dt)
} else {
expr
}
}

/**
* Widens numeric types and converts strings to numbers when appropriate.
Expand Down Expand Up @@ -513,6 +527,7 @@ object TypeCoercion {
* This ensure that the types for various functions are as expected.
*/
object FunctionArgumentConversion extends TypeCoercionRule {

override protected def coerceTypes(
plan: LogicalPlan): LogicalPlan = plan transformAllExpressions {
// Skip nodes who's children have not been resolved yet.
Expand All @@ -521,15 +536,15 @@ object TypeCoercion {
case a @ CreateArray(children) if !haveSameType(children) =>
val types = children.map(_.dataType)
findWiderCommonType(types) match {
case Some(finalDataType) => CreateArray(children.map(Cast(_, finalDataType)))
case Some(finalDataType) => CreateArray(children.map(castIfNotSameType(_, finalDataType)))
Copy link
Contributor

@cloud-fan cloud-fan Jul 11, 2018

Choose a reason for hiding this comment

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

is it needed? I think optimizer can remove unnecessary cast.

Copy link
Member Author

@ueshin ueshin Jul 12, 2018

Choose a reason for hiding this comment

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

Currently the optimizer doesn't remove the cast when the difference of the finalDataType is only the nullabilities of nested types.

case None => a
}

case c @ Concat(children) if children.forall(c => ArrayType.acceptsType(c.dataType)) &&
!haveSameType(children) =>
val types = children.map(_.dataType)
findWiderCommonType(types) match {
case Some(finalDataType) => Concat(children.map(Cast(_, finalDataType)))
case Some(finalDataType) => Concat(children.map(castIfNotSameType(_, finalDataType)))
case None => c
}

Expand All @@ -555,7 +570,7 @@ object TypeCoercion {
} else {
val types = m.keys.map(_.dataType)
findWiderCommonType(types) match {
case Some(finalDataType) => m.keys.map(Cast(_, finalDataType))
case Some(finalDataType) => m.keys.map(castIfNotSameType(_, finalDataType))
case None => m.keys
}
}
Expand All @@ -565,7 +580,7 @@ object TypeCoercion {
} else {
val types = m.values.map(_.dataType)
findWiderCommonType(types) match {
case Some(finalDataType) => m.values.map(Cast(_, finalDataType))
case Some(finalDataType) => m.values.map(castIfNotSameType(_, finalDataType))
case None => m.values
}
}
Expand Down Expand Up @@ -593,7 +608,7 @@ object TypeCoercion {
case c @ Coalesce(es) if !haveSameType(es) =>
val types = es.map(_.dataType)
findWiderCommonType(types) match {
case Some(finalDataType) => Coalesce(es.map(Cast(_, finalDataType)))
case Some(finalDataType) => Coalesce(es.map(castIfNotSameType(_, finalDataType)))
case None => c
}

Expand All @@ -603,14 +618,14 @@ object TypeCoercion {
case g @ Greatest(children) if !haveSameType(children) =>
val types = children.map(_.dataType)
findWiderTypeWithoutStringPromotion(types) match {
case Some(finalDataType) => Greatest(children.map(Cast(_, finalDataType)))
case Some(finalDataType) => Greatest(children.map(castIfNotSameType(_, finalDataType)))
case None => g
}

case l @ Least(children) if !haveSameType(children) =>
val types = children.map(_.dataType)
findWiderTypeWithoutStringPromotion(types) match {
case Some(finalDataType) => Least(children.map(Cast(_, finalDataType)))
case Some(finalDataType) => Least(children.map(castIfNotSameType(_, finalDataType)))
case None => l
}

Expand Down
43 changes: 20 additions & 23 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 @@ -687,46 +687,43 @@ class TypeCoercionSuite extends AnalysisTest {

ruleTest(rule,
Coalesce(Seq(doubleLit, intLit, floatLit)),
Coalesce(Seq(Cast(doubleLit, DoubleType),
Cast(intLit, DoubleType), Cast(floatLit, DoubleType))))
Coalesce(Seq(doubleLit, Cast(intLit, DoubleType), Cast(floatLit, DoubleType))))

ruleTest(rule,
Coalesce(Seq(longLit, intLit, decimalLit)),
Coalesce(Seq(Cast(longLit, DecimalType(22, 0)),
Cast(intLit, DecimalType(22, 0)), Cast(decimalLit, DecimalType(22, 0)))))
Cast(intLit, DecimalType(22, 0)), decimalLit)))

ruleTest(rule,
Coalesce(Seq(nullLit, intLit)),
Coalesce(Seq(Cast(nullLit, IntegerType), Cast(intLit, IntegerType))))
Coalesce(Seq(Cast(nullLit, IntegerType), intLit)))

ruleTest(rule,
Coalesce(Seq(timestampLit, stringLit)),
Coalesce(Seq(Cast(timestampLit, StringType), Cast(stringLit, StringType))))
Coalesce(Seq(Cast(timestampLit, StringType), stringLit)))

ruleTest(rule,
Coalesce(Seq(nullLit, floatNullLit, intLit)),
Coalesce(Seq(Cast(nullLit, FloatType), Cast(floatNullLit, FloatType),
Cast(intLit, FloatType))))
Coalesce(Seq(Cast(nullLit, FloatType), floatNullLit, Cast(intLit, FloatType))))

ruleTest(rule,
Coalesce(Seq(nullLit, intLit, decimalLit, doubleLit)),
Coalesce(Seq(Cast(nullLit, DoubleType), Cast(intLit, DoubleType),
Cast(decimalLit, DoubleType), Cast(doubleLit, DoubleType))))
Cast(decimalLit, DoubleType), doubleLit)))

ruleTest(rule,
Coalesce(Seq(nullLit, floatNullLit, doubleLit, stringLit)),
Coalesce(Seq(Cast(nullLit, StringType), Cast(floatNullLit, StringType),
Cast(doubleLit, StringType), Cast(stringLit, StringType))))
Cast(doubleLit, StringType), stringLit)))

ruleTest(rule,
Coalesce(Seq(timestampLit, intLit, stringLit)),
Coalesce(Seq(Cast(timestampLit, StringType), Cast(intLit, StringType),
Cast(stringLit, StringType))))
Coalesce(Seq(Cast(timestampLit, StringType), Cast(intLit, StringType), stringLit)))

ruleTest(rule,
Coalesce(Seq(tsArrayLit, intArrayLit, strArrayLit)),
Coalesce(Seq(Cast(tsArrayLit, ArrayType(StringType)),
Cast(intArrayLit, ArrayType(StringType)), Cast(strArrayLit, ArrayType(StringType)))))
Cast(intArrayLit, ArrayType(StringType)), strArrayLit)))
}

test("CreateArray casts") {
Expand All @@ -735,7 +732,7 @@ class TypeCoercionSuite extends AnalysisTest {
:: Literal(1)
:: Literal.create(1.0, FloatType)
:: Nil),
CreateArray(Cast(Literal(1.0), DoubleType)
CreateArray(Literal(1.0)
:: Cast(Literal(1), DoubleType)
:: Cast(Literal.create(1.0, FloatType), DoubleType)
:: Nil))
Expand All @@ -747,7 +744,7 @@ class TypeCoercionSuite extends AnalysisTest {
:: Nil),
CreateArray(Cast(Literal(1.0), StringType)
:: Cast(Literal(1), StringType)
:: Cast(Literal("a"), StringType)
:: Literal("a")
:: Nil))

ruleTest(TypeCoercion.FunctionArgumentConversion,
Expand All @@ -765,7 +762,7 @@ class TypeCoercionSuite extends AnalysisTest {
:: 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)).cast(DecimalType(38, 38))
:: Literal.create(null, DecimalType(38, 38))
:: Nil))
}

Expand All @@ -779,7 +776,7 @@ class TypeCoercionSuite extends AnalysisTest {
:: Nil),
CreateMap(Cast(Literal(1), FloatType)
:: Literal("a")
:: Cast(Literal.create(2.0, FloatType), FloatType)
:: Literal.create(2.0, FloatType)
:: Literal("b")
:: Nil))
ruleTest(TypeCoercion.FunctionArgumentConversion,
Expand All @@ -801,7 +798,7 @@ class TypeCoercionSuite extends AnalysisTest {
:: Literal(3.0)
:: Nil),
CreateMap(Literal(1)
:: Cast(Literal("a"), StringType)
:: Literal("a")
:: Literal(2)
:: Cast(Literal(3.0), StringType)
:: Nil))
Expand All @@ -814,7 +811,7 @@ class TypeCoercionSuite extends AnalysisTest {
CreateMap(Literal(1)
:: Literal.create(null, DecimalType(38, 0)).cast(DecimalType(38, 38))
:: Literal(2)
:: Literal.create(null, DecimalType(38, 38)).cast(DecimalType(38, 38))
:: Literal.create(null, DecimalType(38, 38))
:: Nil))
// type coercion for both map keys and values
ruleTest(TypeCoercion.FunctionArgumentConversion,
Expand All @@ -824,8 +821,8 @@ class TypeCoercionSuite extends AnalysisTest {
:: Literal(3.0)
:: Nil),
CreateMap(Cast(Literal(1), DoubleType)
:: Cast(Literal("a"), StringType)
:: Cast(Literal(2.0), DoubleType)
:: Literal("a")
:: Literal(2.0)
:: Cast(Literal(3.0), StringType)
:: Nil))
}
Expand All @@ -837,7 +834,7 @@ class TypeCoercionSuite extends AnalysisTest {
:: Literal(1)
:: Literal.create(1.0, FloatType)
:: Nil),
operator(Cast(Literal(1.0), DoubleType)
operator(Literal(1.0)
:: Cast(Literal(1), DoubleType)
:: Cast(Literal.create(1.0, FloatType), DoubleType)
:: Nil))
Expand All @@ -848,14 +845,14 @@ class TypeCoercionSuite extends AnalysisTest {
:: Nil),
operator(Cast(Literal(1L), DecimalType(22, 0))
:: Cast(Literal(1), DecimalType(22, 0))
:: Cast(Literal(new java.math.BigDecimal("1000000000000000000000")), DecimalType(22, 0))
:: Literal(new java.math.BigDecimal("1000000000000000000000"))
:: Nil))
ruleTest(TypeCoercion.FunctionArgumentConversion,
operator(Literal(1.0)
:: Literal.create(null, DecimalType(10, 5))
:: Literal(1)
:: Nil),
operator(Literal(1.0).cast(DoubleType)
operator(Literal(1.0)
:: Literal.create(null, DecimalType(10, 5)).cast(DoubleType)
:: Literal(1).cast(DoubleType)
:: Nil))
Expand Down