From d1674ebf5a14d6fce40f875eeef794470cbe5055 Mon Sep 17 00:00:00 2001 From: bowenlan-amzn Date: Wed, 6 Dec 2023 09:40:49 -0800 Subject: [PATCH] Cleaning Signed-off-by: bowenlan-amzn --- .../search/aggregations/bucket/BucketsAggregator.java | 5 +---- .../aggregations/bucket/FilterRewriteHelper.java | 7 +++---- .../bucket/composite/CompositeAggregator.java | 10 +++++++++- .../composite/CompositeValuesCollectorQueue.java | 10 +++++----- .../bucket/composite/LongValuesSource.java | 3 +-- .../bucket/composite/PointsSortedDocsProducer.java | 5 +++-- .../bucket/composite/SingleDimensionValuesSource.java | 3 +-- .../bucket/composite/SortedDocsProducer.java | 7 +++---- .../histogram/AutoDateHistogramAggregatorTests.java | 10 ++++------ 9 files changed, 30 insertions(+), 30 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/BucketsAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/BucketsAggregator.java index 8e43bf6ec6dd9..eef427754f535 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/BucketsAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/BucketsAggregator.java @@ -422,10 +422,7 @@ protected final InternalAggregation[] buildAggregationsForVariableBuckets( + "]" ); } - buckets.add(bucketBuilder.build( - ordsEnum.value(), - bucketDocCount(ordsEnum.ord()), - subAggregationResults[b++])); + buckets.add(bucketBuilder.build(ordsEnum.value(), bucketDocCount(ordsEnum.ord()), subAggregationResults[b++])); } results[ordIdx] = resultBuilder.build(owningBucketOrds[ordIdx], buckets); } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/FilterRewriteHelper.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/FilterRewriteHelper.java index 78418d25d5217..f5ccc4e03c166 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/FilterRewriteHelper.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/FilterRewriteHelper.java @@ -146,7 +146,7 @@ private static Weight[] createFilterForAggregations( // Below rounding is needed as the interval could return in // non-rounded values for something like calendar month roundedLow = preparedRounding.round(roundedLow + interval); - if (prevRounded == roundedLow) break; // TODO reading prevents getting into an infinite loop? + if (prevRounded == roundedLow) break; // prevents getting into an infinite loop prevRounded = roundedLow; } @@ -210,9 +210,9 @@ public static FilterContext buildFastFilterContext( CheckedFunction computeBounds ) throws IOException { if (parent == null && subAggLength == 0 && !valueSourceContext.missing && !valueSourceContext.hasScript) { - MappedFieldType fieldType = valueSourceContext.getFieldType(); + MappedFieldType fieldType = valueSourceContext.fieldType; if (fieldType != null) { - final String fieldName = valueSourceContext.getFieldType().name(); + final String fieldName = fieldType.name(); final long[] bounds = computeBounds.apply(valueSourceContext); if (bounds != null) { assert fieldType instanceof DateFieldMapper.DateFieldType; @@ -252,7 +252,6 @@ public ValueSourceContext(boolean missing, boolean hasScript, MappedFieldType fi this.fieldType = fieldType; } - // TODO reading why boolean doesn't need getter? public MappedFieldType getFieldType() { return fieldType; } 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 bbb9c6167e6f7..519de6712a80c 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 @@ -63,7 +63,15 @@ import org.opensearch.index.mapper.DateFieldMapper; import org.opensearch.lucene.queries.SearchAfterSortedDocQuery; import org.opensearch.search.DocValueFormat; -import org.opensearch.search.aggregations.*; +import org.opensearch.search.aggregations.Aggregator; +import org.opensearch.search.aggregations.AggregatorFactories; +import org.opensearch.search.aggregations.BucketCollector; +import org.opensearch.search.aggregations.CardinalityUpperBound; +import org.opensearch.search.aggregations.InternalAggregation; +import org.opensearch.search.aggregations.InternalAggregations; +import org.opensearch.search.aggregations.LeafBucketCollector; +import org.opensearch.search.aggregations.MultiBucketCollector; +import org.opensearch.search.aggregations.MultiBucketConsumerService; import org.opensearch.search.aggregations.bucket.BucketsAggregator; import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import org.opensearch.search.aggregations.bucket.terms.LongKeyedBucketOrds; diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesCollectorQueue.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesCollectorQueue.java index 4d75a46bbb203..ae74d5c72df88 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesCollectorQueue.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesCollectorQueue.java @@ -108,7 +108,7 @@ public int hashCode() { @Override protected boolean lessThan(Integer a, Integer b) { - return compare(a, b) > 0; // TODO reading a > b is true, this is a max heap? + return compare(a, b) > 0; // max heap } /** @@ -123,7 +123,7 @@ boolean isFull() { * the slot if the candidate is already in the queue or null if the candidate is not present. */ Integer compareCurrent() { - return map.get(new Slot(CANDIDATE_SLOT)); // TODO reading this check the slot/bucket? of the current value + return map.get(new Slot(CANDIDATE_SLOT)); } /** @@ -152,7 +152,7 @@ long getDocCount(int slot) { */ private void copyCurrent(int slot, long value) { for (int i = 0; i < arrays.length; i++) { - arrays[i].copyCurrent(slot); // TODO reading valueSource knows current value, set the value to this slot/index + arrays[i].copyCurrent(slot); } docCounts = bigArrays.grow(docCounts, slot + 1); docCounts.set(slot, value); @@ -204,7 +204,7 @@ boolean equals(int slot1, int slot2) { int hashCode(int slot) { int result = 1; for (int i = 0; i < arrays.length; i++) { - result = 31 * result + // TODO reading why 31 here? For each array, it multiplies the running result by 31. Multiplying by a prime number like 31 helps distribute the hash codes more evenly. + result = 31 * result + (slot == CANDIDATE_SLOT ? arrays[i].hashCodeCurrent() : arrays[i].hashCode(slot)); @@ -256,7 +256,7 @@ LeafBucketCollector getLeafCollector(Comparable forceLeadSourceValue, LeafReader int last = arrays.length - 1; LeafBucketCollector collector = in; while (last > 0) { - collector = arrays[last--].getLeafCollector(context, collector); // TODO reading the pass-in collect will work after current + collector = arrays[last--].getLeafCollector(context, collector); } if (forceLeadSourceValue != null) { diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java index ee3b1d252fa25..445bbb831370a 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java @@ -253,8 +253,7 @@ static boolean checkMatchAllOrRangeQuery(Query query, String fieldName) { @Override SortedDocsProducer createSortedDocsProducerOrNull(IndexReader reader, Query query) { query = extractQuery(query); - if (checkIfSortedDocsIsApplicable(reader, fieldType) == false - || checkMatchAllOrRangeQuery(query, fieldType.name()) == false) { + if (checkIfSortedDocsIsApplicable(reader, fieldType) == false || checkMatchAllOrRangeQuery(query, fieldType.name()) == false) { return null; } final byte[] lowerPoint; diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/PointsSortedDocsProducer.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/PointsSortedDocsProducer.java index 391d810d6bb37..ae6bfeac05db1 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/PointsSortedDocsProducer.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/PointsSortedDocsProducer.java @@ -148,8 +148,9 @@ public void visit(int docID, byte[] packedValue) throws IOException { } long bucket = bucketFunction.applyAsLong(packedValue); - if (first == false && bucket != lastBucket) { // TODO reading process previous bucket when new bucket appears - if (processBucket(queue, context, bucketDocsBuilder.build().iterator(), lastBucket, builder) && + if (first == false && bucket != lastBucket) { // process previous bucket when new bucket appears + final DocIdSet docIdSet = bucketDocsBuilder.build(); + if (processBucket(queue, context, docIdSet.iterator(), lastBucket, builder) && // lower bucket is inclusive lowerBucket != lastBucket) { // this bucket does not have any competitive composite buckets, diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java index 8eabd6e552f10..fe0801d6d230e 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java @@ -185,8 +185,7 @@ protected boolean checkIfSortedDocsIsApplicable(IndexReader reader, MappedFieldT return false; } - if (reader.hasDeletions() - && (reader.numDocs() == 0 || (double) reader.numDocs() / (double) reader.maxDoc() < 0.5)) { + if (reader.hasDeletions() && (reader.numDocs() == 0 || (double) reader.numDocs() / (double) reader.maxDoc() < 0.5)) { // do not use the index if it has more than 50% of deleted docs return false; } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SortedDocsProducer.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SortedDocsProducer.java index 0c9b1b945f4cf..95d3ecad31669 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SortedDocsProducer.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SortedDocsProducer.java @@ -91,7 +91,7 @@ protected boolean processBucket( @Override public void collect(int doc, long bucket) throws IOException { hasCollected[0] = true; - long docCount = docCountProvider.getDocCount(doc); // TODO reading _doc_count can be >1 + long docCount = docCountProvider.getDocCount(doc); if (queue.addIfCompetitive(docCount)) { topCompositeCollected[0]++; if (adder != null && doc != lastDoc) { // TODO reading why doc can be == lastDoc? @@ -109,11 +109,10 @@ public void collect(int doc, long bucket) throws IOException { } }; - final LeafBucketCollector collector = queue.getLeafCollector(leadSourceBucket, context, queueCollector); - final Bits liveDocs = context.reader().getLiveDocs(); + final LeafBucketCollector collector = queue.getLeafCollector(leadSourceBucket, context, queueCollector); while (iterator.nextDoc() != DocIdSetIterator.NO_MORE_DOCS) { - if (liveDocs == null || liveDocs.get(iterator.docID())) { // TODO reading doc exists + if (liveDocs == null || liveDocs.get(iterator.docID())) { collector.collect(iterator.docID()); } } 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 e8aab0b143108..dda053af78b30 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 @@ -441,12 +441,10 @@ public void testUnmappedMissing() throws IOException { final DateFieldMapper.DateFieldType fieldType = new DateFieldMapper.DateFieldType("date_field"); - testCase(aggregation, DEFAULT_QUERY, iw -> {}, - (Consumer) histogram -> { - assertEquals(0, histogram.getBuckets().size()); - assertFalse(AggregationInspectionHelper.hasValue(histogram)); - }, - fieldType); + testCase(aggregation, DEFAULT_QUERY, iw -> {}, (Consumer) histogram -> { + assertEquals(0, histogram.getBuckets().size()); + assertFalse(AggregationInspectionHelper.hasValue(histogram)); + }, fieldType); } public void testIntervalYear() throws IOException {