Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SQL: Fix query translation for scripted queries #35408

Merged
merged 3 commits into from
Nov 12, 2018
Merged

SQL: Fix query translation for scripted queries #35408

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb70e44
SQL: Fix query translation for scripted queries
matriv Nov 9, 2018
761087f
Address comments
matriv Nov 11, 2018
d06c62c
Merge remote-tracking branch 'upstream/master' into mt/fix-35232
matriv Nov 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions ...c/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Neg.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import org.elasticsearch.xpack.sql.expression.NamedExpression;
import org.elasticsearch.xpack.sql.expression.function.scalar.UnaryScalarFunction;
import org.elasticsearch.xpack.sql.expression.gen.processor.Processor;
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptWeaver;
import org.elasticsearch.xpack.sql.expression.gen.script.Scripts;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.UnaryArithmeticProcessor.UnaryArithmeticOperation;
import org.elasticsearch.xpack.sql.tree.Location;
Expand All @@ -21,7 +20,7 @@
/**
* Negation function (@{code -x}).
*/
public class Neg extends UnaryScalarFunction implements ScriptWeaver {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScalarFunction already implements ScriptWeaver

public class Neg extends UnaryScalarFunction {

public Neg(Location location, Expression field) {
super(location, field);
Expand Down
19 changes: 3 additions & 16 deletions ...rc/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,12 @@
*/
package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison;

import org.elasticsearch.xpack.sql.expression.Attribute;
import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.expression.Expressions;
import org.elasticsearch.xpack.sql.expression.Foldables;
import org.elasticsearch.xpack.sql.expression.NamedExpression;
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunctionAttribute;
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction;
import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe;
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate;
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptWeaver;
import org.elasticsearch.xpack.sql.tree.Location;
import org.elasticsearch.xpack.sql.tree.NodeInfo;
import org.elasticsearch.xpack.sql.type.DataType;
Expand All @@ -29,14 +26,13 @@

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

public class In extends NamedExpression implements ScriptWeaver {
public class In extends ScalarFunction {

private final Expression value;
private final List<Expression> list;
private Attribute lazyAttribute;

public In(Location location, Expression value, List<Expression> list) {
super(location, null, CollectionUtils.combine(list, value), null);
super(location, CollectionUtils.combine(list, value));
this.value = value;
this.list = new ArrayList<>(new LinkedHashSet<>(list));
}
Expand Down Expand Up @@ -95,15 +91,6 @@ public String name() {
return Expressions.name(value) + sj.toString();
}

@Override
public Attribute toAttribute() {
if (lazyAttribute == null) {
lazyAttribute = new ScalarFunctionAttribute(location(), name(), dataType(), null,
false, id(), false, "IN", asScript(), null, asPipe());
}
return lazyAttribute;
}

@Override
public ScriptTemplate asScript() {
ScriptTemplate leftScript = asScript(value);
Expand Down
63 changes: 22 additions & 41 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFunction;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeHistogramFunction;
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate;
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNull;
import org.elasticsearch.xpack.sql.expression.predicate.Range;
import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MatchQueryPredicate;
import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MultiMatchQueryPredicate;
Expand All @@ -40,6 +38,7 @@
import org.elasticsearch.xpack.sql.expression.predicate.logical.Not;
import org.elasticsearch.xpack.sql.expression.predicate.logical.Or;
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNotNull;
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNull;
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparison;
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.Equals;
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.GreaterThan;
Expand Down Expand Up @@ -94,6 +93,7 @@
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.function.Supplier;

import static java.util.Collections.singletonList;
import static org.elasticsearch.xpack.sql.expression.Foldables.doubleValuesOf;
Expand Down Expand Up @@ -487,11 +487,8 @@ protected QueryTranslation asQuery(Not not, boolean onAggs) {
if (onAggs) {
aggFilter = new AggFilter(not.id().toString(), not.asScript());
} else {
query = new NotQuery(not.location(), toQuery(not.field(), false).query);
// query directly on the field
if (not.field() instanceof FieldAttribute) {
query = wrapIfNested(query, not.field());
}
query = generateQuery(not, not.field(),
() -> new NotQuery(not.location(), toQuery(not.field(), false).query));
}

return new QueryTranslation(query, aggFilter);
Expand All @@ -508,11 +505,8 @@ protected QueryTranslation asQuery(IsNotNull isNotNull, boolean onAggs) {
if (onAggs) {
aggFilter = new AggFilter(isNotNull.id().toString(), isNotNull.asScript());
} else {
query = new ExistsQuery(isNotNull.location(), nameOf(isNotNull.field()));
// query directly on the field
if (isNotNull.field() instanceof NamedExpression) {
query = wrapIfNested(query, isNotNull.field());
}
query = generateQuery(isNotNull, isNotNull.field(),
() -> new ExistsQuery(isNotNull.location(), nameOf(isNotNull.field())));
}

return new QueryTranslation(query, aggFilter);
Expand All @@ -529,11 +523,8 @@ protected QueryTranslation asQuery(IsNull isNull, boolean onAggs) {
if (onAggs) {
aggFilter = new AggFilter(isNull.id().toString(), isNull.asScript());
} else {
query = new NotQuery(isNull.location(), new ExistsQuery(isNull.location(), nameOf(isNull.field())));
// query directly on the field
if (isNull.field() instanceof NamedExpression) {
query = wrapIfNested(query, isNull.field());
}
query = generateQuery(isNull, isNull.field(),
() -> new NotQuery(isNull.location(), new ExistsQuery(isNull.location(), nameOf(isNull.field()))));
}

return new QueryTranslation(query, aggFilter);
Expand Down Expand Up @@ -564,12 +555,7 @@ protected QueryTranslation asQuery(BinaryComparison bc, boolean onAggs) {
aggFilter = new AggFilter(at.id().toString(), bc.asScript());
}
else {
// query directly on the field
if (at instanceof FieldAttribute) {
query = wrapIfNested(translateQuery(bc), ne);
} else {
query = new ScriptQuery(at.location(), bc.asScript());
}
query = generateQuery(bc, ne, () -> translateQuery(bc));
}
return new QueryTranslation(query, aggFilter);
}
Expand Down Expand Up @@ -646,17 +632,11 @@ protected QueryTranslation asQuery(In in, boolean onAggs) {
//
// Agg context means HAVING -> PipelineAggs
//
ScriptTemplate script = in.asScript();
if (onAggs) {
aggFilter = new AggFilter(at.id().toString(), script);
aggFilter = new AggFilter(at.id().toString(), in.asScript());
}
else {
// query directly on the field
if (at instanceof FieldAttribute) {
query = wrapIfNested(new TermsQuery(in.location(), ne.name(), in.list()), ne);
} else {
query = new ScriptQuery(at.location(), script);
}
query = generateQuery(in, ne, () -> new TermsQuery(in.location(), ne.name(), in.list()));
}
return new QueryTranslation(query, aggFilter);
}
Expand Down Expand Up @@ -687,16 +667,9 @@ protected QueryTranslation asQuery(Range r, boolean onAggs) {
if (onAggs) {
aggFilter = new AggFilter(at.id().toString(), r.asScript());
} else {
// typical range; no scripting involved
if (at instanceof FieldAttribute) {
RangeQuery rangeQuery = new RangeQuery(r.location(), nameOf(r.value()), valueOf(r.lower()), r.includeLower(),
valueOf(r.upper()), r.includeUpper(), dateFormat(r.value()));
query = wrapIfNested(rangeQuery, r.value());
}
// scripted query
else {
query = new ScriptQuery(at.location(), r.asScript());
}
query = generateQuery(r, r.value(),
() -> new RangeQuery(r.location(), nameOf(r.value()), valueOf(r.lower()), r.includeLower(),
valueOf(r.upper()), r.includeUpper(), dateFormat(r.value())));
}
return new QueryTranslation(query, aggFilter);
} else {
Expand Down Expand Up @@ -845,6 +818,14 @@ public QueryTranslation translate(Expression exp, boolean onAggs) {

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


protected static Query generateQuery(ScalarFunction sf, Expression field, Supplier<Query> query) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better suggestion for a name here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapQuery/handleQuery/handleScript?

if (field instanceof FieldAttribute) {
return wrapIfNested(query.get(), field);
}
return new ScriptQuery(sf.location(), sf.asScript());
}

protected static Query wrapIfNested(Query query, Expression exp) {
if (exp instanceof FieldAttribute) {
FieldAttribute fa = (FieldAttribute) exp;
Expand Down
46 changes: 46 additions & 0 deletions ...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,22 @@ public void testLikeConstructsNotSupported() {
assertEquals("Scalar function (LTRIM(keyword)) not allowed (yet) as arguments for LIKE", ex.getMessage());
}

public void testTranslateNotExpression_WhereClause_Painless() {
LogicalPlan p = plan("SELECT * FROM test WHERE NOT(POSITION('x', keyword) = 0)");
assertTrue(p instanceof Project);
assertTrue(p.children().get(0) instanceof Filter);
Expression condition = ((Filter) p.children().get(0)).condition();
assertFalse(condition.foldable());
QueryTranslation translation = QueryTranslator.toQuery(condition, false);
assertTrue(translation.query instanceof ScriptQuery);
ScriptQuery sc = (ScriptQuery) translation.query;
assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.not(" +
"InternalSqlScriptUtils.eq(InternalSqlScriptUtils.position(" +
"params.v0,InternalSqlScriptUtils.docValue(doc,params.v1)),params.v2)))",
sc.script().toString());
assertEquals("[{v=x}, {v=keyword}, {v=0}]", sc.script().params().toString());
}

public void testTranslateIsNullExpression_WhereClause() {
LogicalPlan p = plan("SELECT * FROM test WHERE keyword IS NULL");
assertTrue(p instanceof Project);
Expand All @@ -178,6 +194,21 @@ public void testTranslateIsNullExpression_WhereClause() {
eq.asBuilder().toString().replaceAll("\\s+", ""));
}

public void testTranslateIsNullExpression_WhereClause_Painless() {
LogicalPlan p = plan("SELECT * FROM test WHERE POSITION('x', keyword) IS NULL");
assertTrue(p instanceof Project);
assertTrue(p.children().get(0) instanceof Filter);
Expression condition = ((Filter) p.children().get(0)).condition();
assertFalse(condition.foldable());
QueryTranslation translation = QueryTranslator.toQuery(condition, false);
assertTrue(translation.query instanceof ScriptQuery);
ScriptQuery sc = (ScriptQuery) translation.query;
assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.isNull(" +
"InternalSqlScriptUtils.position(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))))",
sc.script().toString());
assertEquals("[{v=x}, {v=keyword}]", sc.script().params().toString());
}

public void testTranslateIsNotNullExpression_WhereClause() {
LogicalPlan p = plan("SELECT * FROM test WHERE keyword IS NOT NULL");
assertTrue(p instanceof Project);
Expand All @@ -191,6 +222,21 @@ public void testTranslateIsNotNullExpression_WhereClause() {
eq.asBuilder().toString().replaceAll("\\s+", ""));
}

public void testTranslateIsNotNullExpression_WhereClause_Painless() {
LogicalPlan p = plan("SELECT * FROM test WHERE POSITION('x', keyword) IS NOT NULL");
assertTrue(p instanceof Project);
assertTrue(p.children().get(0) instanceof Filter);
Expression condition = ((Filter) p.children().get(0)).condition();
assertFalse(condition.foldable());
QueryTranslation translation = QueryTranslator.toQuery(condition, false);
assertTrue(translation.query instanceof ScriptQuery);
ScriptQuery sc = (ScriptQuery) translation.query;
assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.isNotNull(" +
"InternalSqlScriptUtils.position(params.v0,InternalSqlScriptUtils.docValue(doc,params.v1))))",
sc.script().toString());
assertEquals("[{v=x}, {v=keyword}]", sc.script().params().toString());
}

public void testTranslateIsNullExpression_HavingClause_Painless() {
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IS NULL");
assertTrue(p instanceof Project);
Expand Down