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/04/09 11:45:24 UTC
[GitHub] [druid] chenyuzhi459 opened a new pull request #11088: add round test
chenyuzhi459 opened a new pull request #11088:
URL: https://github.com/apache/druid/pull/11088
### Description
Add unit test for round function for sql.
##### Key changed/added classes in this PR
* `org.apache.druid.sql.calcite.CalciteQueryTest`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added integration tests.
- [x] 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.
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 pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11088:
URL: https://github.com/apache/druid/pull/11088#issuecomment-817094633
Ah, your test has found a bug :+1:
The native `round` function does not correctly handle null values:
```
[ERROR] testRoundFuc(org.apache.druid.sql.calcite.CalciteQueryTest) Time elapsed: 0.047 s <<< ERROR!
org.apache.druid.java.util.common.IAE: The first argument to the function[round] should be integer or double type but got the type: STRING
at org.apache.druid.math.expr.Function$Round.apply(Function.java:1318)
at org.apache.druid.math.expr.FunctionExpr.eval(FunctionalExpr.java:176)
at org.apache.druid.segment.virtual.ExpressionColumnValueSelector.getObject(ExpressionColumnValueSelector.java:76)
at org.apache.druid.segment.virtual.ExpressionColumnValueSelector.getObject(ExpressionColumnValueSelector.java:34)
at org.apache.druid.segment.virtual.ExpressionSelectors$1.getObject(ExpressionSelectors.java:107)
at org.apache.druid.query.scan.ScanQueryEngine$1$1.getColumnValue(ScanQueryEngine.java:241)
```
The error message is sort of funny, but what is happening here is that in non-vectorized expression processing, null values end up "typed" as STRING.
this is the check: https://github.com/apache/druid/blob/master/core/src/main/java/org/apache/druid/math/expr/Function.java#L1314
we need to modify this code to be something like
```
if (NullHandling.sqlCompatible() && value1.isNumericNull()) {
return ExprEval.of(null);
}
if (value1.type() != ExprType.LONG && value1.type() != ExprType.DOUBLE) {
throw new IAE(
"The first argument to the function[%s] should be integer or double type but got the type: %s",
name(),
value1.type()
);
}
...
```
--
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.
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 pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11088:
URL: https://github.com/apache/druid/pull/11088#issuecomment-818966475
skipping the rest of CI since it failed coverage again, integration tests passed on previous run and the latest change is trivial
--
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.
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] chenyuzhi459 commented on a change in pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11088:
URL: https://github.com/apache/druid/pull/11088#discussion_r611481236
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -407,12 +407,40 @@ public void testRoundWithExtremeNumbers()
assertExpr("round(CAST(minLong, 'DOUBLE') - 1, -2)", BigDecimal.valueOf(((double) Long.MIN_VALUE) - 1).setScale(-2, RoundingMode.HALF_UP).doubleValue());
}
+ @Test
+ public void testRoundWithNullValue()
+ {
+ Set<Pair<String, String>> invalidArguments = ImmutableSet.of(
+ Pair.of("null", "STRING"),
+ Pair.of("x", "STRING")
+ );
+ for (Pair<String, String> argAndType : invalidArguments) {
+ if (NullHandling.sqlCompatible()) {
+ assertExpr(String.format(Locale.ENGLISH, "round(%s)", argAndType.lhs), null);
Review comment:
Thanks for your tips .Actually, I think it can be more flexable to handle null value. If we handle it like:
if (NullHandling.sqlCompatible() && value1.isNumericNull()) {
return ExprEval.of(null);
}
if (value1.type() != ExprType.LONG && value1.type() != ExprType.DOUBLE) {
throw new IAE(
"The first argument to the function[%s] should be integer or double type but got the type: %s",
name(),
value1.type()
);
}
It is not compatible for usage `round(null, 1)` , which should assign a default value for null. I am trying to be compatible for usages like `round(null)` and `round(null, 1)`
--
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.
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] chenyuzhi459 commented on a change in pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11088:
URL: https://github.com/apache/druid/pull/11088#discussion_r611481236
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -407,12 +407,40 @@ public void testRoundWithExtremeNumbers()
assertExpr("round(CAST(minLong, 'DOUBLE') - 1, -2)", BigDecimal.valueOf(((double) Long.MIN_VALUE) - 1).setScale(-2, RoundingMode.HALF_UP).doubleValue());
}
+ @Test
+ public void testRoundWithNullValue()
+ {
+ Set<Pair<String, String>> invalidArguments = ImmutableSet.of(
+ Pair.of("null", "STRING"),
+ Pair.of("x", "STRING")
+ );
+ for (Pair<String, String> argAndType : invalidArguments) {
+ if (NullHandling.sqlCompatible()) {
+ assertExpr(String.format(Locale.ENGLISH, "round(%s)", argAndType.lhs), null);
Review comment:
Thanks for your tips .Actually, I think it can be more flexable to handle null value. If we handle it like:
if (NullHandling.sqlCompatible() && value1.isNumericNull()) {
return ExprEval.of(null);
}
if (value1.type() != ExprType.LONG && value1.type() != ExprType.DOUBLE) {
throw new IAE(
"The first argument to the function[%s] should be integer or double type but got the type: %s",
name(),
value1.type()
);
}
It is not compatible for usage `round(null, 1)` , which should assign a default value for null.
Thus, I am trying to be compatible for usages like `round(null)` and `round(null, 1)`
--
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.
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] chenyuzhi459 commented on a change in pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11088:
URL: https://github.com/apache/druid/pull/11088#discussion_r612152099
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -407,12 +407,40 @@ public void testRoundWithExtremeNumbers()
assertExpr("round(CAST(minLong, 'DOUBLE') - 1, -2)", BigDecimal.valueOf(((double) Long.MIN_VALUE) - 1).setScale(-2, RoundingMode.HALF_UP).doubleValue());
}
+ @Test
+ public void testRoundWithNullValue()
+ {
+ Set<Pair<String, String>> invalidArguments = ImmutableSet.of(
+ Pair.of("null", "STRING"),
+ Pair.of("x", "STRING")
+ );
+ for (Pair<String, String> argAndType : invalidArguments) {
+ if (NullHandling.sqlCompatible()) {
+ assertExpr(String.format(Locale.ENGLISH, "round(%s)", argAndType.lhs), null);
Review comment:
ok, I have changed it.
--
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.
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] chenyuzhi459 edited a comment on pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 edited a comment on pull request #11088:
URL: https://github.com/apache/druid/pull/11088#issuecomment-816786507
It seems something trouble broken in [Travis CI test](https://travis-ci.com/github/apache/druid/jobs/497366531) for SQL compatibility. How can I fix it ? @clintropolis
--
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.
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 merged pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #11088:
URL: https://github.com/apache/druid/pull/11088
--
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.
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] chenyuzhi459 commented on a change in pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11088:
URL: https://github.com/apache/druid/pull/11088#discussion_r611481236
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -407,12 +407,40 @@ public void testRoundWithExtremeNumbers()
assertExpr("round(CAST(minLong, 'DOUBLE') - 1, -2)", BigDecimal.valueOf(((double) Long.MIN_VALUE) - 1).setScale(-2, RoundingMode.HALF_UP).doubleValue());
}
+ @Test
+ public void testRoundWithNullValue()
+ {
+ Set<Pair<String, String>> invalidArguments = ImmutableSet.of(
+ Pair.of("null", "STRING"),
+ Pair.of("x", "STRING")
+ );
+ for (Pair<String, String> argAndType : invalidArguments) {
+ if (NullHandling.sqlCompatible()) {
+ assertExpr(String.format(Locale.ENGLISH, "round(%s)", argAndType.lhs), null);
Review comment:
Thanks for your tips .Actually, I think it can be more flexable to handle null value. If we handle it like:
if (NullHandling.sqlCompatible() && value1.isNumericNull()) {
return ExprEval.of(null);
}
if (value1.type() != ExprType.LONG && value1.type() != ExprType.DOUBLE) {
throw new IAE(
"The first argument to the function[%s] should be integer or double type but got the type: %s",
name(),
value1.type()
);
}
It is not compatible for usage `round(null, 1)` , which should assign a default value for null.
Thus, I am trying to be compatible for usages like `round(null)` and `round(null, 1)`
--
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.
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 #11088: add round test
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11088:
URL: https://github.com/apache/druid/pull/11088#discussion_r612130172
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -407,12 +407,40 @@ public void testRoundWithExtremeNumbers()
assertExpr("round(CAST(minLong, 'DOUBLE') - 1, -2)", BigDecimal.valueOf(((double) Long.MIN_VALUE) - 1).setScale(-2, RoundingMode.HALF_UP).doubleValue());
}
+ @Test
+ public void testRoundWithNullValue()
+ {
+ Set<Pair<String, String>> invalidArguments = ImmutableSet.of(
+ Pair.of("null", "STRING"),
+ Pair.of("x", "STRING")
+ );
+ for (Pair<String, String> argAndType : invalidArguments) {
+ if (NullHandling.sqlCompatible()) {
+ assertExpr(String.format(Locale.ENGLISH, "round(%s)", argAndType.lhs), null);
Review comment:
Oh, I meant we have a utility function that does this format with locale stuff, https://github.com/apache/druid/blob/master/core/src/main/java/org/apache/druid/java/util/common/StringUtils.java#L166 that could be used to make this a bit shorter, but since is a test it isn't a big deal whether or not you change to use it
--
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.
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 #11088: add round test
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11088:
URL: https://github.com/apache/druid/pull/11088#discussion_r611393741
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -407,12 +407,40 @@ public void testRoundWithExtremeNumbers()
assertExpr("round(CAST(minLong, 'DOUBLE') - 1, -2)", BigDecimal.valueOf(((double) Long.MIN_VALUE) - 1).setScale(-2, RoundingMode.HALF_UP).doubleValue());
}
+ @Test
+ public void testRoundWithNullValue()
+ {
+ Set<Pair<String, String>> invalidArguments = ImmutableSet.of(
+ Pair.of("null", "STRING"),
+ Pair.of("x", "STRING")
+ );
+ for (Pair<String, String> argAndType : invalidArguments) {
+ if (NullHandling.sqlCompatible()) {
+ assertExpr(String.format(Locale.ENGLISH, "round(%s)", argAndType.lhs), null);
Review comment:
nit: could use `StringUtils.format(...)`
--
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.
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] suneet-s commented on pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #11088:
URL: https://github.com/apache/druid/pull/11088#issuecomment-817879490
> changes lgtm, I don't think this is going to pass the coverage check in default mode because the added code is only hit in SQL compatible null handling mode, so I think we can ignore it.
I triggered the tests in phase 2 manually, since the tests in the null compatibility mode show that we have sufficient coverage.
--
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.
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] chenyuzhi459 commented on a change in pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11088:
URL: https://github.com/apache/druid/pull/11088#discussion_r611532181
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -407,12 +407,40 @@ public void testRoundWithExtremeNumbers()
assertExpr("round(CAST(minLong, 'DOUBLE') - 1, -2)", BigDecimal.valueOf(((double) Long.MIN_VALUE) - 1).setScale(-2, RoundingMode.HALF_UP).doubleValue());
}
+ @Test
+ public void testRoundWithNullValue()
+ {
+ Set<Pair<String, String>> invalidArguments = ImmutableSet.of(
+ Pair.of("null", "STRING"),
+ Pair.of("x", "STRING")
+ );
+ for (Pair<String, String> argAndType : invalidArguments) {
+ if (NullHandling.sqlCompatible()) {
+ assertExpr(String.format(Locale.ENGLISH, "round(%s)", argAndType.lhs), null);
Review comment:
Sorry, I don't understand what you mean. Could you explain more details?
--
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.
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] chenyuzhi459 commented on pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on pull request #11088:
URL: https://github.com/apache/druid/pull/11088#issuecomment-816786507
It seems something trouble broken in [Travis CI test](https://travis-ci.com/github/apache/druid/jobs/497366531) for SQL compality. How can I fix it
--
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.
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] chenyuzhi459 edited a comment on pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 edited a comment on pull request #11088:
URL: https://github.com/apache/druid/pull/11088#issuecomment-816786507
It seems something trouble broken in [Travis CI test](https://travis-ci.com/github/apache/druid/jobs/497366531) for SQL compatibility. How can I fix it
--
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.
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] chenyuzhi459 commented on a change in pull request #11088: add round test
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11088:
URL: https://github.com/apache/druid/pull/11088#discussion_r611532181
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -407,12 +407,40 @@ public void testRoundWithExtremeNumbers()
assertExpr("round(CAST(minLong, 'DOUBLE') - 1, -2)", BigDecimal.valueOf(((double) Long.MIN_VALUE) - 1).setScale(-2, RoundingMode.HALF_UP).doubleValue());
}
+ @Test
+ public void testRoundWithNullValue()
+ {
+ Set<Pair<String, String>> invalidArguments = ImmutableSet.of(
+ Pair.of("null", "STRING"),
+ Pair.of("x", "STRING")
+ );
+ for (Pair<String, String> argAndType : invalidArguments) {
+ if (NullHandling.sqlCompatible()) {
+ assertExpr(String.format(Locale.ENGLISH, "round(%s)", argAndType.lhs), null);
Review comment:
Sorry, I don't understand what you mean. Could you explain it for more details?
--
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.
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