You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/12/08 22:25:44 UTC
[GitHub] [druid] LakshSingla opened a new pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
LakshSingla opened a new pull request #11923:
URL: https://github.com/apache/druid/pull/11923
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
Bug: `SELECT floor(123)` returns `1230`
(In comparison,
`SELECT 123` returns `123`,
`SELECT floor(123) from druid.foo` returns `123`
)
Cause: `DruidLogicalValuesRule` while transforming to `DruidRel` can return incorrect values, if during the creation of the literal it was created from a float value. The `BigDecimal` representation stores `123.0`, and it seems that using `RexLiteral's` method while conversion returns the __inflated__ value (which is 1230). I am unsure if this is intentional from Calcite's perspective, and the actual change should be done somewhere else.
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
Extract the values of INT/LONG from the RexLiteral in the `DruidLogicalValuesRule`, via `BigDecimal.longValue()` method.
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
<hr>
##### Key changed/added classes in this PR
* Minor update in `DruidLogicalValuesRule#getValueFromLiteral`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r765549260
##########
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 makes more assumptions about how that code is written inside the library. Calling `literal.getValueAs(BigDecimal.class)` makes the intention explicit that `BigDecimal` is the type we want. Current ways seem future-proof to unexpected changes in the calcite library.
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by 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
[GitHub] [druid] LakshSingla commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766513930
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
Good catch, I checked and as per your comment this is an edge case that changes with this `if` condition.
More specifically, iff `literal.getType.getSqlTypeName() == BOOELAN`, `literal.getSqlTypeName() =/= BOOLEAN` and `literal.value == null`, then we would exit early from the code and return null, but the older method should be returning `false`.
--
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
[GitHub] [druid] jihoonson closed pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
jihoonson closed pull request #11923:
URL: https://github.com/apache/druid/pull/11923
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
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: 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
[GitHub] [druid] abhishekagarwal87 merged pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged pull request #11923:
URL: https://github.com/apache/druid/pull/11923
--
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
[GitHub] [druid] LakshSingla commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766533412
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
We do need the null check since that is getting activated in the test cases. Maybe it can be put in the respective switch case?
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r765561312
##########
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 mean we sort of already have to know how the library works to call `literal.getValueAs(BigDecimal.class)` to get a type we don't want to turn it into one we do so it doesn't really seem any different to me. This is the only place we are using `getValueAs` in our code, most other callers use `RexLiteral.value(...)` which is equivalent to `getValue` for `RexLiteral` but also allows `CAST` and unary negation functions to get literal values. Most of those callers appear to be casting the value to `Number` (see `Expressions.literalToDruidExpression`)
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by 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 seem like always `NlsString`, numeric types will always be `BigDecimal`, so we can just convert to float, double, or long as appropriate.
```
...
case VARCHAR:
return ((NlsString) literal.getValue()).getValue();
case FLOAT:
return ((BigDecimal) literal.getValue()).floatValue();
case DOUBLE:
case REAL:
case DECIMAL:
return ((BigDecimal) literal.getValue()).doubleValue();
case TINYINT:
case SMALLINT:
case INTEGER:
case BIGINT:
return ((BigDecimal) literal.getValue()).longValue();
...
```
--
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
[GitHub] [druid] jihoonson commented on pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #11923:
URL: https://github.com/apache/druid/pull/11923#issuecomment-989277513
Closing and reopening as Travis job is in a strange state that shows running forever.
--
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
[GitHub] [druid] LakshSingla commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766517562
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
@abhishekagarwal87 shared a snippet in Calcite's library: [RexUtil](https://github.com/apache/calcite/blob/82dd78a14f6aef2eeec2f9c94978d04b4acc5359/core/src/main/java/org/apache/calcite/sql/SqlUtil.java#L177) which states that it is intended for it to be null.
--
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
[GitHub] [druid] LakshSingla commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766517562
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
@abhishekagarwal87 shared a snippet in Calcite's library: [RexUtil](https://github.com/apache/calcite/blob/82dd78a14f6aef2eeec2f9c94978d04b4acc5359/core/src/main/java/org/apache/calcite/sql/SqlUtil.java#L177) which states that it is intended for it to be ~~null~~ false.
--
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
[GitHub] [druid] LakshSingla commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766517562
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
@abhishekagarwal87 shared a snippet in Calcite's library: [RexUtil](https://github.com/apache/calcite/blob/82dd78a14f6aef2eeec2f9c94978d04b4acc5359/core/src/main/java/org/apache/calcite/sql/SqlUtil.java#L177) which states that it is intended for it to be false.
--
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
[GitHub] [druid] LakshSingla closed pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla closed pull request #11923:
URL: https://github.com/apache/druid/pull/11923
--
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
[GitHub] [druid] LakshSingla commented on pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11923:
URL: https://github.com/apache/druid/pull/11923#issuecomment-971429251
Closed since the actual cause of the issue can be different. Need some investigation in this.
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r765649541
##########
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 mean we sort of already have to know how the library works to call literal.getValueAs(BigDecimal.class)
That's true. Though we can still try to minimize relying on the internal details. 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.
--
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
[GitHub] [druid] jihoonson commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r765369593
##########
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:
Per Julian's suggestion in the linked thread, this case statement should be simply `return literal.getValueAs(BigDecimal.class).longValue();`
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by 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.
```
...
case VARCHAR:
return ((NlsString) literal.getValue()).getValue();
case FLOAT:
return ((BigDecimal) literal.getValue()).floatValue();
case DOUBLE:
case REAL:
case DECIMAL:
return ((BigDecimal) literal.getValue()).doubleValue();
case TINYINT:
case SMALLINT:
case INTEGER:
case BIGINT:
return ((BigDecimal) literal.getValue()).longValue();
...
```
--
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
[GitHub] [druid] LakshSingla commented on pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11923:
URL: https://github.com/apache/druid/pull/11923#issuecomment-975482658
Reopening with the following comment:
The type conversion happens because Floor in `Functions` always returns a double expression as a result. Therefore we have a BigDecimal(123.0) wrapped in an INTEGER note. When trying to extract the value, Calcite extracts it using it as `unscaled` value.
`SELECT FLOOR(123) FROM druid.foo` returns the correct result, because the actual conversion is delegated to the native layer (without trying to extract a Long value via Calcite's methods).
Another fix that we can do is remove this that Floor returns a type of the result which is passed to it, i.e. FLOOR(INT) should return INT and FLOOR(DOUBLE) should return DOUBLE, but I am unsure if that is the correct approach for this issue.
--
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
[GitHub] [druid] LakshSingla commented on pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11923:
URL: https://github.com/apache/druid/pull/11923#issuecomment-992130236
I have updated the PR with `null` handling only in the cases when required. I agree with @abhishekagarwal87 comment that we shouldn't perform many type conversions if we can help it, because it might conflict with Calcite's behavior. The original code, does circumvent this without any explicit null check, but only because we are defaulting to original code in case something wrong happens.
```
Object maybeBigDecimalImpl = literal.getValue();
if (maybeBigDecimalImpl instanceof BigDecimal) {
return ((BigDecimal) maybeBigDecimalImpl).longValue();
}
return literal.getValueAs(Long.class);
```
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766532567
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
http://sqlfiddle.com/#!9/7fff3f/35
looks like it is another place to use `useStrictBoolean` flag? :) But my larger point was around the internal calcite quirks that we might not be aware of. I am concerned that we will accidentally change the behaviour of existing queries as happened here. I will suggest keeping the scope of this PR to just fixing the bug.
--
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
[GitHub] [druid] LakshSingla commented on pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11923:
URL: https://github.com/apache/druid/pull/11923#issuecomment-987574208
Some analysis into the approaches that can be used to fix this:
1. Cast the output type of the node with the output type of the function, i.e. floor(int) should return a double.
PROS:
(None that I can distinctly think of over the others)
CONS:
Could introduce many new corner cases.
I am of the idea that Calcite already casts the nodes appropriately, i.e. strlen("abc") is of type int and not string (not verified this). Therefore there is no issue with how the literal conversion is happening in DruidRexExecutor
Is not in line with other SQL dialects as well.
2. Functions or ExprEval can accept an input type, and try to cast the results to that type.
PROS:
Might make future errors less likely
We would be able to cast the values in the corresponding function/expr eval itself rather than relying on the DruidRexExecutor to do the same in future.
CONS:
Huge amount of code change
I am not sure if this is the correct way to go about it, since other rules are working fine without this additional complexity.
3. Improve the extraction in the DruidLogicalValuesRule (The change that is introduced in this PR)
PROS:
Localized code change, would be able to identify the cause of the regressions if this causes.
Since this is the only rule that is giving an error, I think this is where we should solve it.
CONS:
Can be dependent on the implementation of the Numeric type of the RexLiteral. (See the top level doc [here](https://calcite.apache.org/javadocAggregate/org/apache/calcite/rex/RexLiteral.html) ).
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by 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.
```
...
case VARCHAR:
return literal.getValue();
case FLOAT:
return ((BigDecimal) literal.getValue()).floatValue();
case DOUBLE:
case REAL:
case DECIMAL:
return ((BigDecimal) literal.getValue()).doubleValue();
case TINYINT:
case SMALLINT:
case INTEGER:
case BIGINT:
return ((BigDecimal) literal.getValue()).longValue();
...
```
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766485610
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
BTW I looked at internal calcite code. If literal.getType().getSqlTypeName() is `Boolean` and literal.getSqlTypeName() is not `Boolean` then this method was earlier returning 0 but now will return null.
--
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
[GitHub] [druid] LakshSingla commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766517562
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
Abhishek shared a snippet in Calcite's library: [RexUtil](https://github.com/apache/calcite/blob/82dd78a14f6aef2eeec2f9c94978d04b4acc5359/core/src/main/java/org/apache/calcite/sql/SqlUtil.java#L177) which states that it is intended for it to be null.
--
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
[GitHub] [druid] jihoonson commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
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: 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
[GitHub] [druid] clintropolis commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766514916
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
er, `CAST(null as boolean)` should be null 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: 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
[GitHub] [druid] abhishekagarwal87 commented on pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #11923:
URL: https://github.com/apache/druid/pull/11923#issuecomment-994300347
Merged since CI failure is unrelated. Thank you @LakshSingla
--
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
[GitHub] [druid] jihoonson commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r765326343
##########
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:
BTW, I was not sure if this is a bug or not, so posted a question in the calcite dev (https://lists.apache.org/thread/yr26vot133q9p7kjjh8nlq6pb1mjx9vl).
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766532567
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
http://sqlfiddle.com/#!9/7fff3f/35
Mysql also returns null.
looks like it is another place to use `useStrictBoolean` flag? :) But my larger point was around the internal calcite quirks that we might not be aware of. I am concerned that we will accidentally change the behaviour of existing queries as happened here. I will suggest keeping the scope of this PR to just fixing the bug.
--
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
[GitHub] [druid] LakshSingla commented on a change in pull request #11923: Fix incorrect type conversion in DruidLogicalValueRule
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11923:
URL: https://github.com/apache/druid/pull/11923#discussion_r766517562
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -93,24 +94,28 @@ public void onMatch(RelOptRuleCall call)
*
* @throws IllegalArgumentException for unsupported types
*/
+ @Nullable
@VisibleForTesting
static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerContext)
{
+ if (RexLiteral.value(literal) == null) {
Review comment:
@abhishekagarwal87 shared a snippet in Calcite's library: [RexUtil](https://github.com/apache/calcite/blob/82dd78a14f6aef2eeec2f9c94978d04b4acc5359/core/src/main/java/org/apache/calcite/sql/SqlUtil.java#L177) which seems to state that it is intended for it to be ~~null~~ false. (I guess it should be applicable here as well).
--
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