thomasrebele commented on code in PR #6900: URL: https://github.com/apache/hive/pull/6900#discussion_r4336360931
########## serde/src/java/org/apache/hadoop/hive/serde2/OpenCSVSerde.java: ########## Review Comment: The serialization to JSON is different: in most cases [HiveJsonWriter#primitiveToNode](https://github.com/apache/hive/blob/852582d791da60886f8a1a5881bc31e5918a8fcc/serde/src/java/org/apache/hadoop/hive/serde2/json/HiveJsonWriter.java#L290) gets the primitive value and wraps it in a NumberNode. The most of the remaining cases (including TIMESTAMP and DATE) use toString to converts the result of #getPrimitiveJavaObject to a TextNode. While it is technically possible to combine these two conversions, I advice to keep it as it is, at least in the context of this PR. A refactoring could introduce backward compatibility issues. The PR aims to just fix a UnsupportedOperationException when disabling vectorization. Also, JSON differentiates between numbers and strings (see [examples](https://www.json.org/example.html)), so if we combined these two primitive to X conversions, we would need to handle this difference. Speaking about backward compatibility: if we change the serialization of timestamps here, the output of OpenCSVSerde would change, and all the users that are expecting the previous format would need to adapt. In summary, I would suggest keeping the conversion as-is, and postpone a refactoring to a follow-up ticket, if necessary. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
