Skip to content

[SPARK-21811][SQL] Fix the inconsistency behavior when finding the widest common type #21074

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

[SPARK-21811][SQL] Fix the inconsistency behavior when finding the widest common type #21074

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
803a6a4
fix type coercion when promting to StringType
jiangxb1987 Apr 15, 2018
571912f
refactor and update migration guide.
jiangxb1987 Apr 17, 2018
d77f213
update comments
jiangxb1987 Apr 17, 2018
4ce5081
fix also for array types
jiangxb1987 Apr 18, 2018
55eefe0
refactor
jiangxb1987 Apr 18, 2018
c2abce2
update comments
jiangxb1987 Apr 18, 2018
ff514af
update migration guide
jiangxb1987 Apr 18, 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
fix also for array types
  • Loading branch information
@jiangxb1987
jiangxb1987 committed Apr 18, 2018
commit 4ce5081fb2da8dabb216413fdda4da0f0b061f71
4 changes: 3 additions & 1 deletion 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 @@ -180,7 +180,9 @@ object TypeCoercion {
// to a op (b op c). This is only a problem for StringType. Excluding StringType,
Copy link
Contributor

@cloud-fan cloud-fan Apr 18, 2018

Choose a reason for hiding this comment

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

This is only a problem for StringType or nested StringType in ArrayType. Excluding these types, ...

// findWiderTypeForTwo satisfies the associative law. For instance, (TimestampType,
// IntegerType, StringType) should have StringType as the wider common type.
val (stringTypes, nonStringTypes) = types.partition(_ == StringType)
val (stringTypes, nonStringTypes) = types.partition { t =>
t == StringType || t == ArrayType(StringType)
Copy link
Contributor

@cloud-fan cloud-fan Apr 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

we need something like

def hasStringType(dt: DataType): Boolean = dt match {
  case StringType => true
  case ArrayType(et, _) => hasStringType(et)
  // Add StructType if we support string promotion for struct fields in the future.
  case _ => false
}

}
(stringTypes.distinct ++ nonStringTypes).foldLeft[Option[DataType]](Some(NullType))((r, c) =>
r match {
case Some(d) => findWiderTypeForTwo(d, c)
Expand Down
8 changes: 8 additions & 0 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 @@ -539,6 +539,9 @@ class TypeCoercionSuite extends AnalysisTest {
val floatLit = Literal.create(1.0f, FloatType)
val timestampLit = Literal.create("2017-04-12", TimestampType)
val decimalLit = Literal(new java.math.BigDecimal("1000000000000000000000"))
val tsArrayLit = Literal(Array(new Timestamp(System.currentTimeMillis())))
val strArrayLit = Literal(Array("c"))
val intArrayLit = Literal(Array(1))

ruleTest(rule,
Coalesce(Seq(doubleLit, intLit, floatLit)),
Expand Down Expand Up @@ -577,6 +580,11 @@ class TypeCoercionSuite extends AnalysisTest {
Coalesce(Seq(timestampLit, intLit, stringLit)),
Coalesce(Seq(Cast(timestampLit, StringType), Cast(intLit, StringType),
Cast(stringLit, StringType))))

ruleTest(rule,
Coalesce(Seq(tsArrayLit, intArrayLit, strArrayLit)),
Coalesce(Seq(Cast(tsArrayLit, ArrayType(StringType)),
Cast(intArrayLit, ArrayType(StringType)), Cast(strArrayLit, ArrayType(StringType)))))
Copy link
Member

@gatorsmile gatorsmile Apr 18, 2018

Choose a reason for hiding this comment

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

Could you add an end to end test case that can trigger this?

Copy link
Contributor

@cloud-fan cloud-fan Apr 19, 2018

Choose a reason for hiding this comment

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

We usually don't add end-to-end tests for type coercion changes, as the type coercion logic is pretty isolated, it's very unlikely that we can pass the unit test but not end-to-end test.

}

test("CreateArray casts") {
Expand Down