Skip to content

Commit cab2fa1

Browse files
Marios Trivyzasmatriv
Marios Trivyzas
authored and
matriv
committed
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 19a6e9e commit cab2fa1

4 files changed

Lines changed: 72 additions & 58 deletions

File tree

‎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 & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +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;
3433
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNotNull;
3534
import org.elasticsearch.xpack.sql.expression.predicate.Range;
3635
import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MatchQueryPredicate;
@@ -39,6 +38,7 @@
3938
import org.elasticsearch.xpack.sql.expression.predicate.logical.And;
4039
import org.elasticsearch.xpack.sql.expression.predicate.logical.Not;
4140
import org.elasticsearch.xpack.sql.expression.predicate.logical.Or;
41+
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNotNull;
4242
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNull;
4343
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparison;
4444
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.Equals;
@@ -94,6 +94,7 @@
9494
import java.util.Map;
9595
import java.util.Map.Entry;
9696
import java.util.Optional;
97+
import java.util.function.Supplier;
9798

9899
import static java.util.Collections.singletonList;
99100
import static org.elasticsearch.xpack.sql.expression.Foldables.doubleValuesOf;
@@ -487,11 +488,8 @@ protected QueryTranslation asQuery(Not not, boolean onAggs) {
487488
if (onAggs) {
488489
aggFilter = new AggFilter(not.id().toString(), not.asScript());
489490
} 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-
}
491+
query = handleQuery(not, not.field(),
492+
() -> new NotQuery(not.location(), toQuery(not.field(), false).query));
495493
}
496494

497495
return new QueryTranslation(query, aggFilter);
@@ -508,11 +506,8 @@ protected QueryTranslation asQuery(IsNotNull isNotNull, boolean onAggs) {
508506
if (onAggs) {
509507
aggFilter = new AggFilter(isNotNull.id().toString(), isNotNull.asScript());
510508
} 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-
}
509+
query = handleQuery(isNotNull, isNotNull.field(),
510+
() -> new ExistsQuery(isNotNull.location(), nameOf(isNotNull.field())));
516511
}
517512

518513
return new QueryTranslation(query, aggFilter);
@@ -529,11 +524,8 @@ protected QueryTranslation asQuery(IsNull isNull, boolean onAggs) {
529524
if (onAggs) {
530525
aggFilter = new AggFilter(isNull.id().toString(), isNull.asScript());
531526
} 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-
}
527+
query = handleQuery(isNull, isNull.field(),
528+
() -> new NotQuery(isNull.location(), new ExistsQuery(isNull.location(), nameOf(isNull.field()))));
537529
}
538530

539531
return new QueryTranslation(query, aggFilter);
@@ -564,12 +556,7 @@ protected QueryTranslation asQuery(BinaryComparison bc, boolean onAggs) {
564556
aggFilter = new AggFilter(at.id().toString(), bc.asScript());
565557
}
566558
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-
}
559+
query = handleQuery(bc, ne, () -> translateQuery(bc));
573560
}
574561
return new QueryTranslation(query, aggFilter);
575562
}
@@ -646,17 +633,11 @@ protected QueryTranslation asQuery(In in, boolean onAggs) {
646633
//
647634
// Agg context means HAVING -> PipelineAggs
648635
//
649-
ScriptTemplate script = in.asScript();
650636
if (onAggs) {
651-
aggFilter = new AggFilter(at.id().toString(), script);
637+
aggFilter = new AggFilter(at.id().toString(), in.asScript());
652638
}
653639
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-
}
640+
query = handleQuery(in, ne, () -> new TermsQuery(in.location(), ne.name(), in.list()));
660641
}
661642
return new QueryTranslation(query, aggFilter);
662643
}
@@ -687,16 +668,9 @@ protected QueryTranslation asQuery(Range r, boolean onAggs) {
687668
if (onAggs) {
688669
aggFilter = new AggFilter(at.id().toString(), r.asScript());
689670
} 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-
}
671+
query = handleQuery(r, r.value(),
672+
() -> new RangeQuery(r.location(), nameOf(r.value()), valueOf(r.lower()), r.includeLower(),
673+
valueOf(r.upper()), r.includeUpper(), dateFormat(r.value())));
700674
}
701675
return new QueryTranslation(query, aggFilter);
702676
} else {
@@ -845,6 +819,14 @@ public QueryTranslation translate(Expression exp, boolean onAggs) {
845819

846820
protected abstract QueryTranslation asQuery(E e, boolean onAggs);
847821

822+
823+
protected static Query handleQuery(ScalarFunction sf, Expression field, Supplier<Query> query) {
824+
if (field instanceof FieldAttribute) {
825+
return wrapIfNested(query.get(), field);
826+
}
827+
return new ScriptQuery(sf.location(), sf.asScript());
828+
}
829+
848830
protected static Query wrapIfNested(Query query, Expression exp) {
849831
if (exp instanceof FieldAttribute) {
850832
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)