[SPARK-39015][SQL] Remove the usage of toSQLValue(v) without an explicit type

[HyukjinKwon]

What changes were proposed in this pull request?

This PR proposes to remove the the usage of toSQLValue(v) without an explicit type.

Literal(v) is intended to be used from end-users so it cannot handle our internal types such as UTF8String and ArrayBasedMapData. Using this method can lead to unexpected error messages such as:

Caused by: org.apache.spark.SparkRuntimeException: [UNSUPPORTED_FEATURE] The feature is not supported: literal for 'hair' of class org.apache.spark.unsafe.types.UTF8String.
  at org.apache.spark.sql.errors.QueryExecutionErrors$.literalTypeUnsupportedError(QueryExecutionErrors.scala:241)
  at org.apache.spark.sql.catalyst.expressions.Literal$.apply(literals.scala:99)
  at org.apache.spark.sql.errors.QueryErrorsBase.toSQLValue(QueryErrorsBase.scala:45)
  ...

Since It is impossible to have the corresponding data type from the internal types as one type can map to multiple external types (e.g., Long for Timestamp, TimestampNTZ, and LongType), the removal approach was taken.

Why are the changes needed?

To provide the error messages as intended.

Does this PR introduce any user-facing change?

Yes.

import org.apache.spark.sql.Row
import org.apache.spark.sql.types.StructType
import org.apache.spark.sql.types.StringType
import org.apache.spark.sql.types.DataTypes

val arrayStructureData = Seq(
Row(Map("hair"->"black", "eye"->"brown")),
Row(Map("hair"->"blond", "eye"->"blue")),
Row(Map()))

val mapType  = DataTypes.createMapType(StringType, StringType)

val arrayStructureSchema = new StructType().add("properties", mapType)

val mapTypeDF = spark.createDataFrame(
    spark.sparkContext.parallelize(arrayStructureData),arrayStructureSchema)

spark.conf.set("spark.sql.ansi.enabled", true)
mapTypeDF.selectExpr("element_at(properties, 'hair')").show

Before:

Caused by: org.apache.spark.SparkRuntimeException: [UNSUPPORTED_FEATURE] The feature is not supported: literal for 'hair' of class org.apache.spark.unsafe.types.UTF8String.
  at org.apache.spark.sql.errors.QueryExecutionErrors$.literalTypeUnsupportedError(QueryExecutionErrors.scala:241)
  at org.apache.spark.sql.catalyst.expressions.Literal$.apply(literals.scala:99)
  at org.apache.spark.sql.errors.QueryErrorsBase.toSQLValue(QueryErrorsBase.scala:45)
  ...

After:

Caused by: org.apache.spark.SparkNoSuchElementException: [MAP_KEY_DOES_NOT_EXIST] Key 'hair' does not exist. To return NULL instead, use 'try_element_at'. If necessary set spark.sql.ansi.enabled to false to bypass this error.
== SQL(line 1, position 0) ==
element_at(properties, 'hair')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

How was this patch tested?

Unittest was added. Otherwise, existing test cases should cover.

[HyukjinKwon]

cc @gengliangwang @MaxGekk FYI

[MaxGekk]

Literal(v) is intended to be used from end-users so it cannot handle our internal types such as UTF8String and ArrayBasedMapData.

When I added two methods toSQLValue(), I supposed to use:

• def toSQLValue(v: Any, t: DataType) with internal value
• def toSQLValue(v: Any): String w/ external like String

If you need to pass somewhere an internal value, just use the first method. Why do you need to remove the second one, I didn't get.

[HyukjinKwon]

def toSQLValue(v: Any): String w/ external like String

Actually, I think this is error-prone (e.g., see the reported and fixed case here). With def toSQLValue(v: Any, t: DataType), you can use it everywhere without thinking about internal or external.

[MaxGekk]

you can use it everywhere without thinking about internal or external

Now, you have to convert String to UTF8String (maybe other values) everywhere which is inconvenient.

[MaxGekk]

Can't you just use correct function, and don't remove another one. And add comments to functions. Removing the second function seems like unrelated to the fix.

[HyukjinKwon]

Now, you have to convert String to UTF8String (maybe other values) everywhere which is inconvenient.

We actually don't need to convert. Both Literal.create(a: String, StringType) and Literal.create(a: UTF8String, StringType) work. But Liternal(s: UTF8String) doesn't work.

[HyukjinKwon]

This is anther example of being error-prone.

[HyukjinKwon]

I thought that it's easier to have one function to use it everywhere vs having two to use. I can separate the fix if you still think it's arguable.

[MaxGekk]

I can separate the fix if you still think it's arguable.

Isn't needed, if it is possible to point out the type in all cases and toSQLValue() can handle "external" types as well.

[HyukjinKwon]

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala#L155-L164

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala#L500-L522

Here, all external types are listed - if they are some missing, it's a bug. They are converted to Literal(internal value).
If the given value isn't external type, they will be just Literal(v).

There seems two cases missing Char and collection.mutable.WrappedArray where both were added lately. I think they missed to fix ti here together.

[HyukjinKwon]

There's another case this PR fixes.

[MaxGekk]

+1, LGTM. Merging to master.
Thank you, @HyukjinKwon.

[MaxGekk]

@HyukjinKwon Could you backport the changes to branch-3.3, please.

[HyukjinKwon]

sure