-
Notifications
You must be signed in to change notification settings - Fork 25k
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: Remove "wildcard" function #76099
Conversation
This removes "wildcard" as an available function in EQL. This has already been replace with "like" and "regex" embedded synthax (and respective case insensitive variants).
Pinging @elastic/es-ql (Team:QL) |
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.
My only note is that I'm not sure where wildcard
should be deleted and where it should be changed to like
.
[[queries]] | ||
name = "wildcardFunctionWildcardPattern" | ||
query = ''' | ||
file where wildcard(file_name, "winini*.exe", "lsass.*") and opcode == 2 |
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.
should this be replaced with like
?
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.
There's already a like
test for this (likeMultipleArgWithPattern
).
@@ -411,73 +411,3 @@ description = "Test the `substring` function when the case already matches" | |||
[[substring.fold.tests]] | |||
expression = '''substring("hello world", -5, -1)''' | |||
expected = "worl" | |||
|
|||
[wildcard] |
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.
should this one be replaced with like
? is this file even used?
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.
Yes, file still in use.
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 👍
I've tried to preserve the wildcard-now-like tests where they're part of a more complex suite (like those in queries.toml) and removed them where |
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.
Assuming the removed tests already have equivalent for like
the only comment I have is around fully removing the Wildcard
class which is not needed and the substitute construct can be created directly by the parser.
@@ -205,35 +205,4 @@ public void testStringContainsWrongParams() { | |||
assertEquals("1:15: second argument of [stringContains(process_name, 1)] must be [string], found value [1] type [integer]", | |||
error("process where stringContains(process_name, 1)")); | |||
} | |||
|
|||
public void testWildcardNotEnoughArguments() { |
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.
Please convert these to like
if there aren't tests that already do that.
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.
These tests become now syntactic failures. One case could however be indeed converted (ex testWildcardWithNumericField
).
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.
As long as we have equivalent semantical tests, I'm 👍 . Note that wildcard tests that rely on the syntax can be changed to like/like~
hence my point of replacing removed tests (or checking) there are semantical equivalents to them.
@@ -31,7 +31,7 @@ | |||
|
|||
/** | |||
* EQL wildcard function. Matches the form: | |||
* wildcard(field, "*wildcard*pattern*", ...) | |||
* field like ("*wildcard*pattern*", ...) | |||
*/ | |||
public class Wildcard extends BaseSurrogateFunction { |
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.
No need to keep this class around.
It's used inside the parser ExpressionBuilder#visitOperatorExpressionDefault
however its only role is to call makeSubstitute
which is similar to ExpressionBuilder#combineExpressions
. See the handling of EqlBaseParser.IN_INSENSITIVE:
in visitOperatorExpressionDefault
.
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.
Thanks. Class removed.
- remove Wildcard class; - recover one failed translation test.
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
@elasticmachine run elasticsearch-ci/bwc |
@elasticmachine update branch |
* master: (868 commits) Query API key - Rest spec and yaml tests (elastic#76238) Delay shard reassignment from nodes which are known to be restarting (elastic#75606) Reenable bwc tests for elastic#76475 (elastic#76576) Set version to 7.15 in BWC code (elastic#76577) Don't remove warning headers on all failure (elastic#76434) Disable bwc tests for elastic#76475 (elastic#76541) Re-enable bwc tests (elastic#76567) Keep track of data recovered from snapshots in RecoveryState (elastic#76499) [Transform] Align transform checkpoint range with date_histogram interval for better performance (elastic#74004) EQL: Remove "wildcard" function (elastic#76099) Fix 'accept' and 'content_type' fields for search_mvt API Add persistent licensed feature tracking (elastic#76476) Add system data streams to feature state snapshots (elastic#75902) fix the error message for instance methods that don't exist (elastic#76512) ILM: Add validation of the number_of_shards parameter in Shrink Action of ILM (elastic#74219) remove dashboard only reserved role (elastic#76507) Fix Stack Overflow in UnassignedInfo in Corner Case (elastic#76480) Add (Extended)KeyUsage KeyUsage, CipherSuite & Protocol to SSL diagnostics (elastic#65634) Add recovery from snapshot to tests (elastic#76535) Reenable BwC Tests after elastic#76532 (elastic#76534) ...
This removes "wildcard" as an available function in EQL. This has
already been replace with "like" and "regex" embedded synthax (and
respective case insensitive variants).
Relates #71906.