EQL: Add string function #54470
EQL: Add string function #54470
Conversation
|
Pinging @elastic/es-ql (:Query Languages/EQL) |
| } | ||
|
|
||
| public static Object doProcess(Object source) { | ||
| return source == null ? "null" : source.toString(); |
There was a problem hiding this comment.
Why not null the instance instead of "null" the string?
There was a problem hiding this comment.
per #54465 and https://github.com/endgameinc/eql/blob/master/eql/functions.py#L626 I was following a contract (we can change it, of course) that this function always returns a string. So I catch null directly, since I can't call a method on it. Here's the current behavior for how it folds string(null):
>>> import eql
>>> eql.parse_expression("string(null)")
String(value='None')There was a problem hiding this comment.
Since we're not using some concept of Optional, returning null instead of a string "null" is better since otherwise there's no way to differentiate between a string with "null" chars vs actual null since they would both be equivalent which is not what we want.
...org/elasticsearch/xpack/eql/expression/function/scalar/whitelist/InternalEqlScriptUtils.java
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/xpack/eql/expression/function/scalar/string/ToStringFunctionPipe.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/eql/src/main/resources/org/elasticsearch/xpack/eql/plugin/eql_whitelist.txt
Show resolved
Hide resolved
costin
left a comment
costin
left a comment
There was a problem hiding this comment.
Left some comments - LGTM otherwise
| } | ||
|
|
||
| public static Object doProcess(Object source) { | ||
| return source == null ? "null" : source.toString(); |
There was a problem hiding this comment.
Since we're not using some concept of Optional, returning null instead of a string "null" is better since otherwise there's no way to differentiate between a string with "null" chars vs actual null since they would both be equivalent which is not what we want.
| */ | ||
| public class ToString extends ScalarFunction { | ||
|
|
||
| private final Expression source; |
There was a problem hiding this comment.
Please rename source to something else (delegate, target, value?) since it conflicts with Source source convention. While source was used in a couple of Processors, it was mainly to follow the function delegation and their official param names.
This applies to the whole PR.
| note = "check that string(null) returns 'null'" | ||
| expected_event_ids = [1, 2] | ||
| query = 'process where opcode == 3 and string(ppid) == "null"' | ||
|
|
There was a problem hiding this comment.
The original idea was to keep this file unchanged from the original eql implementation
https://github.com/endgameinc/eql/blob/master/eql/etc/test_queries.toml
so we can sync up easier with the reference repo/impl
and add more queries into separate file(s), could be just one, could be individual files for specific extensions
started doing something like this in my other PR
https://github.com/elastic/elasticsearch/pull/54277/files/be0b67cdf5022f38ec3610a68bbb91af8b10b8b3#diff-00cfec987b028a9cc67295e57c02cf68R1
aleksmaus
left a comment
aleksmaus
left a comment
There was a problem hiding this comment.
Left one comment, open for discussion, also agree on not returning "null" as string.
Other than that LGTM.
* EQL: Add string() function * EQL: Reorder queryfolder_tests * EQL: Add test queries * EQL: Fix InternalEqlScriptUtils.string and test case * EQL: Fix testStringFunctionWithText error message * EQL: Flatten ToStringFunctionPipe.equals * EQL: Reorder painless whitelist * EQL: Address feedback and remove string(null) handling * EQL: Move string(pid) test over * EQL: Rename source -> value
|
7.x backport 96a903b |
5 participants
Resolves #54465