From 1272ae411e638d7593c1f2a8779e317472fd6e96 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 16 Mar 2020 11:55:08 +0100 Subject: [PATCH] SQL: Fix issue with LIKE/RLIKE as painless script (#53495) Add missing asScript() implementation for LIKE/RLIKE expressions. When LIKE/RLIKE are used for example in GROUP BY or are wrapped with scalar functions in a WHERE clause, the translation must produce a painless script which will be executed to implement the correct behaviour and previously this was completely missing, and as a consquence wrong results were silently (no error) returned. Fixes: #53486 (cherry picked from commit eaa8ead6742a8e7dcf343bcbaff8de031550fd77) --- .../ql/expression/predicate/regex/Like.java | 13 -------- .../predicate/regex/LikePattern.java | 8 ++--- .../ql/expression/predicate/regex/RLike.java | 17 ++--------- .../predicate/regex/RLikePattern.java | 20 +++++++++++++ .../predicate/regex/RegexMatch.java | 30 +++++++++++++++++-- .../predicate/regex/StringPattern.java | 13 ++++++++ .../ql/planner/ExpressionTranslators.java | 4 +-- .../ql/optimizer/OptimizerRulesTests.java | 5 ++-- .../xpack/sql/parser/ExpressionBuilder.java | 3 +- .../xpack/sql/optimizer/OptimizerTests.java | 3 +- .../sql/planner/QueryTranslatorTests.java | 17 +++++++++++ 11 files changed, 92 insertions(+), 41 deletions(-) create mode 100644 x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RLikePattern.java create mode 100644 x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/StringPattern.java diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/Like.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/Like.java index dbe751276be..dfde94b5df7 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/Like.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/Like.java @@ -6,8 +6,6 @@ package org.elasticsearch.xpack.ql.expression.predicate.regex; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.gen.processor.Processor; -import org.elasticsearch.xpack.ql.expression.predicate.regex.RegexProcessor.RegexOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; @@ -26,15 +24,4 @@ public class Like extends RegexMatch { protected Like replaceChild(Expression newLeft) { return new Like(source(), newLeft, pattern()); } - - @Override - public Boolean fold() { - Object val = field().fold(); - return RegexOperation.match(val, pattern().asJavaRegex()); - } - - @Override - protected Processor makeProcessor() { - return new RegexProcessor(pattern().asJavaRegex()); - } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/LikePattern.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/LikePattern.java index fd6663b4b5a..096f7d8ae27 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/LikePattern.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/LikePattern.java @@ -17,7 +17,7 @@ import java.util.Objects; * * To prevent conflicts with ES, the string and char must be validated to not contain '*'. */ -public class LikePattern { +public class LikePattern implements StringPattern { private final String pattern; private final char escape; @@ -43,9 +43,7 @@ public class LikePattern { return escape; } - /** - * Returns the pattern in (Java) regex format. - */ + @Override public String asJavaRegex() { return regex; } @@ -83,4 +81,4 @@ public class LikePattern { return Objects.equals(pattern, other.pattern) && escape == other.escape; } -} \ No newline at end of file +} diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RLike.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RLike.java index 9ec70bc79d1..af40f4ffb17 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RLike.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RLike.java @@ -6,14 +6,12 @@ package org.elasticsearch.xpack.ql.expression.predicate.regex; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.gen.processor.Processor; -import org.elasticsearch.xpack.ql.expression.predicate.regex.RegexProcessor.RegexOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; -public class RLike extends RegexMatch { +public class RLike extends RegexMatch { - public RLike(Source source, Expression value, String pattern) { + public RLike(Source source, Expression value, RLikePattern pattern) { super(source, value, pattern); } @@ -26,15 +24,4 @@ public class RLike extends RegexMatch { protected RLike replaceChild(Expression newChild) { return new RLike(source(), newChild, pattern()); } - - @Override - public Boolean fold() { - Object val = field().fold(); - return RegexOperation.match(val, pattern()); - } - - @Override - protected Processor makeProcessor() { - return new RegexProcessor(pattern()); - } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RLikePattern.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RLikePattern.java new file mode 100644 index 00000000000..69747f03e21 --- /dev/null +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RLikePattern.java @@ -0,0 +1,20 @@ +/* + * 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.ql.expression.predicate.regex; + +public class RLikePattern implements StringPattern { + + private final String regexpPattern; + + public RLikePattern(String regexpPattern) { + this.regexpPattern = regexpPattern; + } + + @Override + public String asJavaRegex() { + return regexpPattern; + } +} diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java index 517c243992c..565a75d4897 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java @@ -10,15 +10,19 @@ import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.Expressions; import org.elasticsearch.xpack.ql.expression.Nullability; import org.elasticsearch.xpack.ql.expression.function.scalar.UnaryScalarFunction; +import org.elasticsearch.xpack.ql.expression.gen.processor.Processor; +import org.elasticsearch.xpack.ql.expression.gen.script.ScriptTemplate; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; import org.elasticsearch.xpack.ql.type.DataTypes; import java.util.Objects; +import static org.elasticsearch.common.logging.LoggerMessageFormat.format; import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isStringAndExact; +import static org.elasticsearch.xpack.ql.expression.gen.script.ParamsBuilder.paramsBuilder; -public abstract class RegexMatch extends UnaryScalarFunction { +public abstract class RegexMatch extends UnaryScalarFunction { private final T pattern; @@ -54,8 +58,30 @@ public abstract class RegexMatch extends UnaryScalarFunction { // right() is not directly foldable in any context but Like can fold it. return field().foldable(); } - + @Override + public Boolean fold() { + Object val = field().fold(); + return RegexProcessor.RegexOperation.match(val, pattern().asJavaRegex()); + } + + @Override + protected Processor makeProcessor() { + return new RegexProcessor(pattern().asJavaRegex()); + } + + @Override + public ScriptTemplate asScript() { + ScriptTemplate fieldAsScript = asScript(field()); + return new ScriptTemplate( + formatTemplate(format("{sql}.", "regex({},{})", fieldAsScript.template())), + paramsBuilder() + .script(fieldAsScript.params()) + .variable(pattern.asJavaRegex()) + .build(), + dataType()); + } + public boolean equals(Object obj) { return super.equals(obj) && Objects.equals(((RegexMatch) obj).pattern(), pattern()); } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/StringPattern.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/StringPattern.java new file mode 100644 index 00000000000..6dac865b4ac --- /dev/null +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/StringPattern.java @@ -0,0 +1,13 @@ +/* + * 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.ql.expression.predicate.regex; + +interface StringPattern { + /** + * Returns the pattern in (Java) regex format. + */ + String asJavaRegex(); +} diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java index 424a1d48661..2c2d6f657d1 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java @@ -112,7 +112,7 @@ public final class ExpressionTranslators { } if (e instanceof RLike) { - String pattern = ((RLike) e).pattern(); + String pattern = ((RLike) e).pattern().asJavaRegex(); q = new RegexQuery(e.source(), targetFieldName, pattern); } @@ -352,4 +352,4 @@ public final class ExpressionTranslators { } return new BoolQuery(source, isAnd, left, right); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java index b853443a0ea..9121b42e7c9 100644 --- a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java +++ b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java @@ -33,6 +33,7 @@ import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NullE import org.elasticsearch.xpack.ql.expression.predicate.regex.Like; import org.elasticsearch.xpack.ql.expression.predicate.regex.LikePattern; import org.elasticsearch.xpack.ql.expression.predicate.regex.RLike; +import org.elasticsearch.xpack.ql.expression.predicate.regex.RLikePattern; 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,13 +49,13 @@ import java.util.Collections; import java.util.List; import static java.util.Collections.emptyMap; +import static org.elasticsearch.xpack.ql.TestUtils.of; import static org.elasticsearch.xpack.ql.expression.Literal.FALSE; import static org.elasticsearch.xpack.ql.expression.Literal.NULL; import static org.elasticsearch.xpack.ql.expression.Literal.TRUE; import static org.elasticsearch.xpack.ql.tree.Source.EMPTY; import static org.elasticsearch.xpack.ql.type.DataTypes.BOOLEAN; import static org.elasticsearch.xpack.ql.type.DataTypes.INTEGER; -import static org.elasticsearch.xpack.ql.TestUtils.of; public class OptimizerRulesTests extends ESTestCase { @@ -189,7 +190,7 @@ public class OptimizerRulesTests extends ESTestCase { new ConstantFolding().rule(new Like(EMPTY, of("test_emp"), new LikePattern("test%", (char) 0))) .canonical()); assertEquals(TRUE, - new ConstantFolding().rule(new RLike(EMPTY, of("test_emp"), "test.emp")).canonical()); + new ConstantFolding().rule(new RLike(EMPTY, of("test_emp"), new RLikePattern("test.emp"))).canonical()); } public void testArithmeticFolding() { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java index 0455039a34e..bf8b22d8a39 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java @@ -44,6 +44,7 @@ import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NullE import org.elasticsearch.xpack.ql.expression.predicate.regex.Like; import org.elasticsearch.xpack.ql.expression.predicate.regex.LikePattern; import org.elasticsearch.xpack.ql.expression.predicate.regex.RLike; +import org.elasticsearch.xpack.ql.expression.predicate.regex.RLikePattern; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; import org.elasticsearch.xpack.ql.type.DataTypes; @@ -235,7 +236,7 @@ abstract class ExpressionBuilder extends IdentifierBuilder { e = new Like(source, exp, visitPattern(pCtx.pattern())); break; case SqlBaseParser.RLIKE: - e = new RLike(source, exp, string(pCtx.regex)); + e = new RLike(source, exp, new RLikePattern(string(pCtx.regex))); break; case SqlBaseParser.NULL: // shortcut to avoid double negation later on (since there's no IsNull (missing in ES is a negated exists)) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java index d4ef29f667e..922b32d199f 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java @@ -39,6 +39,7 @@ import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.LessT import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NullEquals; import org.elasticsearch.xpack.ql.expression.predicate.regex.RLike; +import org.elasticsearch.xpack.ql.expression.predicate.regex.RLikePattern; import org.elasticsearch.xpack.ql.index.EsIndex; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification; @@ -369,7 +370,7 @@ public class OptimizerTests extends ESTestCase { // comparison assertNullLiteral(rule.rule(new GreaterThan(EMPTY, getFieldAttribute(), NULL))); // regex - assertNullLiteral(rule.rule(new RLike(EMPTY, NULL, "123"))); + assertNullLiteral(rule.rule(new RLike(EMPTY, NULL, new RLikePattern("123")))); } public void testNullFoldingOnCast() { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index 736e94f29b1..eecfb2e48a9 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -547,6 +547,23 @@ public class QueryTranslatorTests extends ESTestCase { assertEquals("keyword", rqsq.field()); } + public void testLikeRLikeAsPainlessScripts() { + LogicalPlan p = plan("SELECT count(*), CASE WHEN keyword LIKE '%foo%' THEN 1 WHEN keyword RLIKE '.*bar.*' THEN 2 " + + "ELSE 3 END AS t FROM test GROUP BY t"); + assertTrue(p instanceof Aggregate); + Expression condition = ((Aggregate) p).groupings().get(0); + assertFalse(condition.foldable()); + GroupingContext groupingContext = QueryFolder.FoldAggregate.groupBy(((Aggregate) p).groupings()); + assertNotNull(groupingContext); + ScriptTemplate scriptTemplate = groupingContext.tail.script(); + assertEquals("InternalSqlScriptUtils.caseFunction([InternalSqlScriptUtils.regex(InternalSqlScriptUtils.docValue(" + + "doc,params.v0),params.v1),params.v2,InternalSqlScriptUtils.regex(InternalSqlScriptUtils.docValue(" + + "doc,params.v3),params.v4),params.v5,params.v6])", + scriptTemplate.toString()); + assertEquals("[{v=keyword}, {v=^.*foo.*$}, {v=1}, {v=keyword}, {v=.*bar.*}, {v=2}, {v=3}]", + scriptTemplate.params().toString()); + } + public void testTranslateNotExpression_WhereClause_Painless() { LogicalPlan p = plan("SELECT * FROM test WHERE NOT(POSITION('x', keyword) = 0)"); assertTrue(p instanceof Project);