[SPARK-8010][SQL]Promote types to StringType as implicit conversion in non-binary expression of HiveTypeCoercion

[OopsOutOfMemory]

1. Given a query
   select coalesce(null, 1, '1') from dual will cause exception:
   java.lang.RuntimeException: Could not determine return type of Coalesce for IntegerType,StringType
2. Given a query:
   select case when true then 1 else '1' end from dual will cause exception:
   java.lang.RuntimeException: Types in CASE WHEN must be the same or coercible to a common type: StringType != IntegerType
   I checked the code, the main cause is the HiveTypeCoercion doesn't do implicit convert when there is a IntegerType and StringType.

Numeric types can be promoted to string type

Hive will always do this implicit conversion.

[OopsOutOfMemory]

@chenghao-intel Could u please review this?

[cloud-fan]

numericPrecedence doesn't contain StringType, you need to write a separate case like case (t1 @ StringType(), t2 @ NumericType()).

[cloud-fan]

findTightestCommonType is also used in Rule WidenTypes which is run before PromoteStrings, and your change will break some special logic for binary comparison in PromoteStrings. We need to be careful to avoid breaking existing rules.

[chenghao-intel]

It's better to update the rule for CaseWhenCoercion and Coalesce directly, not provide a general rule, which probably break the existed logic as @cloud-fan said.

Beside, can you also add a unit test to compare the hive answer?

[OopsOutOfMemory]

Thanks, @cloud-fan and @chenghao-intel. I will update it tomorrow night.

[OopsOutOfMemory]

@cloud-fan and @chenghao-intel. Updated, Any more comments?

[chenghao-intel]

Can you compare the result with Hive?

[chenghao-intel]

And probably it's better to move the test into https://github.com/OopsOutOfMemory/spark/blob/pnts/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

[OopsOutOfMemory]

@cloud-fan and @chenghao-intel
Resolve conflicts and test suite passed locally.
Would you mind to review this?

[OopsOutOfMemory]

@rxin @marmbrus

[cloud-fan]

How about ArrayType, StructType, etc.? Does hive has a document for this?

[OopsOutOfMemory]

yes, @cloud-fan
you can refer the chart at the bottom of page in https://cwiki.apache.org/confluence/display/Hive/LanguageManual+Types

This is only for primitive type.

[cloud-fan]

Then I think we should add t2.isInstanceOf[AtomicType] to make sure it's primitive type.

[OopsOutOfMemory]

@cloud-fan That's make sense, thanks. Updated.

[OopsOutOfMemory]

ping..

[rxin]

why not make this a function instead?

[rxin]

and make it private

[rxin]

@cloud-fan and @yhuai can you both take another look at this?

[cloud-fan]

cc @rxin , findTightestCommonTypeOfTwo is a function and I think we should make them consistent...

[OopsOutOfMemory]

@cloud-fan I'm sorry, I don't quite understand when to use a function or when to use a method. And what's the purpose here to use a function.

[cloud-fan]

Here we don't care about the parameter names(i.e. t1 and t2). Using functions, we can just write the partially applied function(i.e. {case ...}) and don't need to add (t1, t2) match before it.
However, it's not a big deal, using method is OK, but we should make it consistent with existing code.

[OopsOutOfMemory]

ping...
May AmplabJenkins test this please?

[cloud-fan]

LGTM, but we need a more general rule to handle these promote-string stuff...

[marmbrus]

ok to test

[SparkQA]

Test build #34894 has finished for PR 6551 at commit 7a209d7.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[marmbrus]

Thanks! Merging to master.