[GitHub] [calcite] zhztheplayer commented on a change in pull request #1014: [CALCITE-2804] Druid adapter: Cast does not work when casting to timestamp

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

zhztheplayer commented on a change in pull request #1014: [CALCITE-2804] Druid adapter: Cast does not work when casting to timestamp
URL: https://github.com/apache/calcite/pull/1014#discussion_r275119933
 
 

 ##########
 File path: druid/src/main/java/org/apache/calcite/adapter/druid/DruidSqlCastConverter.java
 ##########
 @@ -129,13 +129,13 @@
   private static String castCharToDateTime(
       TimeZone timeZone,
       String operand,
-      final SqlTypeName toType, String format) {
+      final SqlTypeName toType, final SqlTypeName fromType) {
 
 Review comment:
   Though I can see the reason of this change,  to me it is more like a workaround. 
   
   Now in the PR we call the method by code:
   ```java
   castCharToDateTime(
             timeZone,
             castDateTimeToChar(DateTimeUtils.UTC_ZONE, operandExpression, fromType),
             toType,
             fromType);
   ```
   I suggest to change to something like:
   ```java
   castCharToDateTime(
             timeZone,
             castDateTimeToChar(DateTimeUtils.UTC_ZONE, operandExpression, fromType),
             toType,
             dateTimeFormatString(fromType));
   ```
   This way the change of signature on `castCharToDateTime(...)`can be avoided.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services