EQL: Add concat function#55193
EQL: Add concat function#55193rw-access merged 11 commits intoelastic:masterfrom rw-access:eql/concat-function
Conversation
|
Pinging @elastic/es-ql (:Query Languages/EQL) |
astefan
left a comment
There was a problem hiding this comment.
Looks good in general, but I would like to see more tests, please.
| TypeResolution resolution = TypeResolution.TYPE_RESOLVED; | ||
| int index = 0; | ||
| for (Expression value : values) { | ||
| resolution = isExact(value, sourceText(), ParamOrdinal.fromIndex(index)); |
There was a problem hiding this comment.
Why only isExact? Shouldn't this be a string only type of value?
There was a problem hiding this comment.
Also, this loop goes through all of concat's values and returns the resolution of the last element in list. Shouldn't, maybe, stop at the first resolution that is .unresolved() and return that?
There was a problem hiding this comment.
also, this accepts non string parameters as well, and calls .toString() on them
the example uses pid which is a long
https://eql.readthedocs.io/en/latest/query-guide/functions.html#concat
...n/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/ConcatFunctionPipe.java
Outdated
Show resolved
Hide resolved
...n/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/ConcatFunctionPipe.java
Outdated
Show resolved
Hide resolved
| file where between(file_path, "dev", ".json", true) == "\\TestLogs\\something" | ||
| ''' | ||
|
|
||
| [[queries]] |
There was a problem hiding this comment.
Please, add more integration tests here:
- with one pattern only
- with an empty string pattern among other patterns
- with
nullpatterns
...n/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/ConcatFunctionPipe.java
Outdated
Show resolved
Hide resolved
...n/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/ConcatFunctionPipe.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: Andrei Stefan <astefan@users.noreply.github.com>
| file where between(file_path, "dev", ".json", true) == "\\TestLogs\\something" | ||
| ''' | ||
|
|
||
| [[queries]] |
astefan
left a comment
There was a problem hiding this comment.
LGTM. Left one more comment regarding the error message that gets logged when the fields are not of the required type.
| VerificationException e = expectThrows(VerificationException.class, | ||
| () -> plan("process where concat(plain_text)")); | ||
| String msg = e.getMessage(); | ||
| assertEquals("Found 1 problem\nline 1:15: [concat(plain_text)] cannot operate on first argument field of data type " |
There was a problem hiding this comment.
Small improvement here: if it's possible for a one argument call to not use "first" in the error message, that would look more user friendly.
Also, I would like to see one or two tests where another argument different from the first doesn't fulfill the restriction.
* EQL: Add concat function * EQL: for loop spacing for concat * EQL: return unresolved arguments to concat early * EQL: Add concat integration tests * EQL: Fix concat query fail test * EQL: Add class for concat function testing * EQL: Add concat integration tests * EQL: Update concat() null behavior
|
7.x backport 3890820 |
Closes #55185
Adds variable argument
concatfunction. This function will never return null, and only skips over null input parameters.In #55185, I raise three different options for null handling. Currently, I'm choosing the first.
But I'm wondering if the second option (return null if any input is null) is the most appropriate and consistent with null handling. Having a coalesce function to make the user manually handle nulls seems like a better choice than just ignoring null inputs.