diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java index 635a0a6015c..079a6b123f0 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java @@ -26,15 +26,13 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range; import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSource.Numeric; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; -import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; -import org.elasticsearch.search.internal.SearchContext; import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.Objects; +import java.util.function.Function; public abstract class AbstractRangeBuilder, R extends Range> extends ValuesSourceAggregationBuilder { @@ -63,10 +61,10 @@ public abstract class AbstractRangeBuilder config) { + protected Range[] processRanges(Function rangeProcessor) { Range[] ranges = new Range[this.ranges.size()]; for (int i = 0; i < ranges.length; i++) { - ranges[i] = this.ranges.get(i).process(config.format(), context); + ranges[i] = rangeProcessor.apply(this.ranges.get(i)); } sortRanges(ranges); return ranges; diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java index f7b234c783d..7e6da614f7e 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java @@ -22,6 +22,7 @@ package org.elasticsearch.search.aggregations.bucket.range; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; import org.elasticsearch.search.aggregations.AggregatorFactory; @@ -218,7 +219,7 @@ public class DateRangeAggregationBuilder extends AbstractRangeBuilder parent, Builder subFactoriesBuilder) throws IOException { // We need to call processRanges here so they are parsed and we know whether `now` has been used before we make // the decision of whether to cache the request - RangeAggregator.Range[] ranges = processRanges(context, config); + RangeAggregator.Range[] ranges = processRanges(range -> { + DocValueFormat parser = config.format(); + assert parser != null; + double from = range.getFrom(); + double to = range.getTo(); + String fromAsString = range.getFromAsString(); + String toAsString = range.getToAsString(); + if (fromAsString != null) { + from = parser.parseDouble(fromAsString, false, context.getQueryShardContext()::nowInMillis); + } else if (Double.isFinite(from)) { + // from/to provided as double should be converted to string and parsed regardless to support + // different formats like `epoch_millis` vs. `epoch_second` with numeric input + from = parser.parseDouble(Long.toString((long) from), false, context.getQueryShardContext()::nowInMillis); + } + if (toAsString != null) { + to = parser.parseDouble(toAsString, false, context.getQueryShardContext()::nowInMillis); + } else if (Double.isFinite(to)) { + to = parser.parseDouble(Long.toString((long) to), false, context.getQueryShardContext()::nowInMillis); + } + return new RangeAggregator.Range(range.getKey(), from, fromAsString, to, toAsString); + }); if (ranges.length == 0) { throw new IllegalArgumentException("No [ranges] specified for the [" + this.getName() + "] aggregation"); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java index aaa2b857ecb..ff28f51d8b0 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java @@ -22,6 +22,7 @@ package org.elasticsearch.search.aggregations.bucket.range; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; import org.elasticsearch.search.aggregations.AggregatorFactory; @@ -138,7 +139,19 @@ public class RangeAggregationBuilder extends AbstractRangeBuilder config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { // We need to call processRanges here so they are parsed before we make the decision of whether to cache the request - Range[] ranges = processRanges(context, config); + Range[] ranges = processRanges(range -> { + DocValueFormat parser = config.format(); + assert parser != null; + Double from = range.from; + Double to = range.to; + if (range.fromAsStr != null) { + from = parser.parseDouble(range.fromAsStr, false, context.getQueryShardContext()::nowInMillis); + } + if (range.toAsStr != null) { + to = parser.parseDouble(range.toAsStr, false, context.getQueryShardContext()::nowInMillis); + } + return new Range(range.key, from, range.fromAsStr, to, range.toAsStr); + }); if (ranges.length == 0) { throw new IllegalArgumentException("No [ranges] specified for the [" + this.getName() + "] aggregation"); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java index 45128030759..4bbbdcba27e 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java @@ -90,8 +90,27 @@ public class RangeAggregator extends BucketsAggregator { out.writeDouble(to); } + public double getFrom() { + return this.from; + } - protected Range(String key, Double from, String fromAsStr, Double to, String toAsStr) { + public double getTo() { + return this.to; + } + + public String getFromAsString() { + return this.fromAsStr; + } + + public String getToAsString() { + return this.toAsStr; + } + + public String getKey() { + return this.key; + } + + public Range(String key, Double from, String fromAsStr, Double to, String toAsStr) { this.key = key; this.from = from == null ? Double.NEGATIVE_INFINITY : from; this.fromAsStr = fromAsStr; @@ -108,19 +127,6 @@ public class RangeAggregator extends BucketsAggregator { return "[" + from + " to " + to + ")"; } - public Range process(DocValueFormat parser, SearchContext context) { - assert parser != null; - Double from = this.from; - Double to = this.to; - if (fromAsStr != null) { - from = parser.parseDouble(fromAsStr, false, context.getQueryShardContext()::nowInMillis); - } - if (toAsStr != null) { - to = parser.parseDouble(toAsStr, false, context.getQueryShardContext()::nowInMillis); - } - return new Range(key, from, fromAsStr, to, toAsStr); - } - public static Range fromXContent(XContentParser parser) throws IOException { XContentParser.Token token; String currentFieldName = null; diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java index bd4326ce40e..d54d8cd10f0 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.search.aggregations.bucket; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; @@ -136,7 +137,6 @@ public class DateRangeIT extends ESIntegTestCase { assertThat(range.getName(), equalTo("range")); assertThat(range.getBuckets().size(), equalTo(3)); - // TODO: use diamond once JI-9019884 is fixed List buckets = new ArrayList<>(range.getBuckets()); Range.Bucket bucket = buckets.get(0); @@ -855,7 +855,6 @@ public class DateRangeIT extends ESIntegTestCase { assertThat(bucket, Matchers.notNullValue()); Range dateRange = bucket.getAggregations().get("date_range"); - // TODO: use diamond once JI-9019884 is fixed List buckets = new ArrayList<>(dateRange.getBuckets()); assertThat(dateRange, Matchers.notNullValue()); assertThat(dateRange.getName(), equalTo("date_range")); @@ -926,4 +925,142 @@ public class DateRangeIT extends ESIntegTestCase { assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache() .getMissCount(), equalTo(1L)); } + + /** + * Test querying ranges on date mapping specifying a format with to/from + * values specified as Strings + */ + public void testRangeWithFormatStringValue() throws Exception { + String indexName = "dateformat_test_idx"; + assertAcked(prepareCreate(indexName).addMapping("type", "date", "type=date,format=strict_hour_minute_second")); + indexRandom(true, + client().prepareIndex(indexName, "type", "1").setSource(jsonBuilder().startObject().field("date", "00:16:40").endObject()), + client().prepareIndex(indexName, "type", "2").setSource(jsonBuilder().startObject().field("date", "00:33:20").endObject()), + client().prepareIndex(indexName, "type", "3").setSource(jsonBuilder().startObject().field("date", "00:50:00").endObject())); + + // using no format should work when to/from is compatible with format in + // mapping + SearchResponse searchResponse = client().prepareSearch(indexName).setSize(0) + .addAggregation(dateRange("date_range").field("date").addRange("00:16:40", "00:50:00").addRange("00:50:00", "01:06:40")) + .get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + List buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "00:16:40-00:50:00", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "00:50:00-01:06:40", 3000000L, 4000000L); + + // using different format should work when to/from is compatible with + // format in aggregation + searchResponse = client().prepareSearch(indexName).setSize(0).addAggregation( + dateRange("date_range").field("date").addRange("00.16.40", "00.50.00").addRange("00.50.00", "01.06.40").format("HH.mm.ss")) + .get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "00.16.40-00.50.00", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "00.50.00-01.06.40", 3000000L, 4000000L); + + // providing numeric input with format should work, but bucket keys are + // different now + searchResponse = client().prepareSearch(indexName).setSize(0) + .addAggregation( + dateRange("date_range").field("date").addRange(1000000, 3000000).addRange(3000000, 4000000).format("epoch_millis")) + .get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "1000000-3000000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000000-4000000", 3000000L, 4000000L); + + // providing numeric input without format should throw an exception + Exception e = expectThrows(Exception.class, () -> client().prepareSearch(indexName).setSize(0) + .addAggregation(dateRange("date_range").field("date").addRange(1000000, 3000000).addRange(3000000, 4000000)).get()); + Throwable cause = e.getCause(); + assertThat(cause, instanceOf(ElasticsearchParseException.class)); + assertEquals("failed to parse date field [1000000] with format [strict_hour_minute_second]", cause.getMessage()); + } + + /** + * Test querying ranges on date mapping specifying a format with to/from + * values specified as numeric value + */ + public void testRangeWithFormatNumericValue() throws Exception { + String indexName = "dateformat_numeric_test_idx"; + assertAcked(prepareCreate(indexName).addMapping("type", "date", "type=date,format=epoch_second")); + indexRandom(true, + client().prepareIndex(indexName, "type", "1").setSource(jsonBuilder().startObject().field("date", 1000).endObject()), + client().prepareIndex(indexName, "type", "2").setSource(jsonBuilder().startObject().field("date", 2000).endObject()), + client().prepareIndex(indexName, "type", "3").setSource(jsonBuilder().startObject().field("date", 3000).endObject())); + + // using no format should work when to/from is compatible with format in + // mapping + SearchResponse searchResponse = client().prepareSearch(indexName).setSize(0) + .addAggregation(dateRange("date_range").field("date").addRange(1000, 3000).addRange(3000, 4000)).get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + List buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); + + // using no format should also work when and to/from are string values + searchResponse = client().prepareSearch(indexName).setSize(0) + .addAggregation(dateRange("date_range").field("date").addRange("1000", "3000").addRange("3000", "4000")).get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); + + // also e-notation should work, fractional parts should be truncated + searchResponse = client().prepareSearch(indexName).setSize(0) + .addAggregation(dateRange("date_range").field("date").addRange(1.0e3, 3000.8123).addRange(3000.8123, 4.0e3)).get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); + + // however, e-notation should and fractional parts provided as string + // should be parsed and error if not compatible + Exception e = expectThrows(Exception.class, () -> client().prepareSearch(indexName).setSize(0) + .addAggregation(dateRange("date_range").field("date").addRange("1.0e3", "3.0e3").addRange("3.0e3", "4.0e3")).get()); + assertThat(e.getCause(), instanceOf(ElasticsearchParseException.class)); + assertEquals("failed to parse date field [1.0e3] with format [epoch_second]", e.getCause().getMessage()); + + e = expectThrows(Exception.class, () -> client().prepareSearch(indexName).setSize(0) + .addAggregation(dateRange("date_range").field("date").addRange("1000.123", "3000.8").addRange("3000.8", "4000.3")).get()); + assertThat(e.getCause(), instanceOf(ElasticsearchParseException.class)); + assertEquals("failed to parse date field [1000.123] with format [epoch_second]", e.getCause().getMessage()); + + // using different format should work when to/from is compatible with + // format in aggregation + searchResponse = client().prepareSearch(indexName).setSize(0).addAggregation( + dateRange("date_range").field("date").addRange("00.16.40", "00.50.00").addRange("00.50.00", "01.06.40").format("HH.mm.ss")) + .get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "00.16.40-00.50.00", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "00.50.00-01.06.40", 3000000L, 4000000L); + + // providing different numeric input with format should work, but bucket + // keys are different now + searchResponse = client().prepareSearch(indexName).setSize(0) + .addAggregation( + dateRange("date_range").field("date").addRange(1000000, 3000000).addRange(3000000, 4000000).format("epoch_millis")) + .get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "1000000-3000000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000000-4000000", 3000000L, 4000000L); + } + + private static List checkBuckets(Range dateRange, String expectedAggName, long expectedBucketsSize) { + assertThat(dateRange, Matchers.notNullValue()); + assertThat(dateRange.getName(), equalTo(expectedAggName)); + List buckets = new ArrayList<>(dateRange.getBuckets()); + assertThat(buckets.size(), is(2)); + return buckets; + } + + private static void assertBucket(Bucket bucket, long bucketSize, String expectedKey, long expectedFrom, long expectedTo) { + assertThat(bucket.getDocCount(), equalTo(bucketSize)); + assertThat((String) bucket.getKey(), equalTo(expectedKey)); + assertThat(((DateTime) bucket.getFrom()).getMillis(), equalTo(expectedFrom)); + assertThat(((DateTime) bucket.getTo()).getMillis(), equalTo(expectedTo)); + assertThat(bucket.getAggregations().asList().isEmpty(), is(true)); + } } diff --git a/docs/reference/migration/migrate_6_0/aggregations.asciidoc b/docs/reference/migration/migrate_6_0/aggregations.asciidoc index c3414e7cf26..4e10c6de80b 100644 --- a/docs/reference/migration/migrate_6_0/aggregations.asciidoc +++ b/docs/reference/migration/migrate_6_0/aggregations.asciidoc @@ -49,3 +49,11 @@ POST /twitter/_search?size=0 -------------------------------------------------- // CONSOLE // TEST[setup:twitter] + +==== Numeric `to` and `from` parameters in `date_range` aggregation are interpreted according to `format` now + +Numeric `to` and `from` parameters in `date_range` aggregations used to always be interpreted as `epoch_millis`, +making other numeric formats like `epoch_seconds` unusable for numeric input values. +Now we interpret these parameters according to the `format` of the target field. +If the `format` in the mappings is not compatible with the numeric input value, a compatible +`format` (e.g. `epoch_millis`, `epoch_second`) must be specified in the `date_range` aggregation, otherwise an error is thrown. diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml index c058721124f..fd8a016976d 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml @@ -14,6 +14,7 @@ setup: type: double date: type: date + format: epoch_second - do: cluster.health: @@ -225,3 +226,50 @@ setup: - match: { aggregations.ip_range.buckets.1.doc_count: 2 } +--- +"Date range": + - skip: + version: " - 5.99.99" + reason: before 6.0, numeric date_range to/from parameters were always parsed as if they are epoch_millis (#17920) + - do: + index: + index: test + type: test + id: 1 + body: { "date" : 1000 } + + - do: + index: + index: test + type: test + id: 2 + body: { "date" : 2000 } + + - do: + index: + index: test + type: test + id: 3 + body: { "date" : 3000 } + + - do: + indices.refresh: {} + + - do: + search: + body: { "size" : 0, "aggs" : { "date_range" : { "date_range" : { "field" : "date", "ranges": [ { "from" : 1000, "to": 3000 }, { "from": 3000, "to": 4000 } ] } } } } + + - match: { hits.total: 3 } + + - length: { aggregations.date_range.buckets: 2 } + + - match: { aggregations.date_range.buckets.0.doc_count: 2 } + - match: { aggregations.date_range.buckets.0.key: "1000-3000" } + - match: { aggregations.date_range.buckets.0.from: 1000000 } + - match: { aggregations.date_range.buckets.0.to: 3000000 } + + - match: { aggregations.date_range.buckets.1.doc_count: 1 } + - match: { aggregations.date_range.buckets.1.key: "3000-4000" } + - match: { aggregations.date_range.buckets.1.from: 3000000 } + - match: { aggregations.date_range.buckets.1.to: 4000000 } +