From d4befe8f2a2b9314a602db586b9a3f177a2fb5ea Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Wed, 8 Jan 2025 12:29:35 -0800 Subject: [PATCH 01/13] Add comments explanations for auto date histo increaseRoundingIfNeeded. Signed-off-by: Finn Carroll --- .../AutoDateHistogramAggregator.java | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java index f3a36b4882d19..b3fbabd8641f3 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java @@ -403,12 +403,39 @@ private void collectValue(int doc, long rounded) throws IOException { increaseRoundingIfNeeded(rounded); } + /** + * Examine our current bucket count and the most recently added bucket to determine if an update to + * preparedRounding is required to keep total bucket count in compliance with targetBuckets. + * + * @param rounded the most recently collected value rounded + */ private void increaseRoundingIfNeeded(long rounded) { + // If we are already using the rounding with the largest interval nothing can be done if (roundingIdx >= roundingInfos.length - 1) { return; } + + // Re calculate the max and min values we expect to bucket according to most recently rounded val min = Math.min(min, rounded); max = Math.max(max, rounded); + + /** + * Quick explanation of the two below conditions: + * + * 1. [targetBuckets * roundingInfos[roundingIdx].getMaximumInnerInterval()] + * Represents the total bucket count possible before we will exceed targetBuckets + * even if we use the maximum inner interval of our current rounding. For example, consider the + * DAYS_OF_MONTH rounding where the maximum inner interval is 7 days (i.e. 1 week buckets). + * targetBuckets * roundingInfos[roundingIdx].getMaximumInnerInterval() would then be the number of + * 1 day buckets possible such that if we re-bucket to 1 week buckets we will have more 1 week buckets + * than our targetBuckets limit. If the current count of buckets exceeds this limit we must update + * our rounding. + * + * 2. [targetBuckets * roundingInfos[roundingIdx].getMaximumRoughEstimateDurationMillis()] + * The total duration of ms covered by our current rounding. In the case of MINUTES_OF_HOUR rounding + * getMaximumRoughEstimateDurationMillis is 60000. If our current total range in millis (max - min) + * exceeds this range we must update our rounding. + */ if (bucketOrds.size() <= targetBuckets * roundingInfos[roundingIdx].getMaximumInnerInterval() && max - min <= targetBuckets * roundingInfos[roundingIdx].getMaximumRoughEstimateDurationMillis()) { return; From f89f68b56837bd9d1eb5a06547ce4fd4dfd9342f Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 14 Jan 2025 10:49:02 -0800 Subject: [PATCH 02/13] Add testFilterRewriteWithTZRoundingRangeAssert() to reproduce auto date histo assertion bug per #16932 Signed-off-by: Finn Carroll --- .../AutoDateHistogramAggregatorTests.java | 57 +++++++++++++++++++ .../aggregations/AggregatorTestCase.java | 22 +++++-- 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java index dda053af78b30..e852958c43e4b 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java @@ -38,7 +38,9 @@ import org.apache.lucene.document.SortedSetDocValuesField; import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.IndexWriterConfig; import org.apache.lucene.index.IndexableField; +import org.apache.lucene.index.NoMergePolicy; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.MatchNoDocsQuery; @@ -72,6 +74,7 @@ import java.time.Instant; import java.time.LocalDate; import java.time.YearMonth; +import java.time.ZoneId; import java.time.ZoneOffset; import java.time.ZonedDateTime; import java.util.ArrayList; @@ -912,6 +915,60 @@ public void testWithPipelineReductions() throws IOException { ); } + // Bugfix: https://github.com/opensearch-project/OpenSearch/issues/16932 + public void testFilterRewriteWithTZRoundingRangeAssert() throws IOException { + /* + multiBucketIndexData must overlap with DST to produce a 'LinkedListLookup' prepared rounding. + This lookup rounding style maintains a strict max/min input range and will assert each value is in range. + */ + final List multiBucketIndexData = Arrays.asList( + ZonedDateTime.of(2023, 10, 10, 0, 0, 0, 0, ZoneOffset.UTC), + ZonedDateTime.of(2023, 11, 11, 0, 0, 0, 0, ZoneOffset.UTC) + ); + + final List singleBucketIndexData = Arrays.asList( + ZonedDateTime.of(2023, 12, 27, 0, 0, 0, 0, ZoneOffset.UTC) + ); + + try (Directory directory = newDirectory()) { + /* + Ensure we produce two segments on one shard such that the documents in seg 1 will be out of range of the + prepared rounding produced by the filter rewrite optimization considering seg 2 for optimized path. + */ + IndexWriterConfig c = newIndexWriterConfig().setMergePolicy(NoMergePolicy.INSTANCE); + try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory, c)) { + indexSampleData(multiBucketIndexData, indexWriter); + indexWriter.flush(); + indexSampleData(singleBucketIndexData, indexWriter); + } + + try (IndexReader indexReader = DirectoryReader.open(directory)) { + final IndexSearcher indexSearcher = newSearcher(indexReader, true, true); + + // Force agg to update rounding when it begins collecting from the second segment. + final AutoDateHistogramAggregationBuilder aggregationBuilder = new AutoDateHistogramAggregationBuilder("_name"); + aggregationBuilder.setNumBuckets(3).field(DATE_FIELD).timeZone(ZoneId.of("America/New_York")); + + Map expectedDocCount = new TreeMap<>(); + expectedDocCount.put("2023-10-01T00:00:00.000-04:00", 1); + expectedDocCount.put("2023-11-01T00:00:00.000-04:00", 1); + expectedDocCount.put("2023-12-01T00:00:00.000-05:00", 1); + + final InternalAutoDateHistogram histogram = searchAndReduce( + indexSearcher, + DEFAULT_QUERY, + aggregationBuilder, + false, + new DateFieldMapper.DateFieldType(aggregationBuilder.field()), + new NumberFieldMapper.NumberFieldType(INSTANT_FIELD, NumberFieldMapper.NumberType.LONG), + new NumberFieldMapper.NumberFieldType(NUMERIC_FIELD, NumberFieldMapper.NumberType.LONG) + ); + + assertThat(bucketCountsAsMap(histogram), equalTo(expectedDocCount)); + } + } + } + @Override protected IndexSettings createIndexSettings() { final Settings nodeSettings = Settings.builder().put("search.max_buckets", 25000).build(); diff --git a/test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java b/test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java index 27142b298db52..0fcea531fdcb8 100644 --- a/test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java +++ b/test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java @@ -609,9 +609,19 @@ protected A searchAndReduc IndexSearcher searcher, Query query, AggregationBuilder builder, + boolean shardFanOut, MappedFieldType... fieldTypes ) throws IOException { - return searchAndReduce(createIndexSettings(), searcher, query, builder, DEFAULT_MAX_BUCKETS, fieldTypes); + return searchAndReduce(createIndexSettings(), searcher, query, builder, DEFAULT_MAX_BUCKETS, shardFanOut, fieldTypes); + } + + protected A searchAndReduce( + IndexSearcher searcher, + Query query, + AggregationBuilder builder, + MappedFieldType... fieldTypes + ) throws IOException { + return searchAndReduce(createIndexSettings(), searcher, query, builder, DEFAULT_MAX_BUCKETS, randomBoolean(), fieldTypes); } protected A searchAndReduce( @@ -621,7 +631,7 @@ protected A searchAndReduc AggregationBuilder builder, MappedFieldType... fieldTypes ) throws IOException { - return searchAndReduce(indexSettings, searcher, query, builder, DEFAULT_MAX_BUCKETS, fieldTypes); + return searchAndReduce(indexSettings, searcher, query, builder, DEFAULT_MAX_BUCKETS, randomBoolean(), fieldTypes); } protected A searchAndReduce( @@ -631,7 +641,7 @@ protected A searchAndReduc int maxBucket, MappedFieldType... fieldTypes ) throws IOException { - return searchAndReduce(createIndexSettings(), searcher, query, builder, maxBucket, fieldTypes); + return searchAndReduce(createIndexSettings(), searcher, query, builder, maxBucket, randomBoolean(), fieldTypes); } protected A searchAndReduce( @@ -640,9 +650,10 @@ protected A searchAndReduc Query query, AggregationBuilder builder, int maxBucket, + boolean shardFanOut, MappedFieldType... fieldTypes ) throws IOException { - return searchAndReduce(indexSettings, searcher, query, builder, maxBucket, false, fieldTypes); + return searchAndReduce(indexSettings, searcher, query, builder, maxBucket, false, shardFanOut, fieldTypes); } /** @@ -660,6 +671,7 @@ protected A searchAndReduc AggregationBuilder builder, int maxBucket, boolean hasNested, + boolean shardFanOut, MappedFieldType... fieldTypes ) throws IOException { final IndexReaderContext ctx = searcher.getTopReaderContext(); @@ -675,7 +687,7 @@ protected A searchAndReduc ); C root = createAggregator(query, builder, searcher, bucketConsumer, fieldTypes); - if (randomBoolean() && searcher.getIndexReader().leaves().size() > 0) { + if (shardFanOut && searcher.getIndexReader().leaves().size() > 0) { assertThat(ctx, instanceOf(CompositeReaderContext.class)); final CompositeReaderContext compCTX = (CompositeReaderContext) ctx; final int size = compCTX.leaves().size(); From f00226643b4e7afd3b498189d3ce012796abe1d8 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 14 Jan 2025 11:20:22 -0800 Subject: [PATCH 03/13] Fix #16932. Ensure optimized path can only increase preparedRounding of agg. Signed-off-by: Finn Carroll --- .../histogram/AutoDateHistogramAggregator.java | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java index b3fbabd8641f3..4b036bf4bb17b 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java @@ -149,7 +149,6 @@ private AutoDateHistogramAggregator( Aggregator parent, Map metadata ) throws IOException { - super(name, factories, aggregationContext, parent, metadata); this.targetBuckets = targetBuckets; // TODO: Remove null usage here, by using a different aggregator for create @@ -170,6 +169,17 @@ protected void prepare() throws IOException { buildRanges(context); } + /** + * The filter rewrite optimization uses this method to pre-emptively update the preparedRounding + * when considering the optimized path for a single segment. This is necessary since the optimized path + * skips doc collection entirely which is where the preparedRounding is normally updated. + * + * @param low lower bound of rounding to prepare + * @param high upper bound of rounding to prepare + * @return select a prepared rounding which satisfies the conditions: + * 1. Is at least as large as our previously prepared rounding + * 2. Must span a range of [low, high] with buckets <= targetBuckets + */ @Override protected Rounding getRounding(final long low, final long high) { // max - min / targetBuckets = bestDuration @@ -177,7 +187,8 @@ protected Rounding getRounding(final long low, final long high) { // since we cannot exceed targetBuckets, bestDuration should go up, // so the right innerInterval should be an upper bound long bestDuration = (high - low) / targetBuckets; - // reset so this function is idempotent + + int prevRoundingIdx = roundingIdx; roundingIdx = 0; while (roundingIdx < roundingInfos.length - 1) { final RoundingInfo curRoundingInfo = roundingInfos[roundingIdx]; @@ -190,6 +201,8 @@ protected Rounding getRounding(final long low, final long high) { roundingIdx++; } + // Ensure preparedRounding never shrinks + roundingIdx = Math.max(prevRoundingIdx, roundingIdx); preparedRounding = prepareRounding(roundingIdx); return roundingInfos[roundingIdx].rounding; } From fb5c6b2509a8cfeb3dadcb0c772c51fea3d4a216 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 14 Jan 2025 11:23:25 -0800 Subject: [PATCH 04/13] Spotless apply Signed-off-by: Finn Carroll --- .../bucket/histogram/AutoDateHistogramAggregatorTests.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java index e852958c43e4b..95f56d779b088 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java @@ -926,9 +926,7 @@ public void testFilterRewriteWithTZRoundingRangeAssert() throws IOException { ZonedDateTime.of(2023, 11, 11, 0, 0, 0, 0, ZoneOffset.UTC) ); - final List singleBucketIndexData = Arrays.asList( - ZonedDateTime.of(2023, 12, 27, 0, 0, 0, 0, ZoneOffset.UTC) - ); + final List singleBucketIndexData = Arrays.asList(ZonedDateTime.of(2023, 12, 27, 0, 0, 0, 0, ZoneOffset.UTC)); try (Directory directory = newDirectory()) { /* From 122e40ffbbca6b627fedaa9464a6036af947ac39 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Thu, 16 Jan 2025 13:25:32 -0800 Subject: [PATCH 05/13] Fast fail filter rewrite opt in data histo aggs for non UTC timezones Signed-off-by: Finn Carroll --- server/src/main/java/org/opensearch/common/Rounding.java | 2 +- .../filterrewrite/DateHistogramAggregatorBridge.java | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/common/Rounding.java b/server/src/main/java/org/opensearch/common/Rounding.java index 12a399635046e..32114fc4129fe 100644 --- a/server/src/main/java/org/opensearch/common/Rounding.java +++ b/server/src/main/java/org/opensearch/common/Rounding.java @@ -1403,7 +1403,7 @@ public static OptionalLong getInterval(Rounding rounding) { * Helper function for checking if the time zone requested for date histogram * aggregation is utc or not */ - private static boolean isUTCTimeZone(final ZoneId zoneId) { + public static boolean isUTCTimeZone(final ZoneId zoneId) { return "Z".equals(zoneId.getDisplayName(TextStyle.FULL, Locale.ENGLISH)); } } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java index c780732a5ddce..bef0c317222bd 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java @@ -23,6 +23,7 @@ import java.util.function.BiConsumer; import java.util.function.Function; +import static org.opensearch.common.Rounding.isUTCTimeZone; import static org.opensearch.search.aggregations.bucket.filterrewrite.PointTreeTraversal.multiRangesTraverse; /** @@ -33,6 +34,14 @@ public abstract class DateHistogramAggregatorBridge extends AggregatorBridge { int maxRewriteFilters; protected boolean canOptimize(ValuesSourceConfig config) { + /** + * The filter rewrite optimized path does not support bucket intervals which are not fixed. + * For this reason we exclude non UTC timezones. + */ + if (!isUTCTimeZone(config.timezone())) { + return false; + } + if (config.script() == null && config.missing() == null) { MappedFieldType fieldType = config.fieldType(); if (fieldType instanceof DateFieldMapper.DateFieldType) { From 5ddddc374cd27fff7c36d43827acaf9aa4d908ce Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Thu, 16 Jan 2025 13:31:25 -0800 Subject: [PATCH 06/13] Remove redundant UTC check from getInterval(). Signed-off-by: Finn Carroll --- server/src/main/java/org/opensearch/common/Rounding.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/Rounding.java b/server/src/main/java/org/opensearch/common/Rounding.java index 32114fc4129fe..e5489954b3491 100644 --- a/server/src/main/java/org/opensearch/common/Rounding.java +++ b/server/src/main/java/org/opensearch/common/Rounding.java @@ -1382,16 +1382,8 @@ public static OptionalLong getInterval(Rounding rounding) { if (rounding instanceof TimeUnitRounding) { interval = (((TimeUnitRounding) rounding).unit).extraLocalOffsetLookup(); - if (!isUTCTimeZone(((TimeUnitRounding) rounding).timeZone)) { - // Fast filter aggregation cannot be used if it needs time zone rounding - return OptionalLong.empty(); - } } else if (rounding instanceof TimeIntervalRounding) { interval = ((TimeIntervalRounding) rounding).interval; - if (!isUTCTimeZone(((TimeIntervalRounding) rounding).timeZone)) { - // Fast filter aggregation cannot be used if it needs time zone rounding - return OptionalLong.empty(); - } } else { return OptionalLong.empty(); } From 65d684bcf53e9e824da2cc179fdb6347b2b11914 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Thu, 16 Jan 2025 13:35:47 -0800 Subject: [PATCH 07/13] Save a call to prepareRounding if roundingIdx is unchanged. Signed-off-by: Finn Carroll --- .../bucket/histogram/AutoDateHistogramAggregator.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java index 4b036bf4bb17b..88faf69af3c38 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java @@ -203,7 +203,10 @@ protected Rounding getRounding(final long low, final long high) { // Ensure preparedRounding never shrinks roundingIdx = Math.max(prevRoundingIdx, roundingIdx); - preparedRounding = prepareRounding(roundingIdx); + if (roundingIdx != prevRoundingIdx) { + preparedRounding = prepareRounding(roundingIdx); + } + return roundingInfos[roundingIdx].rounding; } From ebd9afb2121680d164a8e9af2bef67f2b36926aa Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Thu, 16 Jan 2025 13:41:36 -0800 Subject: [PATCH 08/13] Spotless apply Signed-off-by: Finn Carroll --- .../bucket/histogram/AutoDateHistogramAggregator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java index 88faf69af3c38..0ad408bbd3ab4 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregator.java @@ -206,7 +206,7 @@ protected Rounding getRounding(final long low, final long high) { if (roundingIdx != prevRoundingIdx) { preparedRounding = prepareRounding(roundingIdx); } - + return roundingInfos[roundingIdx].rounding; } From 0a5825227d360c15975edb76d31421ad22296aa6 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Thu, 16 Jan 2025 14:11:42 -0800 Subject: [PATCH 09/13] Changelog Signed-off-by: Finn Carroll --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 139ce50608699..9751bc90abc74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -104,6 +104,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix multi-value sort for unsigned long ([#16732](https://github.com/opensearch-project/OpenSearch/pull/16732)) - The `phone-search` analyzer no longer emits the tel/sip prefix, international calling code, extension numbers and unformatted input as a token ([#16993](https://github.com/opensearch-project/OpenSearch/pull/16993)) - Fix GRPC AUX_TRANSPORT_PORT and SETTING_GRPC_PORT settings and remove lingering HTTP terminology ([#17037](https://github.com/opensearch-project/OpenSearch/pull/17037)) +- Fix AutoDateHistogramAggregator rounding assertion failure ([#17023](https://github.com/opensearch-project/OpenSearch/pull/17023)) ### Security From 95fea3eaa8a64cb219d1cbeceb7c240cf7834803 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Fri, 17 Jan 2025 09:03:02 -0800 Subject: [PATCH 10/13] Add ZoneId getter for date histo filter rewrite canOptimize check. Signed-off-by: Finn Carroll --- .../src/main/java/org/opensearch/search/DocValueFormat.java | 4 ++++ .../bucket/filterrewrite/DateHistogramAggregatorBridge.java | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/search/DocValueFormat.java b/server/src/main/java/org/opensearch/search/DocValueFormat.java index 9fae14f69b0af..d2a627eda1d15 100644 --- a/server/src/main/java/org/opensearch/search/DocValueFormat.java +++ b/server/src/main/java/org/opensearch/search/DocValueFormat.java @@ -286,6 +286,10 @@ public DateMathParser getDateMathParser() { return parser; } + public ZoneId getZoneId() { + return timeZone; + } + @Override public String format(long value) { return formatter.format(resolution.toInstant(value).atZone(timeZone)); diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java index bef0c317222bd..0056b6bd4be77 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java @@ -14,6 +14,7 @@ import org.opensearch.common.Rounding; import org.opensearch.index.mapper.DateFieldMapper; import org.opensearch.index.mapper.MappedFieldType; +import org.opensearch.search.DocValueFormat; import org.opensearch.search.aggregations.bucket.histogram.LongBounds; import org.opensearch.search.aggregations.support.ValuesSourceConfig; import org.opensearch.search.internal.SearchContext; @@ -38,7 +39,9 @@ protected boolean canOptimize(ValuesSourceConfig config) { * The filter rewrite optimized path does not support bucket intervals which are not fixed. * For this reason we exclude non UTC timezones. */ - if (!isUTCTimeZone(config.timezone())) { + if (config.format() instanceof DocValueFormat.DateTime && + !isUTCTimeZone(((DocValueFormat.DateTime) config.format()).getZoneId()) + ) { return false; } From 444db387d39a360f3e722829af8dae3994f6816c Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Fri, 17 Jan 2025 09:12:08 -0800 Subject: [PATCH 11/13] Spotless apply Signed-off-by: Finn Carroll --- .../bucket/filterrewrite/DateHistogramAggregatorBridge.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java index 0056b6bd4be77..f17b3fa267d4c 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/filterrewrite/DateHistogramAggregatorBridge.java @@ -39,9 +39,7 @@ protected boolean canOptimize(ValuesSourceConfig config) { * The filter rewrite optimized path does not support bucket intervals which are not fixed. * For this reason we exclude non UTC timezones. */ - if (config.format() instanceof DocValueFormat.DateTime && - !isUTCTimeZone(((DocValueFormat.DateTime) config.format()).getZoneId()) - ) { + if (config.format() instanceof DocValueFormat.DateTime && !isUTCTimeZone(((DocValueFormat.DateTime) config.format()).getZoneId())) { return false; } From af705f038a002a2ef79d51e4be3058e57d81579a Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 21 Jan 2025 09:12:04 -0800 Subject: [PATCH 12/13] Disable ff optimzation for composite agg in canOptimize. Signed-off-by: Finn Carroll --- .../main/java/org/opensearch/common/Rounding.java | 4 +++- .../bucket/composite/CompositeAggregator.java | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/common/Rounding.java b/server/src/main/java/org/opensearch/common/Rounding.java index e5489954b3491..65c72e5a24114 100644 --- a/server/src/main/java/org/opensearch/common/Rounding.java +++ b/server/src/main/java/org/opensearch/common/Rounding.java @@ -489,7 +489,7 @@ public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { * * @opensearch.internal */ - static class TimeUnitRounding extends Rounding { + public static class TimeUnitRounding extends Rounding { static final byte ID = 1; private final DateTimeUnit unit; @@ -517,6 +517,8 @@ public byte id() { return ID; } + public ZoneId getTimeZone() { return timeZone; } + private LocalDateTime truncateLocalDateTime(LocalDateTime localDateTime) { switch (unit) { case SECOND_OF_MINUTE: diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java index cfe716eb57ca8..f28bcd934ac56 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java @@ -82,17 +82,20 @@ import org.opensearch.search.sort.SortAndFormats; import java.io.IOException; +import java.time.ZoneId; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.TimeZone; import java.util.function.BiConsumer; import java.util.function.Function; import java.util.function.LongUnaryOperator; import java.util.stream.Collectors; +import static org.opensearch.common.Rounding.isUTCTimeZone; import static org.opensearch.search.aggregations.MultiBucketConsumerService.MAX_BUCKET_SETTING; import static org.opensearch.search.aggregations.bucket.filterrewrite.DateHistogramAggregatorBridge.segmentMatchAll; @@ -182,6 +185,18 @@ protected boolean canOptimize() { }); } + /** + * The filter rewrite optimized path does not support bucket intervals which are not fixed. + * For this reason we exclude non UTC timezones. + */ + Rounding round = this.valuesSource.getRounding(); + if (round instanceof Rounding.TimeUnitRounding) { + ZoneId tz = ((Rounding.TimeUnitRounding) round).getTimeZone(); + if (!isUTCTimeZone(tz)) { + return false; + } + } + // bucketOrds is used for saving the date histogram results got from the optimization path bucketOrds = LongKeyedBucketOrds.build(context.bigArrays(), CardinalityUpperBound.ONE); return true; From 569502d4dd7a5353ee232a00753822b2f7f6e525 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 21 Jan 2025 09:14:25 -0800 Subject: [PATCH 13/13] Spotless apply Signed-off-by: Finn Carroll --- server/src/main/java/org/opensearch/common/Rounding.java | 4 +++- .../aggregations/bucket/composite/CompositeAggregator.java | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/Rounding.java b/server/src/main/java/org/opensearch/common/Rounding.java index 65c72e5a24114..f1845e47c8956 100644 --- a/server/src/main/java/org/opensearch/common/Rounding.java +++ b/server/src/main/java/org/opensearch/common/Rounding.java @@ -517,7 +517,9 @@ public byte id() { return ID; } - public ZoneId getTimeZone() { return timeZone; } + public ZoneId getTimeZone() { + return timeZone; + } private LocalDateTime truncateLocalDateTime(LocalDateTime localDateTime) { switch (unit) { diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java index f28bcd934ac56..020da5dfa235f 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java @@ -89,7 +89,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.TimeZone; import java.util.function.BiConsumer; import java.util.function.Function; import java.util.function.LongUnaryOperator;