[GitHub] HeartSaVioR edited a comment on issue #23854: [SPARK-22000][SQL] Use String.valueOf to assign value to String type of field in Java Bean

[GitBox <gi...@apache.org>]

HeartSaVioR edited a comment on issue #23854: [SPARK-22000][SQL] Use String.valueOf to assign value to String type of field in Java Bean
URL: https://github.com/apache/spark/pull/23854#issuecomment-466158427
 
 
   First of all, I'm not a catalyst expert (actually I'm a beginner on this area) so I might be wrong, so please correct me if I'm mistaken.
   
   > The issue concerned calling `.toString` on primitive types, not `String`.
   
   Why it was happening is IntegerType is matched to `String type of field` when deserializing to Java Bean. If it was matched to Integer type of field, `Integer.valueOf()` would be called.
   
   Looks like Spark doesn't just restrict matching type should be same. It loosens the restriction and handles some implicit type conversion via leveraging Java API - and primitive types are not compatible with how String deals with (`.toString()`) whereas it can still be converted in other way (`String.valueOf()`).
   
   > This change alters behavior a little bit. Now a null is deserialized as `"null"` not `null`. I'm not sure we want that, nor was that an issue before; can this be called on null anyway?
   
   Unless I misread javadoc or javadoc is wrong, `StaticInvoke` would take care of that. This change would not incur calling `String.valueOf(null)`, otherwise passing null should be happening to other types in same if statement as well, and it will not fail to compile generated code but throw NumberFormatException for numeric types when generated code is running.
   
   https://github.com/apache/spark/blob/17d0cfcaa4a43fd55b81065d907538a9c1bf569b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala#L199-L220
   
   Btw I guess the case can be added in new UT - we don't need to worry about it after we apply the case in UT. I'll try.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org