-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SQL: Improve painless script generated from IN
#35055
Conversation
Pinging @elastic/es-search-aggs |
Just a note that |
Replace standard `||` and `==` painless operators with `or` and `eq` null safe alternatives from `InternalSqlScriptUtils`. Follow up to elastic#34750
5a076d4
to
a0cd723
Compare
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor comments. Looks good!
@@ -113,6 +115,10 @@ public static Boolean notNull(Object expression) { | |||
return IsNotNullProcessor.apply(expression); | |||
} | |||
|
|||
public static Boolean in(Object value, List<Object> values) { | |||
return In.doFold(value, values); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming convention is to call the method on the processor (typically InProcessor.apply
)
Boolean result = false; | ||
for (Expression rightValue : list) { | ||
Boolean compResult = Comparisons.eq(foldedLeftValue, rightValue.fold()); | ||
for (Object v : values) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this method to the processor please.
for (Expression rightValue : list) { | ||
Boolean compResult = Comparisons.eq(foldedLeftValue, rightValue.fold()); | ||
for (Object v : values) { | ||
Boolean compResult = Comparisons.eq(value, v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be explicit about boxing. Boolean result = Boolean.FALSE
. if (compResult == Boolean.TRUE) or
compResult.boolean()` ...
|
||
return new ScriptTemplate(format(Locale.ROOT, "%s", sj.toString()), paramsBuilder.build(), dataType()); | ||
// remove duplicates | ||
List<Object> values = new ArrayList<>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new LinkedHashSet(Foldables.valuesOf(list, dataType())
This has the side effect of triggering a conversion if needed (which should expose potential bugs).
assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(" + | ||
"InternalSqlScriptUtils.power(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1), params.v2))", | ||
sc.script().toString()); | ||
assertEquals("[{v=int}, {v=2}, {v=[10, null, 20]}]", sc.script().params().toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@costin Thanks a lot for the review and all your help on this PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
IN
IN
Replace standard `||` and `==` painless operators with new `in` method introduced in `InternalSqlScriptUtils`. This allows the list of values to become a script variable which is replaced each time with the list of values provided by the user. Move In to the same package as InPipe & InProcessor Follow up to #34750 Co-authored-by: Costin Leau <costin.leau@gmail.com>
Backported to |
Replace standard `||` and `==` painless operators with new `in` method introduced in `InternalSqlScriptUtils`. This allows the list of values to become a script variable which is replaced each time with the list of values provided by the user. Move In to the same package as InPipe & InProcessor Follow up to #34750 Co-authored-by: Costin Leau <costin.leau@gmail.com>
Backported to |
Replace standard
||
and==
painless operators withor
andeq
null safe alternatives fromInternalSqlScriptUtils
.Follow up to #34750