Fixed On Fri, Sep 21, 2018 at 7:50 PM Julian Hyde <[email protected]> wrote:
> Thanks for this. However, a minor comment: Since > CompoundQueryExpression.builder is final and you check it is not null in > the constructor, I think you’re being over-cautious checking that it is not > null in other places. > > > On Sep 21, 2018, at 3:26 PM, [email protected] wrote: > > > > Repository: calcite > > Updated Branches: > > refs/heads/master 17e0a0542 -> 2cba81732 > > > > > > [CALCITE-2585] Support NOT Operator in ElasticSearch Adapter > > > > Fix boolean conditions with negation (NOT). The following queries are > now supported: > > ```sql > > select * from elastic where not foo = 1 > > select * from elastic where not foo in (1, 2, 3) > > ``` > > > > Extend existing Expression interface with `not()` method. > > > > Closes apache/calcite#851 > > > > > > Project: http://git-wip-us.apache.org/repos/asf/calcite/repo > > Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/2cba8173 > > Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/2cba8173 > > Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/2cba8173 > > > > Branch: refs/heads/master > > Commit: 2cba817320268f16a707f945d1390d9917188178 > > Parents: 17e0a05 > > Author: Andrei Sereda <[email protected]> > > Authored: Fri Sep 21 11:14:24 2018 -0400 > > Committer: Andrei Sereda <[email protected]> > > Committed: Fri Sep 21 17:37:52 2018 -0400 > > > > ---------------------------------------------------------------------- > > .../elasticsearch/PredicateAnalyzer.java | 57 +++++++++++++++++--- > > .../adapter/elasticsearch/BooleanLogicTest.java | 31 +++++++++++ > > .../elasticsearch/ElasticSearchAdapterTest.java | 28 ++++++++++ > > 3 files changed, 110 insertions(+), 6 deletions(-) > > ---------------------------------------------------------------------- > > > > > > > http://git-wip-us.apache.org/repos/asf/calcite/blob/2cba8173/elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java > > ---------------------------------------------------------------------- > > diff --git > a/elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java > b/elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java > > index a866fe4..1f4ef8d 100644 > > --- > a/elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java > > +++ > b/elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java > > @@ -197,12 +197,15 @@ class PredicateAnalyzer { > > case IS_NOT_NULL: > > case IS_NULL: > > return true; > > - default: // fall through > > } > > - // fall through > > + case PREFIX: // NOT() > > + switch (call.getKind()) { > > + case NOT: > > + return true; > > + } > > + // fall through > > case FUNCTION_ID: > > case FUNCTION_STAR: > > - case PREFIX: // NOT() > > default: > > return false; > > } > > @@ -221,6 +224,8 @@ class PredicateAnalyzer { > > return binary(call); > > case POSTFIX: > > return postfix(call); > > + case PREFIX: > > + return prefix(call); > > case SPECIAL: > > switch (call.getKind()) { > > case CAST: > > @@ -274,6 +279,19 @@ class PredicateAnalyzer { > > } > > } > > > > + private QueryExpression prefix(RexCall call) { > > + Preconditions.checkArgument(call.getKind() == SqlKind.NOT, > > + "Expected %s got %s", SqlKind.NOT, call.getKind()); > > + > > + if (call.getOperands().size() != 1) { > > + String message = String.format(Locale.ROOT, "Unsupported NOT > operator: [%s]", call); > > + throw new PredicateAnalyzerException(message); > > + } > > + > > + QueryExpression expr = (QueryExpression) > call.getOperands().get(0).accept(this); > > + return expr.not(); > > + } > > + > > private QueryExpression postfix(RexCall call) { > > Preconditions.checkArgument(call.getKind() == SqlKind.IS_NULL > > || call.getKind() == SqlKind.IS_NOT_NULL); > > @@ -522,6 +540,11 @@ class PredicateAnalyzer { > > return false; > > } > > > > + /** > > + * Negate {@code this} QueryExpression (not the next one). > > + */ > > + public abstract QueryExpression not(); > > + > > public abstract QueryExpression exists(); > > > > public abstract QueryExpression notExists(); > > @@ -555,6 +578,7 @@ class PredicateAnalyzer { > > throw new PredicateAnalyzerException(message); > > } > > } > > + > > } > > > > /** > > @@ -563,7 +587,7 @@ class PredicateAnalyzer { > > static class CompoundQueryExpression extends QueryExpression { > > > > private final boolean partial; > > - private final BoolQueryBuilder builder = boolQuery(); > > + private final BoolQueryBuilder builder; > > > > public static CompoundQueryExpression or(QueryExpression... > expressions) { > > CompoundQueryExpression bqe = new CompoundQueryExpression(false); > > @@ -590,15 +614,28 @@ class PredicateAnalyzer { > > } > > > > private CompoundQueryExpression(boolean partial) { > > + this(partial, boolQuery()); > > + } > > + > > + private CompoundQueryExpression(boolean partial, BoolQueryBuilder > builder) { > > this.partial = partial; > > + this.builder = Objects.requireNonNull(builder, "builder"); > > } > > > > @Override public boolean isPartial() { > > return partial; > > } > > > > + > > @Override public QueryBuilder builder() { > > - return Objects.requireNonNull(builder); > > + if (builder == null) { > > + throw new IllegalStateException("builder was not set"); > > + } > > + return builder; > > + } > > + > > + @Override public QueryExpression not() { > > + return new CompoundQueryExpression(partial, > QueryBuilders.boolQuery().mustNot(builder())); > > } > > > > @Override public QueryExpression exists() { > > @@ -678,7 +715,15 @@ class PredicateAnalyzer { > > } > > > > @Override public QueryBuilder builder() { > > - return Objects.requireNonNull(builder); > > + if (builder == null) { > > + throw new IllegalStateException("Builder was not initialized"); > > + } > > + return builder; > > + } > > + > > + @Override public QueryExpression not() { > > + builder = boolQuery().mustNot(builder()); > > + return this; > > } > > > > @Override public QueryExpression exists() { > > > > > http://git-wip-us.apache.org/repos/asf/calcite/blob/2cba8173/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/BooleanLogicTest.java > > ---------------------------------------------------------------------- > > diff --git > a/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/BooleanLogicTest.java > b/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/BooleanLogicTest.java > > index c461905..c870234 100644 > > --- > a/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/BooleanLogicTest.java > > +++ > b/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/BooleanLogicTest.java > > @@ -134,6 +134,37 @@ public class BooleanLogicTest { > > + "(c = '0' or (c = 'c' and num = 42))))"); > > } > > > > + /** > > + * Tests negations ({@code NOT} operator). > > + */ > > + @Test > > + public void notExpression() { > > + assertEmpty("select * from view where not a = 'a'"); > > + assertSingle("select * from view where not not a = 'a'"); > > + assertEmpty("select * from view where not not not a = 'a'"); > > + assertSingle("select * from view where not a <> 'a'"); > > + assertSingle("select * from view where not not not a <> 'a'"); > > + assertEmpty("select * from view where not 'a' = a"); > > + assertSingle("select * from view where not 'a' <> a"); > > + assertSingle("select * from view where not a = 'b'"); > > + assertSingle("select * from view where not 'b' = a"); > > + assertEmpty("select * from view where not a in ('a')"); > > + assertEmpty("select * from view where a not in ('a')"); > > + assertSingle("select * from view where not a not in ('a')"); > > + assertEmpty("select * from view where not a not in ('b')"); > > + assertEmpty("select * from view where not not a not in ('a')"); > > + assertSingle("select * from view where not not a not in ('b')"); > > + assertEmpty("select * from view where not a in ('a', 'b')"); > > + assertEmpty("select * from view where a not in ('a', 'b')"); > > + assertEmpty("select * from view where not a not in ('z')"); > > + assertEmpty("select * from view where not a not in ('z')"); > > + assertSingle("select * from view where not a in ('z')"); > > + assertSingle("select * from view where not (not num = 42 or not a > in ('a', 'c'))"); > > + assertEmpty("select * from view where not num > 0"); > > + assertEmpty("select * from view where num = 42 and a not in ('a', > 'c')"); > > + assertSingle("select * from view where not (num > 42 or num < 42 > and num = 42)"); > > + } > > + > > private void assertSingle(String query) { > > CalciteAssert.that() > > .with(newConnectionFactory()) > > > > > http://git-wip-us.apache.org/repos/asf/calcite/blob/2cba8173/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java > > ---------------------------------------------------------------------- > > diff --git > a/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java > b/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java > > index 3d21b03..abcd003 100644 > > --- > a/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java > > +++ > b/elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java > > @@ -494,6 +494,34 @@ public class ElasticSearchAdapterTest { > > } > > > > /** > > + * Testing {@code NOT} operator > > + */ > > + @Test > > + public void notOperator() { > > + // largest zips (states) in mini-zip by pop (sorted) : IL, NY, CA, > MI > > + calciteAssert() > > + .query("select count(*), max(pop) from zips where state not in > ('IL')") > > + .returns("EXPR$0=146; EXPR$1=111396\n"); > > + > > + calciteAssert() > > + .query("select count(*), max(pop) from zips where not state in > ('IL')") > > + .returns("EXPR$0=146; EXPR$1=111396\n"); > > + > > + calciteAssert() > > + .query("select count(*), max(pop) from zips where not state not > in ('IL')") > > + .returns("EXPR$0=3; EXPR$1=112047\n"); > > + > > + calciteAssert() > > + .query("select count(*), max(pop) from zips where state not in > ('IL', 'NY')") > > + .returns("EXPR$0=143; EXPR$1=99568\n"); > > + > > + calciteAssert() > > + .query("select count(*), max(pop) from zips where state not in > ('IL', 'NY', 'CA')") > > + .returns("EXPR$0=140; EXPR$1=84712\n"); > > + > > + } > > + > > + /** > > * Checks > > * <a href=" > https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-metrics-cardinality-aggregation.html > ">Cardinality</a> > > * aggregation {@code approx_count_distinct} > > > >
