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.

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

SQL: Fix edge case: <field> IN (null) #34802

Merged
merged 7 commits into from
Oct 25, 2018
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -78,11 +78,16 @@ public boolean nullable() {

@Override
public boolean foldable() {
return Expressions.foldable(children());
return Expressions.foldable(children()) ||
(Expressions.foldable(list) && list().stream().allMatch(e -> e.dataType() == DataType.NULL));
}

@Override
public Boolean fold() {
if (value.dataType() == DataType.NULL || (list.size() == 1 && list.get(0).dataType() == DataType.NULL)) {
return false;
}

Object foldedLeftValue = value.fold();
Boolean result = false;
for (Expression rightValue : list) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,28 @@
import org.elasticsearch.xpack.sql.tree.Location;
import org.elasticsearch.xpack.sql.type.DataType;

import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;

import static org.elasticsearch.index.query.QueryBuilders.termsQuery;

public class TermsQuery extends LeafQuery {

private final String term;
private final LinkedHashSet<Object> values;
private final Set<Object> values;

public TermsQuery(Location location, String term, List<Expression> values) {
super(location);
this.term = term;
values.removeIf(e -> e.dataType() == DataType.NULL);
this.values = new LinkedHashSet<>(Foldables.valuesOf(values, values.get(0).dataType()));
this.values.removeIf(Objects::isNull);
if (values.isEmpty()) {
this.values = Collections.emptySet();
} else {
this.values = new LinkedHashSet<>(Foldables.valuesOf(values, values.get(0).dataType()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that here all the values in values are of the same type otherwise we would have had the error message about different types.

Copy link
Contributor Author

@matriv matriv Oct 24, 2018

Choose a reason for hiding this comment

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

yes that's true!

}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public class OptimizerTests extends ESTestCase {
private static final Literal FOUR = L(4);
private static final Literal FIVE = L(5);
private static final Literal SIX = L(6);

private static final Literal NULL = L(null);

public static class DummyBooleanExpression extends Expression {

Expand Down Expand Up @@ -323,6 +323,18 @@ public void testConstantFoldingIn_LeftValueNotFoldable() {
assertThat(Foldables.valuesOf(in.list(), DataType.INTEGER), contains(1 ,2 ,3 ,4));
}

public void testConstantFoldingIn_RightValueIsNull() {
In in = new In(EMPTY, getFieldAttribute(), Arrays.asList(NULL, NULL));
Literal result= (Literal) new ConstantFolding().rule(in);
assertEquals(false, result.value());
}

public void testConstantFoldingIn_LeftValueIsNull() {
In in = new In(EMPTY, NULL, Arrays.asList(ONE, TWO, THREE));
Literal result= (Literal) new ConstantFolding().rule(in);
assertEquals(false, result.value());
}

public void testArithmeticFolding() {
assertEquals(10, foldOperator(new Add(EMPTY, L(7), THREE)));
assertEquals(4, foldOperator(new Sub(EMPTY, L(7), THREE)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,16 @@ public void testFoldingToLocalExecWithProject() {
assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
}

public void testFoldingToLocalExecWithProject_FoldableIn() {
PhysicalPlan p = plan("SELECT keyword FROM test WHERE int IN (null, null)");
assertEquals(LocalExec.class, p.getClass());
LocalExec le = (LocalExec) p;
assertEquals(EmptyExecutable.class, le.executable().getClass());
EmptyExecutable ee = (EmptyExecutable) le.executable();
assertEquals(1, ee.output().size());
assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
}

public void testFoldingToLocalExecWithProject_WithOrderAndLimit() {
PhysicalPlan p = plan("SELECT keyword FROM test WHERE 1 = 2 ORDER BY int LIMIT 10");
assertEquals(LocalExec.class, p.getClass());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ public void testTranslateInExpression_WhereClause() throws IOException {
assertEquals("keyword:(bar foo lala)", tq.asBuilder().toQuery(createShardContext()).toString());
}

public void testTranslateInExpression_WhereClauseAndNullHAndling() throws IOException {
public void testTranslateInExpression_WhereClauseAndNullHandling() throws IOException {
LogicalPlan p = plan("SELECT * FROM test WHERE keyword IN ('foo', null, 'lala', null, 'foo', concat('la', 'la'))");
assertTrue(p instanceof Project);
assertTrue(p.children().get(0) instanceof Filter);
Expand Down