osdir.com

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: calcite git commit: [CALCITE-2585] Support NOT Operator in ElasticSearch Adapter


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, sereda@xxxxxxxxxx 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 <25229979+asereda-gs@xxxxxxxxxxxxxxxxxxxxxxxx>
> Authored: Fri Sep 21 11:14:24 2018 -0400
> Committer: Andrei Sereda <25229979+asereda-gs@xxxxxxxxxxxxxxxxxxxxxxxx>
> 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}
>