HyukjinKwon commented on a change in pull request #28593: URL: https://github.com/apache/spark/pull/28593#discussion_r428488834
########## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ########## @@ -2586,6 +2586,22 @@ object SQLConf { .checkValue(_ > 0, "The timeout value must be positive") .createWithDefault(10L) + val LEGACY_NUMERIC_CONVERT_TO_TIMESTAMP_ENABLE = + buildConf("spark.sql.legacy.numericConvertToTimestampEnable") + .doc("when true,use legacy numberic can convert to timestamp") + .version("3.0.0") + .booleanConf + .createWithDefault(false) + + val LEGACY_NUMERIC_CONVERT_TO_TIMESTAMP_IN_SECONDS = + buildConf("spark.sql.legacy.numericConvertToTimestampInSeconds") + .internal() + .doc("The legacy only works when LEGACY_NUMERIC_CONVERT_TO_TIMESTAMP_ENABLE is true." + + "when true,the value will be interpreted as seconds,which follow spark style," + + "when false,value is interpreted as milliseconds,which follow hive style") Review comment: This kind of compatibility isn't fully guaranteed in Spark, see also [Compatibility with Apache Hive](https://spark.apache.org/docs/latest/sql-migration-guide-hive-compatibility.html). Simply following Hive doesn't justify this PR. There are already a bunch of mismatched behaviours and I don't like to target more compatibility, in particular, by fixing the basic functionalities such as cast and adding such switches to maintain. Why is it difficult to just add `ts / 1000`? The workaround seems very easy. If we target to get rid of `cast(ts as long)` away, adding separate functions is a-okay because it doesn't affect existing users, and also looks other DBMSs have their own ways by having such functions in general. Looks we will have workarounds once these functions from https://github.com/apache/spark/pull/28534 are merged, and seems you can leverage these functions alternatively as well. I would say a-no to fix a basic functionality to have another variant and non-standard behaviour, which could potentially trigger to have another set of non-standard behaviours in many other places in Spark. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org