[GitHub] [druid] clintropolis commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule

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

clintropolis commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r765465153



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -110,6 +111,13 @@ static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerCont
       case SMALLINT:
       case INTEGER:
       case BIGINT:
+        // Safegaurd in case the internal implementaion of the RexLiteral for numbers change
+        Object maybeBigDecimalImpl = literal.getValue();
+        if (maybeBigDecimalImpl instanceof BigDecimal) {
+          return ((BigDecimal) maybeBigDecimalImpl).longValue();
+        }
+        // This might return incorrect value if the literal was created from float.
+        // For example, representation of the form 123.0 can return 1230

Review comment:
       given that all of our calls to `getValueAs` are already within a case statement on the literal type, it seems redundant to me to use `getValueAs`, which is going to do another case statement on typeName instead of type.getTypeName.
   
   Maybe we should just do `literal.getValue` instead of `literal.getValueAs`. Strings I think we can just pass through directly, numeric types will always be `BigDecimal`, so we can just convert to float, double, or long as appropriate.




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org