jihoonson commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766078527
##########
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:
I used `getValueAs()` before because it seemed easier than implementing
casts in this method. I don't worry about consistency much but, if we decide to
refactor this method, I agree that it is important to have the least dependency
on Calcite's internal logic. In this sense, using `RexLiteral.value()` and
`RexLiteral.stringValue()` seems the best as Clint suggested since it returns
the raw `value`. However, refactoring doesn't seem to have a big impact at
least for now. So, I don't have a strong opinion here and am OK with just
fixing the bug without refactoring.
--
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]
