clintropolis commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r765680034
##########
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:
>This code path is only called during planning (AFAIK) so there isn't
much performance advantage. So the redundancy doesn't cause any real problem
anyway.
Ah, I wasn't really thinking on the performance angle (though planning does
a lot of duplicate work), more on consistency with the way the rest of Druid
interacts with literals - like we aren't using `getValueAs` anywhere else, so
mostly wondering why we should use it here when it isn't really that useful to
what we are doing. Using `RexLiteral.stringValue(literal)` and `((Number)
RexLiteral.value(literal)` would probably be most consistent I think?
--
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]
