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/11/21 21:32:04 UTC
[GitHub] [druid] LakshSingla opened a new pull request #11968: Improve the DruidRexExecutor w.r.t handling of numeric arrays
LakshSingla opened a new pull request #11968:
URL: https://github.com/apache/druid/pull/11968
### Description
Issue:
`DruidRexExecutor` while reducing Arrays, specially numeric arrays, doesn't convert the value from ExprResult's type to BigDecimal, which causes `makeLiteral` to cast the values. Also, if NaN or Infinite values are present in the array, the error is a generic `NumberFormatException`. For example:
`SELECT ARRAY[1.11, 2.22]` returns [1, 2]
`SELECT SQRT(-1)` throws a generic `NumberFormatException` instead of `IAE`
This PR introduces change to cast the numeric values to `BigDecimal` since Calcite's library understands that easily, and doesn't perform casts.
<hr>
##### Key changed/added classes in this PR
* `DruidRexExecutor#reduce()`
<hr>
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.
- [ ] 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] LakshSingla commented on pull request #11968: Improve the DruidRexExecutor w.r.t handling of numeric arrays
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11968:
URL: https://github.com/apache/druid/pull/11968#issuecomment-975886268
Added the testcases, and fixed the older incorrect ones which were known.
--
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 #11968: Improve the DruidRexExecutor w.r.t handling of numeric arrays
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11968:
URL: https://github.com/apache/druid/pull/11968#discussion_r754830577
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java
##########
@@ -145,7 +146,31 @@ public void reduce(
}
} else if (sqlTypeName == SqlTypeName.ARRAY) {
assert exprResult.isArray();
- literal = rexBuilder.makeLiteral(Arrays.asList(exprResult.asArray()), constExp.getType(), true);
+ if (SqlTypeName.NUMERIC_TYPES.contains(constExp.getType().getComponentType().getSqlTypeName())) {
+ if (exprResult.type().getElementType().is(ExprType.LONG)) {
+ List<BigDecimal> resultAsBigDecimalList = Arrays.stream(exprResult.asLongArray())
+ .map(BigDecimal::valueOf)
+ .collect(Collectors.toList());
+ literal = rexBuilder.makeLiteral(resultAsBigDecimalList, constExp.getType(), true);
+ } else {
Review comment:
An else case is required to stop the "Variable might not have been initialized" exception from cropping up, and I didn't want to provide a failsafe else block. In case when it's not an array (https://github.com/LakshSingla/druid/blob/4dcdb2d68563302a9b4d387cdbb220ec4f3a59b3/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java#L130), it was using the `else` case as well, that's why I omitted the `else if`.
One approach can be to initialize the `literal` to null and then including your suggestion. Let me know if that seems better, and I can add that in a separate commit.
--
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 #11968: Improve the DruidRexExecutor w.r.t handling of numeric arrays
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged pull request #11968:
URL: https://github.com/apache/druid/pull/11968
--
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] rohangarg commented on a change in pull request #11968: Improve the DruidRexExecutor w.r.t handling of numeric arrays
Posted by GitBox <gi...@apache.org>.
rohangarg commented on a change in pull request #11968:
URL: https://github.com/apache/druid/pull/11968#discussion_r754826168
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java
##########
@@ -145,7 +146,31 @@ public void reduce(
}
} else if (sqlTypeName == SqlTypeName.ARRAY) {
assert exprResult.isArray();
- literal = rexBuilder.makeLiteral(Arrays.asList(exprResult.asArray()), constExp.getType(), true);
+ if (SqlTypeName.NUMERIC_TYPES.contains(constExp.getType().getComponentType().getSqlTypeName())) {
+ if (exprResult.type().getElementType().is(ExprType.LONG)) {
+ List<BigDecimal> resultAsBigDecimalList = Arrays.stream(exprResult.asLongArray())
+ .map(BigDecimal::valueOf)
+ .collect(Collectors.toList());
+ literal = rexBuilder.makeLiteral(resultAsBigDecimalList, constExp.getType(), true);
+ } else {
Review comment:
would be better if you can add a check for double type here
--
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