addressing comments

Signed-off-by: Sarthak Aggarwal <[email protected]>
opensearch-project · Sep 2, 2024 · e13e400 · e13e400
1 parent 4aec8d2
commit e13e400
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 15 deletions.
diff --git a/...ain/java/org/opensearch/index/codec/composite/composite99/Composite99DocValuesWriter.java b/...ain/java/org/opensearch/index/codec/composite/composite99/Composite99DocValuesWriter.java
@@ -68,7 +68,7 @@ public class Composite99DocValuesWriter extends DocValuesConsumer {
     public IndexOutput metaOut;
     private final Set<String> segmentFieldSet;
     private final boolean segmentHasCompositeFields;
-    private AtomicInteger fieldNumberAcrossStarTrees;
+    private final AtomicInteger fieldNumberAcrossCompositeFields;
 
     private final Map<String, DocValuesProducer> fieldProducerMap = new HashMap<>();
 
@@ -78,7 +78,7 @@ public Composite99DocValuesWriter(DocValuesConsumer delegate, SegmentWriteState
         this.delegate = delegate;
         this.state = segmentWriteState;
         this.mapperService = mapperService;
-        this.fieldNumberAcrossStarTrees = new AtomicInteger();
+        this.fieldNumberAcrossCompositeFields = new AtomicInteger();
         this.compositeMappedFieldTypes = mapperService.getCompositeFieldTypes();
         compositeFieldSet = new HashSet<>();
         segmentFieldSet = new HashSet<>();
@@ -223,7 +223,7 @@ private void createCompositeIndicesIfPossible(DocValuesProducer valuesProducer,
         if (compositeFieldSet.isEmpty()) {
             for (CompositeMappedFieldType mappedType : compositeMappedFieldTypes) {
                 if (mappedType instanceof StarTreeMapper.StarTreeFieldType) {
-                    try (StarTreesBuilder starTreesBuilder = new StarTreesBuilder(state, mapperService, fieldNumberAcrossStarTrees)) {
+                    try (StarTreesBuilder starTreesBuilder = new StarTreesBuilder(state, mapperService, fieldNumberAcrossCompositeFields)) {
                         starTreesBuilder.build(metaOut, dataOut, fieldProducerMap, composite99DocValuesConsumer);
                     }
                 }
@@ -312,7 +312,7 @@ private void mergeStarTreeFields(MergeState mergeState) throws IOException {
                 }
             }
         }
-        try (StarTreesBuilder starTreesBuilder = new StarTreesBuilder(state, mapperService, fieldNumberAcrossStarTrees)) {
+        try (StarTreesBuilder starTreesBuilder = new StarTreesBuilder(state, mapperService, fieldNumberAcrossCompositeFields)) {
             starTreesBuilder.buildDuringMerge(metaOut, dataOut, starTreeSubsPerField, composite99DocValuesConsumer);
         }
     }

diff --git a/...va/org/opensearch/index/compositeindex/datacube/startree/builder/BaseStarTreeBuilder.java b/...va/org/opensearch/index/compositeindex/datacube/startree/builder/BaseStarTreeBuilder.java
@@ -367,7 +367,7 @@ private void createSortedDocValuesIndices(DocValuesConsumer docValuesConsumer, A
                             throw new IllegalStateException("Unknown metric doc value type");
                     }
                 } catch (IllegalArgumentException e) {
-                    logger.info("could not parse the value, exiting creation of star tree");
+                    logger.error("could not parse the value, exiting creation of star tree");
                 }
             }
         }
@@ -671,7 +671,7 @@ private SequentialDocValuesIterator getIteratorForNumericField(
         String name
     ) throws IOException {
         if (fieldInfo == null) {
-            fieldInfo = getFieldInfo(name, DocValuesType.NUMERIC, 1);
+            fieldInfo = getFieldInfo(name, DocValuesType.NUMERIC);
         }
         SequentialDocValuesIterator sequentialDocValuesIterator;
         assert fieldProducerMap.containsKey(fieldInfo.name);
@@ -822,7 +822,7 @@ private StarTreeDocument createAggregatedDocs(InMemoryTreeNode node) throws IOEx
         StarTreeDocument aggregatedStarTreeDocument = null;
 
         // For leaf node
-        if (node.getChildren().isEmpty() && node.getChildStarNode() == null) {
+        if (!node.hasChild()) {
 
             if (node.getStartDocId() == node.getEndDocId() - 1) {
                 // If it has only one document, use it as the aggregated document

diff --git a/...ain/java/org/opensearch/index/compositeindex/datacube/startree/node/InMemoryTreeNode.java b/...ain/java/org/opensearch/index/compositeindex/datacube/startree/node/InMemoryTreeNode.java
@@ -119,11 +119,10 @@ public void setNodeType(byte nodeType) {
 
     public void addChildNode(InMemoryTreeNode childNode, Long dimensionValue) {
         if (childNode.getNodeType() == StarTreeNodeType.STAR.getValue()) {
-            assert this.childStarNode.get() == null;
             this.childStarNode.set(childNode);
         } else {
-            assert assertStarTreeChildOrder(childNode);
             this.children.put(dimensionValue, childNode);
+            assert assertStarTreeChildOrder(childNode);
         }
     }
 
@@ -158,6 +157,12 @@ private boolean assertStarTreeChildOrder(InMemoryTreeNode childNode) {
                 lastNode = entry.getValue();
             }
             assert lastNode.dimensionValue <= childNode.dimensionValue;
+        } else if (childNode.nodeType == StarTreeNodeType.NULL.getValue() && !this.children.isEmpty()) {
+            InMemoryTreeNode lastNode = null;
+            for (Map.Entry<Long, InMemoryTreeNode> entry : this.children.entrySet()) {
+                lastNode = entry.getValue();
+            }
+            assert lastNode.nodeType == StarTreeNodeType.NULL.getValue();
         }
         return true;
     }

diff --git a/.../opensearch/index/compositeindex/datacube/startree/utils/SequentialDocValuesIterator.java b/.../opensearch/index/compositeindex/datacube/startree/utils/SequentialDocValuesIterator.java
@@ -95,12 +95,7 @@ public Long value(int currentDocId) throws IOException {
             if (docId == DocIdSetIterator.NO_MORE_DOCS || docId != currentDocId) {
                 return null;
             }
-            if (docValue == null) {
-                docValue = sortedNumericDocValues.nextValue();
-            }
-            Long nextValue = docValue;
-            docValue = null;
-            return nextValue;
+            return sortedNumericDocValues.nextValue();
 
         } else {
             throw new IllegalStateException("Unsupported Iterator requested for SequentialDocValuesIterator");

diff --git a/...index/compositeindex/datacube/startree/fileformats/node/FixedLengthStarTreeNodeTests.java b/...index/compositeindex/datacube/startree/fileformats/node/FixedLengthStarTreeNodeTests.java
@@ -12,6 +12,7 @@
 import org.apache.lucene.store.IOContext;
 import org.apache.lucene.store.IndexInput;
 import org.apache.lucene.store.IndexOutput;
+import org.opensearch.index.compositeindex.datacube.startree.StarTreeTestUtils;
 import org.opensearch.index.compositeindex.datacube.startree.fileformats.StarTreeWriter;
 import org.opensearch.index.compositeindex.datacube.startree.fileformats.meta.StarTreeMetadata;
 import org.opensearch.index.compositeindex.datacube.startree.node.InMemoryTreeNode;
@@ -156,6 +157,26 @@ public void testGetChildrenIterator() throws IOException {
         assertEquals(starTreeNode.getNumChildren(), count);
     }
 
+    public void testChildrenOrder() throws IOException {
+        Iterator<FixedLengthStarTreeNode> iterator = starTreeNode.getChildrenIterator();
+        int count = 0;
+        while (iterator.hasNext()) {
+            FixedLengthStarTreeNode child = iterator.next();
+            count++;
+            if (count == 1) {
+                StarTreeTestUtils.assertStarTreeNode(child, starChild);
+            } else if (count == 2) {
+                StarTreeTestUtils.assertStarTreeNode(child, childWithMinus1);
+            } else if (count == starTreeNode.getNumChildren()) {
+                StarTreeTestUtils.assertStarTreeNode(child, nullChild);
+            } else {
+                StarTreeTestUtils.assertStarTreeNode(child, node.getChildren().get(child.getDimensionValue()));
+            }
+            assertNotNull(child);
+        }
+        assertEquals(starTreeNode.getNumChildren(), count);
+    }
+
     public void testGetChildForStarNode() throws IOException {
         // Assuming the first child is a star node in our test data
         FixedLengthStarTreeNode starNode = (FixedLengthStarTreeNode) starTreeNode.getChildStarNode();