Skip to content

Commit

Permalink
{S,E}QL: Fix optimization of NotEquals in conjunctions (#65331) (#6…
Browse files Browse the repository at this point in the history 
…5449)

* Fix the `CombineBinaryComparisons` optimizer rule, so that semantic
equality taken into account during the optimization of `NotEquals`

Examples that previously removed the `NotEquals` expressions (leading
to incorrect results):

```
double >= 10 AND integer != 9
-->  double >= 10

keyword != '2021' AND datetime >= '2020-01-01T00:00:00'
--> datetime >= '2020-01-01T00:00:00'
```

With the fix, expressions like the above will not be touched.
`NotEquals` will only be eliminated from the `AND` expression if the
left side of the `NotEquals` `semanticEquals()` to the left side
of the other expressions within the conjunction (comparisons against
the same field/expression).

* Unit tests and integration tests

Close #65322
(cherry-picked from 8b2b7fa)
  • Loading branch information
Andras Palinkas committed Nov 24, 2020
1 parent be2ed11 commit 7f7e938
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 32 deletions.
44 changes: 23 additions & 21 deletions x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1008,27 +1008,29 @@ private static boolean notEqualsIsRemovableFromConjunction(NotEquals notEquals,
for (int i = 0; i < bcs.size(); i ++) {
BinaryComparison bc = bcs.get(i);

if (bc instanceof LessThan || bc instanceof LessThanOrEqual) {
comp = bc.right().foldable() ? BinaryComparison.compare(neqVal, bc.right().fold()) : null;
if (comp != null) {
if (comp >= 0) {
if (comp == 0 && bc instanceof LessThanOrEqual) { // a != 2 AND a <= 2 -> a < 2
bcs.set(i, new LessThan(bc.source(), bc.left(), bc.right(), bc.zoneId()));
} // else : comp > 0 (a != 2 AND a </<= 1 -> a </<= 1), or == 0 && bc i.of "<" (a != 2 AND a < 2 -> a < 2)
return true;
} // else: comp < 0 : a != 2 AND a </<= 3 -> nop
} // else: non-comparable, nop
} else if (bc instanceof GreaterThan || bc instanceof GreaterThanOrEqual) {
comp = bc.right().foldable() ? BinaryComparison.compare(neqVal, bc.right().fold()) : null;
if (comp != null) {
if (comp <= 0) {
if (comp == 0 && bc instanceof GreaterThanOrEqual) { // a != 2 AND a >= 2 -> a > 2
bcs.set(i, new GreaterThan(bc.source(), bc.left(), bc.right(), bc.zoneId()));
} // else: comp < 0 (a != 2 AND a >/>= 3 -> a >/>= 3), or == 0 && bc i.of ">" (a != 2 AND a > 2 -> a > 2)
return true;
} // else: comp > 0 : a != 2 AND a >/>= 1 -> nop
} // else: non-comparable, nop
} // else: other non-relevant type
if (notEquals.left().semanticEquals(bc.left())) {
if (bc instanceof LessThan || bc instanceof LessThanOrEqual) {
comp = bc.right().foldable() ? BinaryComparison.compare(neqVal, bc.right().fold()) : null;
if (comp != null) {
if (comp >= 0) {
if (comp == 0 && bc instanceof LessThanOrEqual) { // a != 2 AND a <= 2 -> a < 2
bcs.set(i, new LessThan(bc.source(), bc.left(), bc.right(), bc.zoneId()));
} // else : comp > 0 (a != 2 AND a </<= 1 -> a </<= 1), or == 0 && bc i.of "<" (a != 2 AND a < 2 -> a < 2)
return true;
} // else: comp < 0 : a != 2 AND a </<= 3 -> nop
} // else: non-comparable, nop
} else if (bc instanceof GreaterThan || bc instanceof GreaterThanOrEqual) {
comp = bc.right().foldable() ? BinaryComparison.compare(neqVal, bc.right().fold()) : null;
if (comp != null) {
if (comp <= 0) {
if (comp == 0 && bc instanceof GreaterThanOrEqual) { // a != 2 AND a >= 2 -> a > 2
bcs.set(i, new GreaterThan(bc.source(), bc.left(), bc.right(), bc.zoneId()));
} // else: comp < 0 (a != 2 AND a >/>= 3 -> a >/>= 3), or == 0 && bc i.of ">" (a != 2 AND a > 2 -> a > 2)
return true;
} // else: comp > 0 : a != 2 AND a >/>= 1 -> nop
} // else: non-comparable, nop
} // else: other non-relevant type
}
}

return false;
Expand Down
52 changes: 41 additions & 11 deletions x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@
import org.elasticsearch.xpack.ql.util.StringUtils;

import java.time.ZoneId;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;

import static java.util.Arrays.asList;
import static java.util.Collections.emptyMap;
import static org.elasticsearch.xpack.ql.TestUtils.equalsOf;
import static org.elasticsearch.xpack.ql.TestUtils.greaterThanOf;
Expand All @@ -68,7 +68,9 @@
import static org.elasticsearch.xpack.ql.expression.Literal.TRUE;
import static org.elasticsearch.xpack.ql.tree.Source.EMPTY;
import static org.elasticsearch.xpack.ql.type.DataTypes.BOOLEAN;
import static org.elasticsearch.xpack.ql.type.DataTypes.DOUBLE;
import static org.elasticsearch.xpack.ql.type.DataTypes.INTEGER;
import static org.elasticsearch.xpack.ql.type.DataTypes.KEYWORD;

public class OptimizerRulesTests extends ESTestCase {

Expand Down Expand Up @@ -135,7 +137,11 @@ private static FieldAttribute getFieldAttribute() {
}

private static FieldAttribute getFieldAttribute(String name) {
return new FieldAttribute(EMPTY, name, new EsField(name + "f", INTEGER, emptyMap(), true));
return getFieldAttribute(name, INTEGER);
}

private static FieldAttribute getFieldAttribute(String name, DataType dataType) {
return new FieldAttribute(EMPTY, name, new EsField(name + "f", dataType, emptyMap(), true));
}

//
Expand Down Expand Up @@ -289,7 +295,7 @@ public void testBoolEqualsSimplificationOnFields() {

FieldAttribute field = getFieldAttribute();

List<? extends BinaryComparison> comparisons = Arrays.asList(
List<? extends BinaryComparison> comparisons = asList(
new Equals(EMPTY, field, TRUE),
new Equals(EMPTY, field, FALSE),
notEqualsOf(field, TRUE),
Expand Down Expand Up @@ -504,7 +510,7 @@ public void testCombineMultipleComparisonsIntoRange() {
LessThan blt4 = new LessThan(EMPTY, fb, FOUR, zoneId);
LessThan clt4 = new LessThan(EMPTY, fc, FOUR, zoneId);

Expression inputAnd = Predicates.combineAnd(Arrays.asList(agt1, alt3, bgt2, blt4, clt4));
Expression inputAnd = Predicates.combineAnd(asList(agt1, alt3, bgt2, blt4, clt4));

CombineBinaryComparisons rule = new CombineBinaryComparisons();
Expression outputAnd = rule.rule(inputAnd);
Expand All @@ -513,7 +519,7 @@ public void testCombineMultipleComparisonsIntoRange() {
Range bgt2lt4 = new Range(EMPTY, fb, TWO, false, FOUR, false, zoneId);

// The actual outcome is (c < 4) AND (1 < a < 3) AND (2 < b < 4), due to the way the Expression types are combined in the Optimizer
Expression expectedAnd = Predicates.combineAnd(Arrays.asList(clt4, agt1lt3, bgt2lt4));
Expression expectedAnd = Predicates.combineAnd(asList(clt4, agt1lt3, bgt2lt4));

assertTrue(outputAnd.semanticEquals(expectedAnd));
}
Expand Down Expand Up @@ -1015,7 +1021,31 @@ public void testRangesOverlappingNoLowerBoundary() {
Expression exp = rule.rule(or);
assertEquals(r2, exp);
}


public void testBinaryComparisonAndOutOfRangeNotEqualsDifferentFields() {
FieldAttribute doubleOne = getFieldAttribute("double", DOUBLE);
FieldAttribute doubleTwo = getFieldAttribute("double2", DOUBLE);
FieldAttribute intOne = getFieldAttribute("int", INTEGER);
FieldAttribute datetimeOne = getFieldAttribute("datetime", INTEGER);
FieldAttribute keywordOne = getFieldAttribute("keyword", KEYWORD);
FieldAttribute keywordTwo = getFieldAttribute("keyword2", KEYWORD);

List<And> testCases = asList(
// double > 10 AND integer != -10
new And(EMPTY, greaterThanOf(doubleOne, L(10)), notEqualsOf(intOne, L(-10))),
// keyword > '5' AND keyword2 != '48'
new And(EMPTY, greaterThanOf(keywordOne, L("5")), notEqualsOf(keywordTwo, L("48"))),
// keyword != '2021' AND datetime <= '2020-12-04T17:48:22.954240Z'
new And(EMPTY, notEqualsOf(keywordOne, L("2021")), lessThanOrEqualOf(datetimeOne, L("2020-12-04T17:48:22.954240Z"))),
// double > 10.1 AND double2 != -10.1
new And(EMPTY, greaterThanOf(doubleOne, L(10.1d)), notEqualsOf(doubleTwo, L(-10.1d))));

for (And and : testCases) {
CombineBinaryComparisons rule = new CombineBinaryComparisons();
Expression exp = rule.rule(and);
assertEquals("Rule should not have transformed [" + and.nodeString() + "]", and, exp);
}
}

// Equals & NullEquals

Expand Down Expand Up @@ -1188,7 +1218,7 @@ public void testPropagateEquals_VarEq2AndVarLt3AndVarGt1AndVarNeq4() {
NotEquals neq = notEqualsOf(fa, FOUR);

PropagateEquals rule = new PropagateEquals();
Expression and = Predicates.combineAnd(Arrays.asList(eq, lt, gt, neq));
Expression and = Predicates.combineAnd(asList(eq, lt, gt, neq));
Expression exp = rule.rule(and);
assertEquals(eq, exp);
}
Expand All @@ -1202,7 +1232,7 @@ public void testPropagateEquals_VarEq2AndVarRangeGt1Lt3AndVarGt0AndVarNeq4() {
NotEquals neq = notEqualsOf(fa, FOUR);

PropagateEquals rule = new PropagateEquals();
Expression and = Predicates.combineAnd(Arrays.asList(eq, range, gt, neq));
Expression and = Predicates.combineAnd(asList(eq, range, gt, neq));
Expression exp = rule.rule(and);
assertEquals(eq, exp);
}
Expand Down Expand Up @@ -1331,15 +1361,15 @@ public void testPropagateEquals_VarEq2OrVarRangeGt3Lt4OrVarGt2OrVarNe2() {
NotEquals neq = notEqualsOf(fa, TWO);

PropagateEquals rule = new PropagateEquals();
Expression exp = rule.rule(Predicates.combineOr(Arrays.asList(eq, range, neq, gt)));
Expression exp = rule.rule(Predicates.combineOr(asList(eq, range, neq, gt)));
assertEquals(TRUE, exp);
}

//
// Like / Regex
//
public void testMatchAllLikeToExist() throws Exception {
for (String s : Arrays.asList("%", "%%", "%%%")) {
for (String s : asList("%", "%%", "%%%")) {
LikePattern pattern = new LikePattern(s, (char) 0);
FieldAttribute fa = getFieldAttribute();
Like l = new Like(EMPTY, fa, pattern);
Expand All @@ -1361,7 +1391,7 @@ public void testMatchAllRLikeToExist() throws Exception {
}

public void testExactMatchLike() throws Exception {
for (String s : Arrays.asList("ab", "ab0%", "ab0_c")) {
for (String s : asList("ab", "ab0%", "ab0_c")) {
LikePattern pattern = new LikePattern(s, '0');
FieldAttribute fa = getFieldAttribute();
Like l = new Like(EMPTY, fa, pattern);
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugin/sql/qa/server/src/main/resources/filter.sql-spec
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ whereFieldAndComparison
// tag::whereFieldAndComparison
SELECT last_name l FROM "test_emp" WHERE emp_no > 10000 AND emp_no < 10005 ORDER BY emp_no LIMIT 5;
// end::whereFieldAndComparison
whereGreaterThanAndNotEqualityOnDifferentFields
SELECT last_name l FROM "test_emp" WHERE salary >= 50000 AND emp_no != 10002 ORDER BY emp_no LIMIT 5;
whereFieldOrComparison
// tag::whereFieldOrComparison
SELECT last_name l FROM "test_emp" WHERE emp_no < 10003 OR emp_no = 10005 ORDER BY emp_no LIMIT 5;
Expand Down

0 comments on commit 7f7e938

Please sign in to comment.