Skip to content
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.

Sign up for GitHub

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

Jump to bottom

EQL: Add concat function #55193

Merged
merged 11 commits into from
May 5, 2020
Merged

EQL: Add concat function #55193

merged 11 commits into from
May 5, 2020

Conversation

rw-access
Copy link
Contributor

@rw-access rw-access commented Apr 15, 2020 •
edited
Loading

Closes #55185

Adds variable argument concat function. 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.

  1. never return null
  2. return null if any inputs are null
  3. return null if all inputs are null

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.

@rw-access rw-access added the :Analytics/EQL EQL querying label Apr 15, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (:Query Languages/EQL)

astefan
astefan reviewed Apr 15, 2020
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general, but I would like to see more tests, please.

.../eql/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/Concat.java Outdated
TypeResolution resolution = TypeResolution.TYPE_RESOLVED;
int index = 0;
for (Expression value : values) {
resolution = isExact(value, sourceText(), ParamOrdinal.fromIndex(index));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only isExact? Shouldn't this be a string only type of value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 9295e79

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
x-pack/plugin/eql/qa/common/src/main/resources/test_queries_supported.toml
@@ -14,6 +14,16 @@ query = '''
file where between(file_path, "dev", ".json", true) == "\\TestLogs\\something"
'''

[[queries]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add more integration tests here:

  • with one pattern only
  • with an empty string pattern among other patterns
  • with null patterns

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With all nulls as well

...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
@costin costin requested a review from matriv April 16, 2020 11:18
matriv
matriv reviewed Apr 16, 2020
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good overall, I agree with @astefan about more tests with nulls and empty strings.

  • Should ConcatFunctionPipe also have tests?
  • Also missing the context maybe, shouldn't it operate on text too (inexact) ?

x-pack/plugin/eql/qa/common/src/main/resources/test_queries_supported.toml
@@ -14,6 +14,16 @@ query = '''
file where between(file_path, "dev", ".json", true) == "\\TestLogs\\something"
'''

[[queries]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With all nulls as well

astefan
astefan approved these changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left one more comment regarding the error message that gets logged when the fields are not of the required type.

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/planner/QueryFolderFailTests.java Outdated
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 "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@astefan astefan requested a review from matriv April 22, 2020 16:56
matriv
matriv approved these changes May 4, 2020
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rjernst rjernst added the Team:QL (Deprecated) Meta label for query languages team label May 4, 2020
@rw-access rw-access merged commit a9828ab into elastic:master May 5, 2020
@rw-access rw-access deleted the eql/concat-function branch May 5, 2020 18:52
rw-access added a commit that referenced this pull request May 5, 2020
@rw-access
EQL: Add concat function (#55193)
3890820 
* 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
@rw-access
Copy link
Contributor Author

rw-access commented May 5, 2020 •
edited
Loading

7.x backport 3890820

@jrodewig jrodewig mentioned this pull request May 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astefan astefan astefan approved these changes

@matriv matriv matriv approved these changes

@costin costin Awaiting requested review from costin

@aleksmaus aleksmaus Awaiting requested review from aleksmaus

Assignees
No one assigned
Labels
:Analytics/EQL EQL querying Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EQL: implement concat function
5 participants
@rw-access @elasticmachine @astefan @matriv @rjernst