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 2020/12/30 05:36:10 UTC
[GitHub] [druid] qinzhen353 opened a new pull request #10721: javascript filter result convert to java boolean
qinzhen353 opened a new pull request #10721:
URL: https://github.com/apache/druid/pull/10721
Fixes #10719.
### Description
When javascript filter direct return js function result(like `return [1, 2, 3].includes(4);`), it seems the filter not take effect，but change it to `return [1, 2, 3].includes(4) == true;`, it result is correct. I found the js engine(rhino)'s return type is `org.mozilla.javascript.NativeBoolean` when use the former function, and `Context.toBoolean` will treat it as true even though it is false!
So I add a convert js function, convert `org.mozilla.javascript.NativeBoolean` to `java.lang.Boolean`, so that `Context.toBoolean` will return the correct result.
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] 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.
----------------------------------------------------------------
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] jihoonson merged pull request #10721: javascript filter result convert to java boolean
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #10721:
URL: https://github.com/apache/druid/pull/10721
----------------------------------------------------------------
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] FrankChen021 commented on a change in pull request #10721: javascript filter result convert to java boolean
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10721:
URL: https://github.com/apache/druid/pull/10721#discussion_r550377970
##########
File path: processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java
##########
@@ -327,7 +335,8 @@ public boolean applyInContext(Context cx, Object input)
if (extractionFn != null) {
input = extractionFn.apply(input);
}
- return Context.toBoolean(fnApply.call(cx, scope, scope, new Object[]{input}));
+ Object fnResult = fnApply.call(cx, scope, scope, new Object[]{input});
+ return Context.toBoolean(boolConvApply.call(cx, scope, scope, new Object[] {fnResult}));
Review comment:
Instead of evaluating another script to get the final result, how about get value of `java.lang.Boolean` on `NativeBoolean` object directly like this ?
```
(Boolean)((ScriptableObject) fnResult).getDefaultValue(ScriptRuntime.BooleanClass)
```
----------------------------------------------------------------
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] jihoonson merged pull request #10721: javascript filter result convert to java boolean
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #10721:
URL: https://github.com/apache/druid/pull/10721
----------------------------------------------------------------
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] qinzhen353 commented on a change in pull request #10721: javascript filter result convert to java boolean
Posted by GitBox <gi...@apache.org>.
qinzhen353 commented on a change in pull request #10721:
URL: https://github.com/apache/druid/pull/10721#discussion_r550385717
##########
File path: processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java
##########
@@ -327,7 +335,8 @@ public boolean applyInContext(Context cx, Object input)
if (extractionFn != null) {
input = extractionFn.apply(input);
}
- return Context.toBoolean(fnApply.call(cx, scope, scope, new Object[]{input}));
+ Object fnResult = fnApply.call(cx, scope, scope, new Object[]{input});
+ return Context.toBoolean(boolConvApply.call(cx, scope, scope, new Object[] {fnResult}));
Review comment:
Thanks, it works. I change to use this way, and add more unit test for other return 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