Skip to content

Commit 55ff96b

Browse files
author
Marios Trivyzas
authored
SQL: Fix query translation for scripted queries (#35408)
IsNull, IsNotNull and Not where generating wrong queries as the check to generate ScriptQuery was missing. Fixes: #35232
1 parent ef10461 commit 55ff96b

File tree

4 files changed

+72
-59
lines changed

4 files changed

+72
-59
lines changed

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Neg.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
import org.elasticsearch.xpack.sql.expression.NamedExpression;
1212
import org.elasticsearch.xpack.sql.expression.function.scalar.UnaryScalarFunction;
1313
import org.elasticsearch.xpack.sql.expression.gen.processor.Processor;
14-
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptWeaver;
1514
import org.elasticsearch.xpack.sql.expression.gen.script.Scripts;
1615
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.UnaryArithmeticProcessor.UnaryArithmeticOperation;
1716
import org.elasticsearch.xpack.sql.tree.Location;
@@ -21,7 +20,7 @@
2120
/**
2221
* Negation function (@{code -x}).
2322
*/
24-
public class Neg extends UnaryScalarFunction implements ScriptWeaver {
23+
public class Neg extends UnaryScalarFunction {
2524

2625
public Neg(Location location, Expression field) {
2726
super(location, field);

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java‎

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,12 @@
55
*/
66
package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison;
77

8-
import org.elasticsearch.xpack.sql.expression.Attribute;
98
import org.elasticsearch.xpack.sql.expression.Expression;
109
import org.elasticsearch.xpack.sql.expression.Expressions;
1110
import org.elasticsearch.xpack.sql.expression.Foldables;
12-
import org.elasticsearch.xpack.sql.expression.NamedExpression;
13-
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunctionAttribute;
11+
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction;
1412
import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe;
1513
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate;
16-
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptWeaver;
1714
import org.elasticsearch.xpack.sql.tree.Location;
1815
import org.elasticsearch.xpack.sql.tree.NodeInfo;
1916
import org.elasticsearch.xpack.sql.type.DataType;
@@ -29,14 +26,13 @@
2926

3027
import static org.elasticsearch.xpack.sql.expression.gen.script.ParamsBuilder.paramsBuilder;
3128

32-
public class In extends NamedExpression implements ScriptWeaver {
29+
public class In extends ScalarFunction {
3330

3431
private final Expression value;
3532
private final List<Expression> list;
36-
private Attribute lazyAttribute;
3733

3834
public In(Location location, Expression value, List<Expression> list) {
39-
super(location, null, CollectionUtils.combine(list, value), null);
35+
super(location, CollectionUtils.combine(list, value));
4036
this.value = value;
4137
this.list = new ArrayList<>(new LinkedHashSet<>(list));
4238
}
@@ -95,15 +91,6 @@ public String name() {
9591
return Expressions.name(value) + sj.toString();
9692
}
9793

98-
@Override
99-
public Attribute toAttribute() {
100-
if (lazyAttribute == null) {
101-
lazyAttribute = new ScalarFunctionAttribute(location(), name(), dataType(), null,
102-
false, id(), false, "IN", asScript(), null, asPipe());
103-
}
104-
return lazyAttribute;
105-
}
106-
10794
@Override
10895
public ScriptTemplate asScript() {
10996
ScriptTemplate leftScript = asScript(value);

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java‎

Lines changed: 22 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,6 @@
3030
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction;
3131
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFunction;
3232
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeHistogramFunction;
33-
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate;
34-
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNull;
3533
import org.elasticsearch.xpack.sql.expression.predicate.Range;
3634
import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MatchQueryPredicate;
3735
import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MultiMatchQueryPredicate;
@@ -40,6 +38,7 @@
4038
import org.elasticsearch.xpack.sql.expression.predicate.logical.Not;
4139
import org.elasticsearch.xpack.sql.expression.predicate.logical.Or;
4240
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNotNull;
41+
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNull;
4342
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparison;
4443
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.Equals;
4544
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.GreaterThan;
@@ -94,6 +93,7 @@
9493
import java.util.Map;
9594
import java.util.Map.Entry;
9695
import java.util.Optional;
96+
import java.util.function.Supplier;
9797

9898
import static java.util.Collections.singletonList;
9999
import static org.elasticsearch.xpack.sql.expression.Foldables.doubleValuesOf;
@@ -487,11 +487,8 @@ protected QueryTranslation asQuery(Not not, boolean onAggs) {
487487
if (onAggs) {
488488
aggFilter = new AggFilter(not.id().toString(), not.asScript());
489489
} else {
490-
query = new NotQuery(not.location(), toQuery(not.field(), false).query);
491-
// query directly on the field
492-
if (not.field() instanceof FieldAttribute) {
493-
query = wrapIfNested(query, not.field());
494-
}
490+
query = handleQuery(not, not.field(),
491+
() -> new NotQuery(not.location(), toQuery(not.field(), false).query));
495492
}
496493

497494
return new QueryTranslation(query, aggFilter);
@@ -508,11 +505,8 @@ protected QueryTranslation asQuery(IsNotNull isNotNull, boolean onAggs) {
508505
if (onAggs) {
509506
aggFilter = new AggFilter(isNotNull.id().toString(), isNotNull.asScript());
510507
} else {
511-
query = new ExistsQuery(isNotNull.location(), nameOf(isNotNull.field()));
512-
// query directly on the field
513-
if (isNotNull.field() instanceof NamedExpression) {
514-
query = wrapIfNested(query, isNotNull.field());
515-
}
508+
query = handleQuery(isNotNull, isNotNull.field(),
509+
() -> new ExistsQuery(isNotNull.location(), nameOf(isNotNull.field())));
516510
}
517511

518512
return new QueryTranslation(query, aggFilter);
@@ -529,11 +523,8 @@ protected QueryTranslation asQuery(IsNull isNull, boolean onAggs) {
529523
if (onAggs) {
530524
aggFilter = new AggFilter(isNull.id().toString(), isNull.asScript());
531525
} else {
532-
query = new NotQuery(isNull.location(), new ExistsQuery(isNull.location(), nameOf(isNull.field())));
533-
// query directly on the field
534-
if (isNull.field() instanceof NamedExpression) {
535-
query = wrapIfNested(query, isNull.field());
536-
}
526+
query = handleQuery(isNull, isNull.field(),
527+
() -> new NotQuery(isNull.location(), new ExistsQuery(isNull.location(), nameOf(isNull.field()))));
537528
}
538529

539530
return new QueryTranslation(query, aggFilter);
@@ -564,12 +555,7 @@ protected QueryTranslation asQuery(BinaryComparison bc, boolean onAggs) {
564555
aggFilter = new AggFilter(at.id().toString(), bc.asScript());
565556
}
566557
else {
567-
// query directly on the field
568-
if (at instanceof FieldAttribute) {
569-
query = wrapIfNested(translateQuery(bc), ne);
570-
} else {
571-
query = new ScriptQuery(at.location(), bc.asScript());
572-
}
558+
query = handleQuery(bc, ne, () -> translateQuery(bc));
573559
}
574560
return new QueryTranslation(query, aggFilter);
575561
}
@@ -646,17 +632,11 @@ protected QueryTranslation asQuery(In in, boolean onAggs) {
646632
//
647633
// Agg context means HAVING -> PipelineAggs
648634
//
649-
ScriptTemplate script = in.asScript();
650635
if (onAggs) {
651-
aggFilter = new AggFilter(at.id().toString(), script);
636+
aggFilter = new AggFilter(at.id().toString(), in.asScript());
652637
}
653638
else {
654-
// query directly on the field
655-
if (at instanceof FieldAttribute) {
656-
query = wrapIfNested(new TermsQuery(in.location(), ne.name(), in.list()), ne);
657-
} else {
658-
query = new ScriptQuery(at.location(), script);
659-
}
639+
query = handleQuery(in, ne, () -> new TermsQuery(in.location(), ne.name(), in.list()));
660640
}
661641
return new QueryTranslation(query, aggFilter);
662642
}
@@ -687,16 +667,9 @@ protected QueryTranslation asQuery(Range r, boolean onAggs) {
687667
if (onAggs) {
688668
aggFilter = new AggFilter(at.id().toString(), r.asScript());
689669
} else {
690-
// typical range; no scripting involved
691-
if (at instanceof FieldAttribute) {
692-
RangeQuery rangeQuery = new RangeQuery(r.location(), nameOf(r.value()), valueOf(r.lower()), r.includeLower(),
693-
valueOf(r.upper()), r.includeUpper(), dateFormat(r.value()));
694-
query = wrapIfNested(rangeQuery, r.value());
695-
}
696-
// scripted query
697-
else {
698-
query = new ScriptQuery(at.location(), r.asScript());
699-
}
670+
query = handleQuery(r, r.value(),
671+
() -> new RangeQuery(r.location(), nameOf(r.value()), valueOf(r.lower()), r.includeLower(),
672+
valueOf(r.upper()), r.includeUpper(), dateFormat(r.value())));
700673
}
701674
return new QueryTranslation(query, aggFilter);
702675
} else {
@@ -845,6 +818,14 @@ public QueryTranslation translate(Expression exp, boolean onAggs) {
845818

846819
protected abstract QueryTranslation asQuery(E e, boolean onAggs);
847820

821+
822+
protected static Query handleQuery(ScalarFunction sf, Expression field, Supplier<Query> query) {
823+
if (field instanceof FieldAttribute) {
824+
return wrapIfNested(query.get(), field);
825+
}
826+
return new ScriptQuery(sf.location(), sf.asScript());
827+
}
828+
848829
protected static Query wrapIfNested(Query query, Expression exp) {
849830
if (exp instanceof FieldAttribute) {
850831
FieldAttribute fa = (FieldAttribute) exp;

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,22 @@ public void testLikeConstructsNotSupported() {
163163
assertEquals("Scalar function (LTRIM(keyword)) not allowed (yet) as arguments for LIKE", ex.getMessage());
164164
}
165165

166+
public void testTranslateNotExpression_WhereClause_Painless() {
167+
LogicalPlan p = plan("SELECT * FROM test WHERE NOT(POSITION('x', keyword) = 0)");
168+
assertTrue(p instanceof Project);
169+
assertTrue(p.children().get(0) instanceof Filter);
170+
Expression condition = ((Filter) p.children().get(0)).condition();
171+
assertFalse(condition.foldable());
172+
QueryTranslation translation = QueryTranslator.toQuery(condition, false);
173+
assertTrue(translation.query instanceof ScriptQuery);
174+
ScriptQuery sc = (ScriptQuery) translation.query;
175+
assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.not(" +
176+
"InternalSqlScriptUtils.eq(InternalSqlScriptUtils.position(" +
177+
"params.v0,InternalSqlScriptUtils.docValue(doc,params.v1)),params.v2)))",
178+
sc.script().toString());
179+
assertEquals("[{v=x}, {v=keyword}, {v=0}]", sc.script().params().toString());
180+
}
181+
166182
public void testTranslateIsNullExpression_WhereClause() {
167183
LogicalPlan p = plan("SELECT * FROM test WHERE keyword IS NULL");
168184
assertTrue(p instanceof Project);
@@ -178,6 +194,21 @@ public void testTranslateIsNullExpression_WhereClause() {
178194
eq.asBuilder().toString().replaceAll("\\s+", ""));
179195
}
180196

197+
public void testTranslateIsNullExpression_WhereClause_Painless() {
198+
LogicalPlan p = plan("SELECT * FROM test WHERE POSITION('x', keyword) IS NULL");
199+
assertTrue(p instanceof Project);
200+
assertTrue(p.children().get(0) instanceof Filter);
201+
Expression condition = ((Filter) p.children().get(0)).condition();
202+
assertFalse(condition.foldable());
203+
QueryTranslation translation = QueryTranslator.toQuery(condition, false);
204+
assertTrue(translation.query instanceof ScriptQuery);
205+
ScriptQuery sc = (ScriptQuery) translation.query;
206+
assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.isNull(" +
207+
"InternalSqlScriptUtils.position(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))))",
208+
sc.script().toString());
209+
assertEquals("[{v=x}, {v=keyword}]", sc.script().params().toString());
210+
}
211+
181212
public void testTranslateIsNotNullExpression_WhereClause() {
182213
LogicalPlan p = plan("SELECT * FROM test WHERE keyword IS NOT NULL");
183214
assertTrue(p instanceof Project);
@@ -191,6 +222,21 @@ public void testTranslateIsNotNullExpression_WhereClause() {
191222
eq.asBuilder().toString().replaceAll("\\s+", ""));
192223
}
193224

225+
public void testTranslateIsNotNullExpression_WhereClause_Painless() {
226+
LogicalPlan p = plan("SELECT * FROM test WHERE POSITION('x', keyword) IS NOT NULL");
227+
assertTrue(p instanceof Project);
228+
assertTrue(p.children().get(0) instanceof Filter);
229+
Expression condition = ((Filter) p.children().get(0)).condition();
230+
assertFalse(condition.foldable());
231+
QueryTranslation translation = QueryTranslator.toQuery(condition, false);
232+
assertTrue(translation.query instanceof ScriptQuery);
233+
ScriptQuery sc = (ScriptQuery) translation.query;
234+
assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.isNotNull(" +
235+
"InternalSqlScriptUtils.position(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))))",
236+
sc.script().toString());
237+
assertEquals("[{v=x}, {v=keyword}]", sc.script().params().toString());
238+
}
239+
194240
public void testTranslateIsNullExpression_HavingClause_Painless() {
195241
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IS NULL");
196242
assertTrue(p instanceof Project);

0 commit comments

Comments
 (0)