You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Julian Hyde <jh...@apache.org> on 2018/09/21 23:50:42 UTC
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@apache.org 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 <25...@users.noreply.github.com>
> Authored: Fri Sep 21 11:14:24 2018 -0400
> Committer: Andrei Sereda <25...@users.noreply.github.com>
> 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}
>
Re: calcite git commit: [CALCITE-2585] Support NOT Operator in
ElasticSearch Adapter
Posted by Andrei Sereda <an...@sereda.cc>.
Fixed
On Fri, Sep 21, 2018 at 7:50 PM Julian Hyde <jh...@apache.org> 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, sereda@apache.org 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 <25...@users.noreply.github.com>
> > Authored: Fri Sep 21 11:14:24 2018 -0400
> > Committer: Andrei Sereda <25...@users.noreply.github.com>
> > 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}
> >
>
>