From 63e90b00eea48745bcb6cebf28ced1cc51bb9f4e Mon Sep 17 00:00:00 2001 From: bharath-techie Date: Wed, 6 Nov 2024 12:22:55 +0530 Subject: [PATCH] addressing comments Signed-off-by: bharath-techie --- .../lucene/index/DocValuesWriterWrapper.java | 4 +-- .../SortedNumericDocValuesWriterWrapper.java | 10 +++--- .../SortedSetDocValuesWriterWrapper.java | 10 +++--- .../Composite912DocValuesReader.java | 18 ---------- .../Composite912DocValuesWriter.java | 9 +---- .../datacube/KeywordDimension.java | 1 + .../startree/builder/BaseStarTreeBuilder.java | 6 ++-- .../startree/builder/StarTreesBuilder.java | 7 +--- .../startree/index/StarTreeValues.java | 33 ++++++++++++++++--- 9 files changed, 46 insertions(+), 52 deletions(-) diff --git a/server/src/main/java/org/apache/lucene/index/DocValuesWriterWrapper.java b/server/src/main/java/org/apache/lucene/index/DocValuesWriterWrapper.java index 3b8354c074bcb..5329bad776e43 100644 --- a/server/src/main/java/org/apache/lucene/index/DocValuesWriterWrapper.java +++ b/server/src/main/java/org/apache/lucene/index/DocValuesWriterWrapper.java @@ -13,6 +13,6 @@ /** * Base wrapper class for DocValuesWriter. */ -public abstract class DocValuesWriterWrapper { - public abstract T getDocValues(); +public interface DocValuesWriterWrapper { + T getDocValues(); } diff --git a/server/src/main/java/org/apache/lucene/index/SortedNumericDocValuesWriterWrapper.java b/server/src/main/java/org/apache/lucene/index/SortedNumericDocValuesWriterWrapper.java index 36fd29c7fd580..582e4c3f87f98 100644 --- a/server/src/main/java/org/apache/lucene/index/SortedNumericDocValuesWriterWrapper.java +++ b/server/src/main/java/org/apache/lucene/index/SortedNumericDocValuesWriterWrapper.java @@ -18,9 +18,9 @@ * * @opensearch.experimental */ -public class SortedNumericDocValuesWriterWrapper extends DocValuesWriterWrapper { +public class SortedNumericDocValuesWriterWrapper implements DocValuesWriterWrapper { - private final SortedNumericDocValuesWriter sortedNumericDocValuesWriter; + private final SortedNumericDocValuesWriter sortedNumericDocValuesWriterDelegate; /** * Sole constructor. Constructs a new {@link SortedNumericDocValuesWriterWrapper} instance. @@ -29,7 +29,7 @@ public class SortedNumericDocValuesWriterWrapper extends DocValuesWriterWrapper< * @param counter a counter for tracking memory usage */ public SortedNumericDocValuesWriterWrapper(FieldInfo fieldInfo, Counter counter) { - sortedNumericDocValuesWriter = new SortedNumericDocValuesWriter(fieldInfo, counter); + sortedNumericDocValuesWriterDelegate = new SortedNumericDocValuesWriter(fieldInfo, counter); } /** @@ -39,7 +39,7 @@ public SortedNumericDocValuesWriterWrapper(FieldInfo fieldInfo, Counter counter) * @param value the value to add */ public void addValue(int docID, long value) { - sortedNumericDocValuesWriter.addValue(docID, value); + sortedNumericDocValuesWriterDelegate.addValue(docID, value); } /** @@ -49,6 +49,6 @@ public void addValue(int docID, long value) { */ @Override public SortedNumericDocValues getDocValues() { - return sortedNumericDocValuesWriter.getDocValues(); + return sortedNumericDocValuesWriterDelegate.getDocValues(); } } diff --git a/server/src/main/java/org/apache/lucene/index/SortedSetDocValuesWriterWrapper.java b/server/src/main/java/org/apache/lucene/index/SortedSetDocValuesWriterWrapper.java index 229617b4a44b0..95aa242535e48 100644 --- a/server/src/main/java/org/apache/lucene/index/SortedSetDocValuesWriterWrapper.java +++ b/server/src/main/java/org/apache/lucene/index/SortedSetDocValuesWriterWrapper.java @@ -20,9 +20,9 @@ * * @opensearch.experimental */ -public class SortedSetDocValuesWriterWrapper extends DocValuesWriterWrapper { +public class SortedSetDocValuesWriterWrapper implements DocValuesWriterWrapper { - private final SortedSetDocValuesWriter sortedSetDocValuesWriterWrapper; + private final SortedSetDocValuesWriter sortedSetDocValuesWriterDelegate; /** * Sole constructor. Constructs a new {@link SortedSetDocValuesWriterWrapper} instance. @@ -33,7 +33,7 @@ public class SortedSetDocValuesWriterWrapper extends DocValuesWriterWrapper - * Sorted numeric field can be null for cases where the segment doesn't hold a particular value. - * - * @param sortedNumeric the sorted numeric doc values for a field - * @return empty sorted numeric values if the field is not present, else sortedNumeric - */ - public static SortedNumericDocValues getSortedNumericDocValues(SortedNumericDocValues sortedNumeric) { - return sortedNumeric == null ? DocValues.emptySortedNumeric() : sortedNumeric; - } - - public static SortedSetDocValues getSortedSetDocValues(SortedSetDocValues sortedSetDv) { - return sortedSetDv == null ? DocValues.emptySortedSet() : sortedSetDv; - } - } diff --git a/server/src/main/java/org/opensearch/index/codec/composite/composite912/Composite912DocValuesWriter.java b/server/src/main/java/org/opensearch/index/codec/composite/composite912/Composite912DocValuesWriter.java index da64b8bad2030..747f51b4721dc 100644 --- a/server/src/main/java/org/opensearch/index/codec/composite/composite912/Composite912DocValuesWriter.java +++ b/server/src/main/java/org/opensearch/index/codec/composite/composite912/Composite912DocValuesWriter.java @@ -339,14 +339,7 @@ private void mergeStarTreeFields(MergeState mergeState) throws IOException { } } try (StarTreesBuilder starTreesBuilder = new StarTreesBuilder(state, mapperService, fieldNumberAcrossCompositeFields)) { - starTreesBuilder.buildDuringMerge( - metaOut, - dataOut, - starTreeSubsPerField, - compositeDocValuesConsumer, - mergeState, - fieldDocIdSetIteratorMap - ); + starTreesBuilder.buildDuringMerge(metaOut, dataOut, starTreeSubsPerField, compositeDocValuesConsumer); } } diff --git a/server/src/main/java/org/opensearch/index/compositeindex/datacube/KeywordDimension.java b/server/src/main/java/org/opensearch/index/compositeindex/datacube/KeywordDimension.java index 60135244e2cc4..58e248fd548d6 100644 --- a/server/src/main/java/org/opensearch/index/compositeindex/datacube/KeywordDimension.java +++ b/server/src/main/java/org/opensearch/index/compositeindex/datacube/KeywordDimension.java @@ -44,6 +44,7 @@ public int getNumSubDimensions() { @Override public void setDimensionValues(Long value, Consumer dimSetter) { + // This will set the keyword dimension value's ordinal dimSetter.accept(value); } diff --git a/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/BaseStarTreeBuilder.java b/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/BaseStarTreeBuilder.java index a6ef1eacd8a13..cf36f2d7d4126 100644 --- a/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/BaseStarTreeBuilder.java +++ b/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/BaseStarTreeBuilder.java @@ -103,11 +103,11 @@ public abstract class BaseStarTreeBuilder implements StarTreeBuilder { private final IndexOutput metaOut; private final IndexOutput dataOut; private final Counter bytesUsed = Counter.newCounter(); - Map flushSortedSetDocValuesMap = new HashMap<>(); + private Map flushSortedSetDocValuesMap = new HashMap<>(); // Maintains list of sortedSetDocValues for each star tree dimension field across segments during merge - Map> mergeSortedSetDimensionsMap = new HashMap<>(); + private Map> mergeSortedSetDimensionsMap = new HashMap<>(); // Maintains ordinalMap for each star tree dimension field during merge - Map mergeSortedSetDimensionsOrdinalMap = new HashMap<>(); + private Map mergeSortedSetDimensionsOrdinalMap = new HashMap<>(); // This should be true for merge flows protected boolean isMerge = false; diff --git a/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/StarTreesBuilder.java b/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/StarTreesBuilder.java index 1b811e2aabcb9..3d1a780c1c7ef 100644 --- a/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/StarTreesBuilder.java +++ b/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/StarTreesBuilder.java @@ -12,9 +12,7 @@ import org.apache.logging.log4j.Logger; import org.apache.lucene.codecs.DocValuesConsumer; import org.apache.lucene.codecs.DocValuesProducer; -import org.apache.lucene.index.MergeState; import org.apache.lucene.index.SegmentWriteState; -import org.apache.lucene.index.SortedSetDocValues; import org.apache.lucene.store.IndexOutput; import org.opensearch.common.annotation.ExperimentalApi; import org.opensearch.index.compositeindex.datacube.startree.StarTreeField; @@ -112,15 +110,12 @@ public void close() throws IOException { * @param dataOut an IndexInput for star-tree data * @param starTreeValuesSubsPerField starTreeValuesSubs per field * @param starTreeDocValuesConsumer a consumer to write star-tree doc values - * @param fieldDocIdSetIteratorMap */ public void buildDuringMerge( IndexOutput metaOut, IndexOutput dataOut, final Map> starTreeValuesSubsPerField, - DocValuesConsumer starTreeDocValuesConsumer, - MergeState mergeState, - Map fieldDocIdSetIteratorMap + DocValuesConsumer starTreeDocValuesConsumer ) throws IOException { logger.debug("Starting merge of {} star-trees with star-tree fields", starTreeValuesSubsPerField.size()); long startTime = System.currentTimeMillis(); diff --git a/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/index/StarTreeValues.java b/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/index/StarTreeValues.java index 361ca0a919fe1..6a13e6e789f3a 100644 --- a/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/index/StarTreeValues.java +++ b/server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/index/StarTreeValues.java @@ -9,6 +9,7 @@ package org.opensearch.index.compositeindex.datacube.startree.index; import org.apache.lucene.codecs.DocValuesProducer; +import org.apache.lucene.index.DocValues; import org.apache.lucene.index.DocValuesType; import org.apache.lucene.index.FieldInfo; import org.apache.lucene.index.SegmentReadState; @@ -16,7 +17,6 @@ import org.apache.lucene.index.SortedSetDocValues; import org.apache.lucene.store.IndexInput; import org.opensearch.common.annotation.ExperimentalApi; -import org.opensearch.index.codec.composite.composite912.Composite912DocValuesReader; import org.opensearch.index.compositeindex.CompositeIndexMetadata; import org.opensearch.index.compositeindex.datacube.Dimension; import org.opensearch.index.compositeindex.datacube.Metric; @@ -39,7 +39,6 @@ import java.util.Set; import java.util.function.Supplier; -import static org.opensearch.index.codec.composite.composite912.Composite912DocValuesReader.getSortedNumericDocValues; import static org.opensearch.index.compositeindex.CompositeIndexConstants.SEGMENT_DOCS_COUNT; import static org.opensearch.index.compositeindex.CompositeIndexConstants.STAR_TREE_DOCS_COUNT; import static org.opensearch.index.compositeindex.datacube.startree.utils.StarTreeUtils.fullyQualifiedFieldNameForStarTreeDimensionsDocValues; @@ -174,9 +173,7 @@ public StarTreeValues( assert dimensionfieldInfo != null; if (dimensionfieldInfo.getDocValuesType().equals(DocValuesType.SORTED_SET)) { SortedSetDocValues dimensionSortedSetDocValues = compositeDocValuesProducer.getSortedSet(dimensionfieldInfo); - return new SortedSetStarTreeValuesIterator( - Composite912DocValuesReader.getSortedSetDocValues(dimensionSortedSetDocValues) - ); + return new SortedSetStarTreeValuesIterator(getSortedSetDocValues(dimensionSortedSetDocValues)); } else { SortedNumericDocValues dimensionSortedNumericDocValues = compositeDocValuesProducer.getSortedNumeric( dimensionfieldInfo @@ -291,4 +288,30 @@ public StarTreeValuesIterator getMetricValuesIterator(String fullyQualifiedMetri public int getStarTreeDocumentCount() { return starTreeMetadata.getStarTreeDocCount(); } + + /** + * Returns the sorted numeric doc values for the given sorted numeric field. + * If the sorted numeric field is null, it returns an empty doc id set iterator. + *

+ * Sorted numeric field can be null for cases where the segment doesn't hold a particular value. + * + * @param sortedNumeric the sorted numeric doc values for a field + * @return empty sorted numeric values if the field is not present, else sortedNumeric + */ + static SortedNumericDocValues getSortedNumericDocValues(SortedNumericDocValues sortedNumeric) { + return sortedNumeric == null ? DocValues.emptySortedNumeric() : sortedNumeric; + } + + /** + * Returns the sortedSet doc values for the given sortedSet field. + * If the sortedSet field is null, it returns an empty doc id set iterator. + *

+ * SortedSet field can be null for cases where the segment doesn't hold a particular value. + * + * @param sortedSetDv the sortedSet doc values for a field + * @return empty sortedSet values if the field is not present, else sortedSetDv + */ + static SortedSetDocValues getSortedSetDocValues(SortedSetDocValues sortedSetDv) { + return sortedSetDv == null ? DocValues.emptySortedSet() : sortedSetDv; + } }