[SPARK-28497][SQL] Disallow upcasting complex data types to string type#25242
[SPARK-28497][SQL] Disallow upcasting complex data types to string type#25242gengliangwang wants to merge 4 commits intoapache:masterfrom
Conversation
|
good catch! can we also add an end-to-end test in |
|
Test build #108086 has finished for PR 25242 at commit
|
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
Show resolved
Hide resolved
|
Test build #108108 has finished for PR 25242 at commit
|
| test("SPARK-28497: complex type is not compatible with string encoder schema") { | ||
| val encoder = ExpressionEncoder[String] | ||
|
|
||
| { |
There was a problem hiding this comment.
Each block might be easily extracted with inner function to remove duplications. attr (element in attrs), and string representation of attr type are different and remaining is exactly same.
| { | ||
| val attrs = Seq('a.struct('x.long)) | ||
| assert(intercept[AnalysisException](encoder.resolveAndBind(attrs)).message == | ||
| s""" |
There was a problem hiding this comment.
super nit: its ok just check .contains("Cannot up cast a from struct<x:bigint> to string")?
There was a problem hiding this comment.
I am OK with either way. I was following the other test cases in the suite. Let me change it to .contains...
| val attrs = Seq('a.array(StringType)) | ||
| assert(intercept[AnalysisException](encoder.resolveAndBind(attrs)).message == | ||
| s""" | ||
| |Cannot up cast `a` from array<string> to string. |
There was a problem hiding this comment.
It doesn't necessarily compare the whole message. We can check if the message contains some keywords.
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
Looks fine to me too otherwise.
|
Test build #108156 has finished for PR 25242 at commit
|
|
Merged to master. |
7 participants
What changes were proposed in this pull request?
In the current implementation. complex types like Array/Map/StructType are allowed to upcast as StringType.
This is not safe casting. We should disallow it.
How was this patch tested?
Update the existing test case