Skip to content

SQL: Fix negation of equals comparison. #34680

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
*/
package org.elasticsearch.xpack.sql.expression.predicate.logical;

import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.expression.Expressions;
import org.elasticsearch.xpack.sql.expression.function.scalar.UnaryScalarFunction;
import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe;
import org.elasticsearch.xpack.sql.expression.gen.pipeline.UnaryPipe;
import org.elasticsearch.xpack.sql.expression.gen.script.Params;
import org.elasticsearch.xpack.sql.expression.gen.script.ParamsBuilder;
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate;
import org.elasticsearch.xpack.sql.expression.predicate.BinaryOperator.Negateable;
import org.elasticsearch.xpack.sql.tree.Location;
Expand All @@ -18,6 +20,8 @@

import java.util.Objects;

import static org.elasticsearch.xpack.sql.expression.gen.script.Scripts.nullSafeFilter;

public class Not extends UnaryScalarFunction {

public Not(Location location, Expression child) {
Expand Down Expand Up @@ -50,12 +54,14 @@ public Object fold() {

@Override
protected Pipe makePipe() {
throw new SqlIllegalArgumentException("Not supported yet");
return new UnaryPipe(location(), this, Expressions.pipe(field()), new NotLogicProcessor());
}

@Override
public ScriptTemplate asScript() {
throw new SqlIllegalArgumentException("Not supported yet");
ScriptTemplate fieldScript = asScript(field());
Params params = ParamsBuilder.paramsBuilder().script(fieldScript.params()).build();
return new ScriptTemplate("!(" + nullSafeFilter(fieldScript) + ")", params, DataType.BOOLEAN);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* 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.sql.expression.predicate.logical;

import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.xpack.sql.expression.gen.processor.Processor;

import java.io.IOException;

public class NotLogicProcessor implements Processor {

private static final String NAME = "not";

@Override
public String getWriteableName() {
return NAME;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
}

@Override
public Boolean process(Object input) {
if (input == null) {
return null;
}
return !((Boolean) input);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,14 @@ static class Nots extends ExpressionTranslator<Not> {
@Override
protected QueryTranslation asQuery(Not not, boolean onAggs) {
QueryTranslation translation = toQuery(not.field(), onAggs);
if (translation.query == null) {
ScriptTemplate notScript = not.asScript();
AggFilter aggFilter = null;
if (translation.aggFilter != null) {
aggFilter = new AggFilter(translation.aggFilter.name(), notScript);
}
return new QueryTranslation(new ScriptQuery(not.location(), notScript), aggFilter);
}
return new QueryTranslation(not(translation.query), translation.aggFilter);
}
}
Expand Down Expand Up @@ -515,16 +523,15 @@ protected QueryTranslation asQuery(BinaryComparison bc, boolean onAggs) {
//
// Agg context means HAVING -> PipelineAggs
//
ScriptTemplate script = bc.asScript();
if (onAggs) {
aggFilter = new AggFilter(at.id().toString(), script);
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(), script);
query = new ScriptQuery(at.location(), bc.asScript());
}
}
return new QueryTranslation(query, aggFilter);
Expand Down Expand Up @@ -585,11 +592,10 @@ protected QueryTranslation asQuery(Range r, boolean onAggs) {
//
// Agg context means HAVING -> PipelineAggs
//
ScriptTemplate script = r.asScript();
Attribute at = ((NamedExpression) e).toAttribute();

if (onAggs) {
aggFilter = new AggFilter(at.id().toString(), script);
aggFilter = new AggFilter(at.id().toString(), r.asScript());
} else {
// typical range; no scripting involved
if (at instanceof FieldAttribute) {
Expand All @@ -599,7 +605,7 @@ protected QueryTranslation asQuery(Range r, boolean onAggs) {
}
// scripted query
else {
query = new ScriptQuery(at.location(), script);
query = new ScriptQuery(at.location(), r.asScript());
}
}
return new QueryTranslation(query, aggFilter);
Expand Down Expand Up @@ -759,4 +765,4 @@ protected static Query wrapIfNested(Query query, Expression exp) {
return query;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ public NotQuery(Location location, Query child) {
this.child = child;
}

// For testing
public Query childQuery() {
return child;
}

@Override
public boolean containsNestedField(String path, String field) {
return child.containsNestedField(path, field);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.elasticsearch.xpack.sql.planner.QueryTranslator.QueryTranslation;
import org.elasticsearch.xpack.sql.querydsl.query.Query;
import org.elasticsearch.xpack.sql.querydsl.query.RangeQuery;
import org.elasticsearch.xpack.sql.querydsl.query.ScriptQuery;
import org.elasticsearch.xpack.sql.querydsl.query.TermQuery;
import org.elasticsearch.xpack.sql.type.EsField;
import org.elasticsearch.xpack.sql.type.TypesTests;
Expand All @@ -28,6 +29,8 @@
import java.util.Map;
import java.util.TimeZone;

import static org.hamcrest.Matchers.startsWith;

public class QueryTranslatorTests extends ESTestCase {

private SqlParser parser;
Expand Down Expand Up @@ -149,4 +152,18 @@ public void testLikeConstructsNotSupported() {
SqlIllegalArgumentException ex = expectThrows(SqlIllegalArgumentException.class, () -> QueryTranslator.toQuery(condition, false));
assertEquals("Scalar function (LTRIM(keyword)) not allowed (yet) as arguments for LIKE", ex.getMessage());
}
}

public void testTranslateNotExpression_HavingClause_Painless() {
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING NOT max(int) = 10");
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, true);
assertTrue(translation.query instanceof ScriptQuery);
ScriptQuery sq = (ScriptQuery) translation.query;
assertEquals("InternalSqlScriptUtils.nullSafeFilter(!(InternalSqlScriptUtils.nullSafeFilter(" +
"InternalSqlScriptUtils.eq(params.a0,params.v0))))", sq.script().toString());
assertThat(sq.script().params().toString(), startsWith("[{a=MAX(int){a->"));
}
}
2 changes: 2 additions & 0 deletions x-pack/qa/sql/src/main/resources/agg.sql-spec
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ aggCountAndHaving
SELECT gender g, COUNT(*) c FROM "test_emp" GROUP BY g HAVING COUNT(*) > 10 ORDER BY gender;
aggCountAndHavingEquality
SELECT gender g, COUNT(*) c FROM "test_emp" GROUP BY g HAVING COUNT(*) = 10 ORDER BY gender;
aggCountAndHavingNegateEquality
SELECT gender g, COUNT(*) c FROM "test_emp" GROUP BY g HAVING NOT COUNT(*) = 10 ORDER BY gender;
aggCountOnColumnAndHaving
SELECT gender g, COUNT(gender) c FROM "test_emp" GROUP BY g HAVING COUNT(gender) > 10 ORDER BY gender;
aggCountOnColumnAndWildcardAndHaving
Expand Down
19 changes: 19 additions & 0 deletions x-pack/qa/sql/src/main/resources/select.csv-spec
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Cannot be in sql-spec as the column name in H2 is translated to "!="
negateEqualsInSelectClause
SELECT NOT MONTH(birth_date) = 4, MONTH(birth_date), emp_no FROM "test_emp" ORDER BY 2;

NOT((MONTH_OF_YEAR(birth_date [UTC])) == 5)|birth_date
false |1958-05-21T00:00:00Z
true |1959-10-01T00:00:00Z
true |1960-07-20T00:00:00Z
null |null
null |null
null |null
null |null
null |null
null |null
null |null
null |null
null |null
null |null
;