From c4cc3d5a5b7e8ce04ad5e0e9bbda3790459fa812 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Fri, 26 Oct 2018 21:34:01 +0300 Subject: [PATCH] SQL: Handle aggregation for null group (#34916) When dealing with a null group, the associated metric aggs need to return null as well Fix #34896 (cherry picked from commit 55d48e4cc114a995c30645efd901419946c9a66f) --- .../xpack/sql/execution/search/Querier.java | 3 +- .../search/extractor/MetricAggExtractor.java | 37 +++++++++++++++++++ x-pack/qa/sql/src/main/resources/agg.csv-spec | 15 ++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java index 5be361179b326..e3c604feda076 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java @@ -253,8 +253,9 @@ protected List initBucketExtractors(SearchResponse response) { List refs = query.columns(); List exts = new ArrayList<>(refs.size()); + ConstantExtractor totalCount = new ConstantExtractor(response.getHits().getTotalHits()); for (FieldExtraction ref : refs) { - exts.add(createExtractor(ref, new ConstantExtractor(response.getHits().getTotalHits()))); + exts.add(createExtractor(ref, totalCount)); } return exts; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/MetricAggExtractor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/MetricAggExtractor.java index 307b6a7cab681..b95d5ab4696bd 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/MetricAggExtractor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/MetricAggExtractor.java @@ -9,7 +9,12 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket; +import org.elasticsearch.search.aggregations.matrix.stats.MatrixStats; import org.elasticsearch.search.aggregations.metrics.InternalNumericMetricsAggregation; +import org.elasticsearch.search.aggregations.metrics.InternalNumericMetricsAggregation.SingleValue; +import org.elasticsearch.search.aggregations.metrics.percentiles.PercentileRanks; +import org.elasticsearch.search.aggregations.metrics.percentiles.Percentiles; +import org.elasticsearch.search.aggregations.metrics.stats.InternalStats; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; import org.elasticsearch.xpack.sql.querydsl.agg.Aggs; @@ -67,6 +72,11 @@ public Object extract(Bucket bucket) { if (agg == null) { throw new SqlIllegalArgumentException("Cannot find an aggregation named {}", name); } + + if (!containsValues(agg)) { + return null; + } + if (agg instanceof InternalNumericMetricsAggregation.MultiValue) { //TODO: need to investigate when this can be not-null //if (innerKey == null) { @@ -79,6 +89,33 @@ public Object extract(Bucket bucket) { return innerKey != null && v instanceof Map ? ((Map) v).get(innerKey) : v; } + /** + * Check if the given aggregate has been executed and has computed values + * or not (the bucket is null). + * + * Waiting on https://github.com/elastic/elasticsearch/issues/34903 + */ + private static boolean containsValues(InternalAggregation agg) { + // Stats & ExtendedStats + if (agg instanceof InternalStats) { + return ((InternalStats) agg).getCount() != 0; + } + if (agg instanceof MatrixStats) { + return ((MatrixStats) agg).getDocCount() != 0; + } + // sum returns 0 even for null; since that's a common case, we return it as such + if (agg instanceof SingleValue) { + return Double.isFinite(((SingleValue) agg).value()); + } + if (agg instanceof PercentileRanks) { + return Double.isFinite(((PercentileRanks) agg).percent(0)); + } + if (agg instanceof Percentiles) { + return Double.isFinite(((Percentiles) agg).percentile(0)); + } + return true; + } + @Override public int hashCode() { return Objects.hash(name, property, innerKey); diff --git a/x-pack/qa/sql/src/main/resources/agg.csv-spec b/x-pack/qa/sql/src/main/resources/agg.csv-spec index db5ed60997679..17ed219687ae9 100644 --- a/x-pack/qa/sql/src/main/resources/agg.csv-spec +++ b/x-pack/qa/sql/src/main/resources/agg.csv-spec @@ -121,4 +121,19 @@ gender:s | k:d | s:d null |2.2215791166941923 |-0.03373126000214023 F |1.7873117044424276 |0.05504995122217512 M |2.280646181070106 |0.44302407229580243 +; + +nullAggs +SELECT MAX(languages) max, MIN(languages) min, SUM(languages) sum, AVG(languages) avg, + PERCENTILE(languages, 80) percent, PERCENTILE_RANK(languages, 3) percent_rank, + KURTOSIS(languages) kurtosis, SKEWNESS(languages) skewness + FROM test_emp GROUP BY languages ORDER BY languages ASC LIMIT 5; + + max:bt | min:bt | sum:bt | avg:bt | percent:d | percent_rank:d| kurtosis:d | skewness:d +---------------+---------------+---------------+---------------+---------------+---------------+---------------+--------------- +null |null |null |null |null |null |null |null +1 |1 |15 |1 |1.0 |100.0 |NaN |NaN +2 |2 |38 |2 |2.0 |100.0 |NaN |NaN +3 |3 |51 |3 |3.0 |100.0 |NaN |NaN +4 |4 |72 |4 |4.0 |0.0 |NaN |NaN ; \ No newline at end of file