-
Notifications
You must be signed in to change notification settings - Fork 25.6k
EQL: Add wildcard function #54020
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: Add wildcard function #54020
Changes from 1 commit
c0c7995
dea1f7b
d87361d
3826eac
f93d297
78f4fbd
c93b67c
169245d
5c7e147
e12a60c
502ca88
90dc8e0
d482c47
285582f
167c13a
d3b1dd1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,142 @@ | ||||||
| /* | ||||||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||||||
| * or more contributor license agreements. Licensed under the Elastic License; | ||||||
| * you may not use this file except in compliance with the Elastic License. | ||||||
| */ | ||||||
|
|
||||||
| package org.elasticsearch.xpack.eql.expression.function.scalar.string; | ||||||
|
|
||||||
| import org.elasticsearch.xpack.eql.utils.StringUtils; | ||||||
| import org.elasticsearch.xpack.ql.expression.Expression; | ||||||
| import org.elasticsearch.xpack.ql.expression.Expressions; | ||||||
| import org.elasticsearch.xpack.ql.expression.Expressions.ParamOrdinal; | ||||||
| import org.elasticsearch.xpack.ql.expression.function.scalar.ScalarFunction; | ||||||
| import org.elasticsearch.xpack.ql.expression.gen.pipeline.Pipe; | ||||||
| import org.elasticsearch.xpack.ql.expression.gen.script.ScriptTemplate; | ||||||
| import org.elasticsearch.xpack.ql.expression.predicate.logical.Or; | ||||||
| import org.elasticsearch.xpack.ql.expression.predicate.regex.Like; | ||||||
| import org.elasticsearch.xpack.ql.tree.NodeInfo; | ||||||
| import org.elasticsearch.xpack.ql.tree.Source; | ||||||
| import org.elasticsearch.xpack.ql.type.DataType; | ||||||
| import org.elasticsearch.xpack.ql.type.DataTypes; | ||||||
|
|
||||||
| import java.util.Collections; | ||||||
| import java.util.List; | ||||||
|
|
||||||
| import static org.elasticsearch.common.logging.LoggerMessageFormat.format; | ||||||
| import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isStringAndExact; | ||||||
|
|
||||||
| /** | ||||||
| * EQL wildcard function. Matches the form: | ||||||
| * wildcard(field, "*wildcard*pattern*", "*wildcard*pattern*") | ||||||
| */ | ||||||
|
|
||||||
rw-access marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| public class Wildcard extends ScalarFunction { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please implement the methods in the order of the super class (which is not alphabetical) which roughly is: constructor |
||||||
|
|
||||||
| private final Expression field; | ||||||
| private final List<Expression> patterns; | ||||||
|
|
||||||
| private static List<Expression> getArguments(Expression src, List<Expression> patterns) { | ||||||
rw-access marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| List<Expression> arguments = new java.util.ArrayList<>(Collections.singletonList(src)); | ||||||
rw-access marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| arguments.addAll(patterns); | ||||||
| return arguments; | ||||||
| } | ||||||
|
|
||||||
| public Wildcard(Source source, Expression field, List<Expression> patterns) { | ||||||
| super(source, getArguments(field, patterns)); | ||||||
|
||||||
| super(source, getArguments(field, patterns)); | |
| super(source, Arrays.asList(field, patterns)); |
like the rest of the subclasses.
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.
doesn't seem to work if the first value is a T, and the next is a List<T>
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.
CollectionUtils.combine(patterns, field) or if you want to preserve the order:
CollectionUtils.combine(singletonList(field), patterns)
Outdated
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.
Since it's always a Like or an Or, return a ScalarFunction instead. Also the method should be private.
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.
Currently accessing via the Optimizer, which is why it was left public:
https://github.com/elastic/elasticsearch/pull/54020/files#diff-cb6f05d0222f3291ea3df5d33525d6f6R71
Outdated
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.
childrenResolved() == false
Outdated
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.
I don't think isStringAndExact is correct here... "exact" refers to a field being of type keyword or having a sub-field of type keyword basically. isString should be enough imo.
Also, shouldn't the p.foldable() == false (comparison against variables basically) check be before this one?
rw-access marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
rw-access marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
rw-access marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ | |
|
|
||
| package org.elasticsearch.xpack.eql.optimizer; | ||
|
|
||
| import org.elasticsearch.xpack.eql.expression.function.scalar.string.Wildcard; | ||
| import org.elasticsearch.xpack.eql.utils.StringUtils; | ||
| import org.elasticsearch.xpack.ql.expression.Expression; | ||
| import org.elasticsearch.xpack.ql.expression.predicate.logical.Not; | ||
| import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNotNull; | ||
|
|
@@ -14,7 +16,6 @@ | |
| import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; | ||
| import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals; | ||
| import org.elasticsearch.xpack.ql.expression.predicate.regex.Like; | ||
| import org.elasticsearch.xpack.ql.expression.predicate.regex.LikePattern; | ||
| import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; | ||
| import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification; | ||
| import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineBinaryComparisons; | ||
|
|
@@ -48,6 +49,7 @@ protected Iterable<RuleExecutor<LogicalPlan>.Batch> batches() { | |
| new ReplaceNullChecks(), | ||
| new PropagateEquals(), | ||
| new CombineBinaryComparisons(), | ||
| new ReplaceWildcardFunction(), | ||
| // prune/elimination | ||
| new PruneFilters(), | ||
| new PruneLiteralsInOrderBy() | ||
|
|
@@ -60,6 +62,20 @@ protected Iterable<RuleExecutor<LogicalPlan>.Batch> batches() { | |
| } | ||
|
|
||
|
|
||
| private static class ReplaceWildcardFunction extends OptimizerRule<Filter> { | ||
|
|
||
| @Override | ||
| protected LogicalPlan rule(Filter filter) { | ||
| return filter.transformExpressionsUp(e -> { | ||
| if (e instanceof Wildcard) { | ||
| e = ((Wildcard) e).asLikes(); | ||
| } | ||
|
|
||
| return e; | ||
|
||
| }); | ||
| } | ||
| } | ||
|
|
||
| private static class ReplaceWildcards extends OptimizerRule<Filter> { | ||
|
|
||
| private static boolean isWildcard(Expression expr) { | ||
|
|
@@ -70,18 +86,6 @@ private static boolean isWildcard(Expression expr) { | |
| return false; | ||
| } | ||
|
|
||
| private static LikePattern toLikePattern(String s) { | ||
| // pick a character that is guaranteed not to be in the string, because it isn't allowed to escape itself | ||
| char escape = 1; | ||
|
|
||
| // replace wildcards with % and escape special characters | ||
| String likeString = s.replace("%", escape + "%") | ||
| .replace("_", escape + "_") | ||
| .replace("*", "%"); | ||
|
|
||
| return new LikePattern(likeString, escape); | ||
| } | ||
|
|
||
| @Override | ||
| protected LogicalPlan rule(Filter filter) { | ||
| return filter.transformExpressionsUp(e -> { | ||
|
|
@@ -91,7 +95,7 @@ protected LogicalPlan rule(Filter filter) { | |
|
|
||
| if (isWildcard(cmp.right())) { | ||
| String wcString = cmp.right().fold().toString(); | ||
| Expression like = new Like(e.source(), cmp.left(), toLikePattern(wcString)); | ||
| Expression like = new Like(e.source(), cmp.left(), StringUtils.toLikePattern(wcString)); | ||
|
|
||
| if (e instanceof NotEquals) { | ||
| like = new Not(e.source(), like); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.xpack.eql.utils; | ||
|
|
||
| import org.elasticsearch.xpack.ql.expression.predicate.regex.LikePattern; | ||
|
|
||
| public class StringUtils { | ||
rw-access marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| public static LikePattern toLikePattern(String s) { | ||
| // pick a character that is guaranteed not to be in the string, because it isn't allowed to escape itself | ||
| char escape = 1; | ||
|
|
||
| // replace wildcards with % and escape special characters | ||
| String likeString = s.replace("%", escape + "%") | ||
| .replace("_", escape + "_") | ||
| .replace("*", "%"); | ||
|
|
||
| return new LikePattern(likeString, escape); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ | |
|
|
||
| package org.elasticsearch.xpack.eql.planner; | ||
|
|
||
| import org.elasticsearch.xpack.eql.analysis.VerificationException; | ||
| import org.elasticsearch.xpack.ql.ParsingException; | ||
| import org.elasticsearch.xpack.ql.QlIllegalArgumentException; | ||
|
|
||
| public class QueryFolderFailTests extends AbstractQueryFolderTestCase { | ||
|
|
@@ -22,4 +24,19 @@ public void testPropertyEquationInClauseFilterUnsupported() { | |
| String msg = e.getMessage(); | ||
| assertEquals("Line 1:52: Comparisons against variables are not (currently) supported; offender [parent_process_name] in [==]", msg); | ||
| } | ||
|
|
||
| public void testWildcardNotEnoughArguments() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there are other error messages to check with |
||
| ParsingException e = expectThrows(ParsingException.class, | ||
| () -> plan("process where wildcard(process_name)")); | ||
| String msg = e.getMessage(); | ||
| assertEquals("line 1:16: error building [wildcard]: expects at least two arguments", msg); | ||
| } | ||
|
|
||
| public void testWildcardAgainstVariable() { | ||
| VerificationException e = expectThrows(VerificationException.class, | ||
| () -> plan("process where wildcard(process_name, parent_process_name)")); | ||
| String msg = e.getMessage(); | ||
| assertEquals("Found 1 problem\nline 1:15: wildcard against variables are not (currently) supported;" + | ||
| " offender [parent_process_name] in [wildcard(process_name, parent_process_name)]", msg); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -421,4 +421,26 @@ public static <T extends Function> FunctionDefinition def(Class<T> function, | |
| protected interface CastFunctionBuilder<T> { | ||
| T build(Source source, Expression expression, DataType dataType); | ||
| } | ||
|
|
||
| @SuppressWarnings("overloads") // These are ambiguous if you aren't using ctor references but we always do | ||
| public static <T extends Function> FunctionDefinition def(Class<T> function, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How many arguments does wildcard expect?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. at least two, but it's unbounded in the maximum number |
||
| TwoParametersVariadicBuilder<T> ctorRef, String... names) { | ||
| FunctionBuilder builder = (source, children, distinct, cfg) -> { | ||
| boolean hasMinimumOne = OptionalArgument.class.isAssignableFrom(function); | ||
| if (hasMinimumOne && children.size() < 1) { | ||
| throw new QlIllegalArgumentException("expects at least one argument"); | ||
| } else if (!hasMinimumOne && children.size() < 2) { | ||
| throw new QlIllegalArgumentException("expects at least two arguments"); | ||
| } | ||
| if (distinct) { | ||
| throw new QlIllegalArgumentException("does not support DISTINCT yet it was specified"); | ||
| } | ||
| return ctorRef.build(source, children.get(0), children.subList(1, children.size())); | ||
| }; | ||
| return def(function, builder, false, names); | ||
| } | ||
|
|
||
| protected interface TwoParametersVariadicBuilder<T> { | ||
| T build(Source source, Expression src, List<Expression> remaining); | ||
| } | ||
| } | ||
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.
Isn't
wildcarda "string function"? If so, it should belong to the FunctionDefinition array that, also, hassubstringin it. In SQL we were grouping these functions by their type: string, grouping, math, conditional, date etc.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.
oh right, good catch