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.

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

EQL: Update between() case handling and tests #59738

Closed
wants to merge 5 commits into from
Closed

EQL: Update between() case handling and tests #59738

wants to merge 5 commits into from

Conversation

rw-access
Copy link
Contributor

Closes #56749

  • Made the between() function configuration aware and remove the deprecated case_sensitive argument.
  • Updated the null handling to pass tests and match this specification
    • if greedy is unspecified, then it is set to false (unchanged)
    • if any of the input arguments (source, left, right, greedy) are null, then the function will return null
    • if left is not in the string source or right is not in the string source, then null is returned
  • Updated the handling for empty strings to match the specs/test
    • if right is an empty string and greedy is true, then the rest of the string after left is consumed

@rw-access rw-access added >enhancement :Analytics/EQL EQL querying Team:QL (Deprecated) Meta label for query languages team labels Jul 16, 2020
@rw-access rw-access requested review from costin, astefan and matriv July 16, 2020 22:52
@elasticmachine
Copy link
Collaborator

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

@rw-access rw-access changed the title EQL: Update between case handling and tests EQL: Update between() case handling and tests Jul 16, 2020
@costin costin removed their request for review July 17, 2020 08:37
@costin
Copy link
Member

costin commented Jul 17, 2020

I'll be on PTO in the upcoming weeks so I removed myself from the reviewers list.
Please wait for both their approval before merging this in.

Thanks,

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.

Looks good, but maybe add some test that if either of the args is null => null is returned.
And from what I see we need to update the docs to describe that null behaviour.

@rw-access rw-access requested a review from aleksmaus July 22, 2020 22:01
@matriv
Copy link
Contributor

matriv commented Jul 23, 2020

Looks good, but maybe add some test that if either of the args is null => null is returned.
And from what I see we need to update the docs to describe that null behaviour.

@rw-access Please don't forget ^^ .

@rw-access
Copy link
Contributor Author

@matriv re: null behavior

I was relying on these tests

[between]
description = "Test the proper evaluation of the `between` function"
[[between.fold.tests]]
expression = 'between("welcome to the planet", null, "planet")'
# expected = null
[[between.fold.tests]]
expression = 'between("welcome to the planet", "welcome", null)'
# expected = null
[[between.fold.tests]]
expression = 'between(null, "welcome", "planet")'
# expected = null
[[between.fold.tests]]
expression = 'between("welcome to the planet", "welcome", "planet")'
expected = " to the "
[[between.fold.tests]]
expression = 'between("welcome to the planet", "welcome", "village")'
# expected = null
[[between.fold.tests]]
expression = 'between("welcome to the planet", "goodbye", "planet")'
# expected = null
[[between.fold.tests]]
expression = 'between("welcome to the planet", "welcome", "e", false)'
expected = " to th"
[[between.fold.tests]]
expression = 'between("welcome to the planet", "welcome", "e", true)'
expected = " to the plan"
[[between.fold.tests]]
expression = 'between("welcome to the planet", "welcome", "x", false)'
# expected = null
[[between.fold.tests]]
expression = 'between("welcome to the planet", "welcome", "x", true)'
# expected = null
[[between.fold.tests]]
expression = 'between("welcome to the planet", "", "x", true)'
# expected = null
[[between.fold.tests]]
expression = 'between("welcome to the planet", "", "", true)'
expected = "welcome to the planet"
[[between.fold.tests]]
expression = 'between("welcome to the planet", "", "", false)'
expected = ""
[[between.fold.tests]]
expression = 'between("welcome to the planet", "", "planet", false)'
expected = "welcome to the "

but I'll add more tests to

@matriv
Copy link
Contributor

matriv commented Jul 23, 2020

Ah, I see, not insisting, just it would be nice to have a couple in Java unit tests too.

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 in general. I agree with @matriv about Java unit tests. The more, the better.
Also, it's surprising to me that the case sensitivity for between didn't make it for the first (experimental) release of eql.

@@ -40,7 +40,7 @@ public EqlFunctionRegistry() {
// Scalar functions
// String
new FunctionDefinition[] {
def(Between.class, Between::new, 2, "between"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the number of optional parameters is not needed anymore, maybe we should remove it from FunctionRegistry.

@rw-access
Copy link
Contributor Author

@elasticmachine update branch

@rw-access rw-access closed this Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/EQL EQL querying >enhancement Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EQL: implement case sensitivity for between string function
5 participants