From c50e6efd1f5b069686b8dc6c6ccacbaf5b76f2e7 Mon Sep 17 00:00:00 2001 From: chloe-zh Date: Wed, 6 Jan 2021 14:11:12 -0800 Subject: [PATCH] addressed comments --- .../expression/aggregation/Aggregator.java | 20 +++++++++++++++++-- .../expression/aggregation/AvgAggregator.java | 10 +++------- .../aggregation/CountAggregator.java | 8 ++------ .../expression/aggregation/MaxAggregator.java | 8 ++------ .../expression/aggregation/MinAggregator.java | 8 ++------ .../aggregation/NamedAggregator.java | 5 +++-- .../expression/aggregation/SumAggregator.java | 10 +++------- .../resources/correctness/queries/filter.txt | 1 + 8 files changed, 34 insertions(+), 36 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/Aggregator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/Aggregator.java index 6f170e1299..ce9dd937f3 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/Aggregator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/Aggregator.java @@ -60,13 +60,29 @@ public abstract class Aggregator public abstract S create(); /** - * Iterate on the {@link BindingTuple}. + * Iterate on {@link ExprValue}. + * @param value {@link ExprValue} + * @param state {@link AggregationState} + * @return {@link AggregationState} + */ + protected abstract S iterate(ExprValue value, S state); + + /** + * Let the aggregator iterate on the {@link BindingTuple} + * To filter out ExprValues that are missing, null or cannot satisfy {@link #condition} + * Before the specific aggregator iterating ExprValue in the tuple. * * @param tuple {@link BindingTuple} * @param state {@link AggregationState} * @return {@link AggregationState} */ - public abstract S iterate(BindingTuple tuple, S state); + public S iterate(BindingTuple tuple, S state) { + ExprValue value = getArguments().get(0).valueOf(tuple); + if (value.isNull() || value.isMissing() || !conditionValue(tuple)) { + return state; + } + return iterate(value, state); + } @Override public ExprValue valueOf(Environment valueEnv) { diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/AvgAggregator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/AvgAggregator.java index 17f5ce0e9f..0e1d07c790 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/AvgAggregator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/AvgAggregator.java @@ -43,13 +43,9 @@ public AvgState create() { } @Override - public AvgState iterate(BindingTuple tuple, AvgState state) { - Expression expression = getArguments().get(0); - ExprValue value = expression.valueOf(tuple); - if (!(value.isNull() || value.isMissing()) && conditionValue(tuple)) { - state.count++; - state.total += ExprValueUtils.getDoubleValue(value); - } + protected AvgState iterate(ExprValue value, AvgState state) { + state.count++; + state.total += ExprValueUtils.getDoubleValue(value); return state; } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/CountAggregator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/CountAggregator.java index 17c2146921..596f3ae0b2 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/CountAggregator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/CountAggregator.java @@ -39,12 +39,8 @@ public CountAggregator.CountState create() { } @Override - public CountState iterate(BindingTuple tuple, CountState state) { - Expression expression = getArguments().get(0); - ExprValue value = expression.valueOf(tuple); - if (!(value.isNull() || value.isMissing()) && conditionValue(tuple)) { - state.count++; - } + protected CountState iterate(ExprValue value, CountState state) { + state.count++; return state; } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/MaxAggregator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/MaxAggregator.java index 07829e1043..4a4fce7896 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/MaxAggregator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/MaxAggregator.java @@ -37,12 +37,8 @@ public MaxState create() { } @Override - public MaxState iterate(BindingTuple tuple, MaxState state) { - Expression expression = getArguments().get(0); - ExprValue value = expression.valueOf(tuple); - if (!(value.isNull() || value.isMissing()) && conditionValue(tuple)) { - state.max(value); - } + protected MaxState iterate(ExprValue value, MaxState state) { + state.max(value); return state; } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/MinAggregator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/MinAggregator.java index 99f9f9abae..e03e75dcb7 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/MinAggregator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/MinAggregator.java @@ -42,12 +42,8 @@ public MinState create() { } @Override - public MinState iterate(BindingTuple tuple, MinState state) { - Expression expression = getArguments().get(0); - ExprValue value = expression.valueOf(tuple); - if (!(value.isNull() || value.isMissing()) && conditionValue(tuple)) { - state.min(value); - } + protected MinState iterate(ExprValue value, MinState state) { + state.min(value); return state; } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/NamedAggregator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/NamedAggregator.java index c21beba10d..9d92d4f2e5 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/NamedAggregator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/NamedAggregator.java @@ -17,6 +17,7 @@ package com.amazon.opendistroforelasticsearch.sql.expression.aggregation; +import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValue; import com.amazon.opendistroforelasticsearch.sql.expression.ExpressionNodeVisitor; import com.amazon.opendistroforelasticsearch.sql.storage.bindingtuple.BindingTuple; import com.google.common.base.Strings; @@ -63,8 +64,8 @@ public AggregationState create() { } @Override - public AggregationState iterate(BindingTuple tuple, AggregationState state) { - return delegated.iterate(tuple, state); + protected AggregationState iterate(ExprValue value, AggregationState state) { + return delegated.iterate(value, state); } /** diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/SumAggregator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/SumAggregator.java index 11a2ae5516..f3cd990257 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/SumAggregator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/SumAggregator.java @@ -53,13 +53,9 @@ public SumState create() { } @Override - public SumState iterate(BindingTuple tuple, SumState state) { - Expression expression = getArguments().get(0); - ExprValue value = expression.valueOf(tuple); - if (!(value.isNull() || value.isMissing()) && conditionValue(tuple)) { - state.isEmptyCollection = false; - state.add(value); - } + protected SumState iterate(ExprValue value, SumState state) { + state.isEmptyCollection = false; + state.add(value); return state; } diff --git a/integ-test/src/test/resources/correctness/queries/filter.txt b/integ-test/src/test/resources/correctness/queries/filter.txt index b8d468275b..690defb8ae 100644 --- a/integ-test/src/test/resources/correctness/queries/filter.txt +++ b/integ-test/src/test/resources/correctness/queries/filter.txt @@ -2,4 +2,5 @@ SELECT AVG(AvgTicketPrice) FILTER(WHERE Carrier = 'Kibana Airlines') AS filtered SELECT AVG(AvgTicketPrice) FILTER(WHERE Carrier = 'Kibana Airlines') AS filtered FROM kibana_sample_data_flights GROUP BY Origin ORDER BY Origin SELECT AVG(AvgTicketPrice + 1) FILTER(WHERE Carrier = 'Kibana Airlines') AS filtered FROM kibana_sample_data_flights SELECT AVG(AvgTicketPrice) FILTER(WHERE Carrier = 'Kibana Airlines') / 2 AS filtered FROM kibana_sample_data_flights +SELECT AVG(AvgTicketPrice) FILTER(WHERE ABS(AvgTicketPrice) < 10000) AS filtered FROM kibana_sample_data_flights SELECT AVG(AvgTicketPrice) AS unfiltered, AVG(AvgTicketPrice) FILTER(WHERE Carrier = 'Kibana Airlines') AS filtered1, AVG(AvgTicketPrice) FILTER(WHERE Carrier = 'ES-Air') AS filtered2 FROM kibana_sample_data_flights WHERE DestWeather = 'Sunny' \ No newline at end of file