addressing comments

Signed-off-by: bharath-techie <[email protected]>
bharath-techie · Nov 27, 2024 · 7a81d2f · 7a81d2f
1 parent 6237b3b
commit 7a81d2f
Show file tree

Hide file tree

Showing 8 changed files with 49 additions and 29 deletions.
diff --git a/server/src/internalClusterTest/java/org/opensearch/index/mapper/StarTreeMapperIT.java b/server/src/internalClusterTest/java/org/opensearch/index/mapper/StarTreeMapperIT.java
@@ -102,6 +102,10 @@ private static XContentBuilder createMinimalTestMapping(boolean invalidDim, bool
                 .field("type", "keyword")
                 .field("doc_values", false)
                 .endObject()
+                .startObject("ip_no_dv")
+                .field("type", "ip")
+                .field("doc_values", false)
+                .endObject()
                 .startObject("ip")
                 .field("type", "ip")
                 .field("doc_values", true)
@@ -368,7 +372,7 @@ private XContentBuilder getMappingWithDuplicateFields(boolean isDuplicateDim, bo
 
     private static String getDim(boolean hasDocValues, boolean isWildCard) {
         if (hasDocValues) {
-            return random().nextBoolean() ? "numeric" : random().nextBoolean() ? "keyword" : "ip";
+            return random().nextBoolean() ? "numeric" : random().nextBoolean() ? "keyword" : "ip_no_dv";
         } else if (isWildCard) {
             return "wildcard";
         }

diff --git a/...n/java/org/opensearch/index/codec/composite/composite912/Composite912DocValuesWriter.java b/...n/java/org/opensearch/index/codec/composite/composite912/Composite912DocValuesWriter.java
@@ -33,10 +33,11 @@
 import org.opensearch.index.compositeindex.datacube.startree.builder.StarTreesBuilder;
 import org.opensearch.index.compositeindex.datacube.startree.index.CompositeIndexValues;
 import org.opensearch.index.compositeindex.datacube.startree.index.StarTreeValues;
+import org.opensearch.index.fielddata.IndexNumericFieldData;
+import org.opensearch.index.fielddata.plain.SortedSetOrdinalsIndexFieldData;
 import org.opensearch.index.mapper.CompositeMappedFieldType;
 import org.opensearch.index.mapper.DocCountFieldMapper;
-import org.opensearch.index.mapper.IpFieldMapper;
-import org.opensearch.index.mapper.KeywordFieldMapper;
+import org.opensearch.index.mapper.MappedFieldType;
 import org.opensearch.index.mapper.MapperService;
 
 import java.io.IOException;
@@ -45,6 +46,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -263,23 +265,38 @@ public SortedSetDocValues getSortedSet(FieldInfo field) {
                         return DocValues.emptySortedSet();
                     }
                 });
-            }
-            // TODO : change this logic to evaluate for sortedNumericField specifically
-            else {
+            } else if (isSortedNumericField(compositeField)) {
                 fieldProducerMap.put(compositeField, new EmptyDocValuesProducer() {
                     @Override
                     public SortedNumericDocValues getSortedNumeric(FieldInfo field) {
                         return DocValues.emptySortedNumeric();
                     }
                 });
+            } else {
+                throw new IllegalStateException(
+                    String.format(Locale.ROOT, "Unsupported DocValues field associated with the composite field : %s", compositeField)
+                );
             }
         }
         compositeFieldSet.remove(compositeField);
     }
 
     private boolean isSortedSetField(String field) {
-        return mapperService.fieldType(field) instanceof KeywordFieldMapper.KeywordFieldType
-            || mapperService.fieldType(field) instanceof IpFieldMapper.IpFieldType;
+        MappedFieldType ft = mapperService.fieldType(field);
+        assert ft.isAggregatable();
+        return ft.fielddataBuilder(
+            "",
+            () -> { throw new UnsupportedOperationException("SearchLookup not available"); }
+        ) instanceof SortedSetOrdinalsIndexFieldData.Builder;
+    }
+
+    private boolean isSortedNumericField(String field) {
+        MappedFieldType ft = mapperService.fieldType(field);
+        assert ft.isAggregatable();
+        return ft.fielddataBuilder(
+            "",
+            () -> { throw new UnsupportedOperationException("SearchLookup not available"); }
+        ) instanceof IndexNumericFieldData.Builder;
     }
 
     @Override
@@ -372,5 +389,4 @@ private static SegmentWriteState getSegmentWriteState(SegmentWriteState segmentW
             segmentWriteState.segmentSuffix
         );
     }
-
 }
diff --git a/server/src/main/java/org/opensearch/index/compositeindex/datacube/DimensionFactory.java b/server/src/main/java/org/opensearch/index/compositeindex/datacube/DimensionFactory.java
@@ -25,7 +25,7 @@
 
 import static org.opensearch.index.compositeindex.datacube.DateDimension.CALENDAR_INTERVALS;
 import static org.opensearch.index.compositeindex.datacube.IpDimension.IP;
-import static org.opensearch.index.compositeindex.datacube.KeywordDimension.KEYWORD;
+import static org.opensearch.index.compositeindex.datacube.OrdinalDimension.ORDINAL;
 
 /**
  * Dimension factory class mainly used to parse and create dimension from the mappings
@@ -45,8 +45,8 @@ public static Dimension parseAndCreateDimension(
                 return parseAndCreateDateDimension(name, dimensionMap, c);
             case NumericDimension.NUMERIC:
                 return new NumericDimension(name);
-            case KEYWORD:
-                return new KeywordDimension(name);
+            case ORDINAL:
+                return new OrdinalDimension(name);
             case IP:
                 return new IpDimension(name);
             default:
@@ -72,8 +72,8 @@ public static Dimension parseAndCreateDimension(
                 return parseAndCreateDateDimension(name, dimensionMap, c);
             case NUMERIC:
                 return new NumericDimension(name);
-            case KEYWORD:
-                return new KeywordDimension(name);
+            case ORDINAL:
+                return new OrdinalDimension(name);
             case IP:
                 return new IpDimension(name);
             default:

diff --git a/server/src/main/java/org/opensearch/index/compositeindex/datacube/DimensionType.java b/server/src/main/java/org/opensearch/index/compositeindex/datacube/DimensionType.java
@@ -30,10 +30,10 @@ public enum DimensionType {
     DATE,
 
     /**
-     * Represents a keyword dimension type.
-     * This is used for dimensions that contain keyword ordinals.
+     * Represents dimension types which uses ordinals.
+     * This is used for dimensions that contain sortedSet ordinals.
      */
-    KEYWORD,
+    ORDINAL,
 
     /**
      * Represents an IP dimension type.

diff --git a/...ositeindex/datacube/KeywordDimension.java → ...ositeindex/datacube/OrdinalDimension.java b/...ositeindex/datacube/KeywordDimension.java → ...ositeindex/datacube/OrdinalDimension.java
@@ -24,11 +24,11 @@
  * @opensearch.experimental
  */
 @ExperimentalApi
-public class KeywordDimension implements Dimension {
-    public static final String KEYWORD = "keyword";
+public class OrdinalDimension implements Dimension {
+    public static final String ORDINAL = "ordinal";
     private final String field;
 
-    public KeywordDimension(String field) {
+    public OrdinalDimension(String field) {
         this.field = field;
     }
 
@@ -62,7 +62,7 @@ public DocValuesType getDocValuesType() {
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject();
         builder.field(CompositeDataCubeFieldType.NAME, field);
-        builder.field(CompositeDataCubeFieldType.TYPE, KEYWORD);
+        builder.field(CompositeDataCubeFieldType.TYPE, ORDINAL);
         builder.endObject();
         return builder;
     }
@@ -71,7 +71,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
     public boolean equals(Object o) {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
-        KeywordDimension dimension = (KeywordDimension) o;
+        OrdinalDimension dimension = (OrdinalDimension) o;
         return Objects.equals(field, dimension.getField());
     }
 

diff --git a/server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java
@@ -259,7 +259,7 @@ public KeywordFieldMapper build(BuilderContext context) {
 
         @Override
         public Optional<DimensionType> getSupportedDataCubeDimensionType() {
-            return Optional.of(DimensionType.KEYWORD);
+            return Optional.of(DimensionType.ORDINAL);
         }
     }
 

diff --git a/...nsearch/index/compositeindex/datacube/startree/builder/StarTreeBuilderFlushFlowTests.java b/...nsearch/index/compositeindex/datacube/startree/builder/StarTreeBuilderFlushFlowTests.java
@@ -21,10 +21,10 @@
 import org.opensearch.index.codec.composite.composite912.Composite912DocValuesFormat;
 import org.opensearch.index.compositeindex.datacube.Dimension;
 import org.opensearch.index.compositeindex.datacube.IpDimension;
-import org.opensearch.index.compositeindex.datacube.KeywordDimension;
 import org.opensearch.index.compositeindex.datacube.Metric;
 import org.opensearch.index.compositeindex.datacube.MetricStat;
 import org.opensearch.index.compositeindex.datacube.NumericDimension;
+import org.opensearch.index.compositeindex.datacube.OrdinalDimension;
 import org.opensearch.index.compositeindex.datacube.startree.StarTreeDocument;
 import org.opensearch.index.compositeindex.datacube.startree.StarTreeField;
 import org.opensearch.index.compositeindex.datacube.startree.StarTreeFieldConfiguration;
@@ -533,8 +533,8 @@ private StarTreeField getStarTreeFieldWithMultipleMetrics() {
     }
 
     private StarTreeField getStarTreeFieldWithKeywordField(boolean isIp) {
-        Dimension d1 = isIp ? new IpDimension("field1") : new KeywordDimension("field1");
-        Dimension d2 = isIp ? new IpDimension("field3") : new KeywordDimension("field3");
+        Dimension d1 = isIp ? new IpDimension("field1") : new OrdinalDimension("field1");
+        Dimension d2 = isIp ? new IpDimension("field3") : new OrdinalDimension("field3");
         Metric m1 = new Metric("field2", List.of(MetricStat.SUM));
         Metric m2 = new Metric("field2", List.of(MetricStat.VALUE_COUNT));
         Metric m3 = new Metric("field2", List.of(MetricStat.AVG));

diff --git a/...rg/opensearch/index/compositeindex/datacube/startree/builder/StarTreeBuilderTestCase.java b/...rg/opensearch/index/compositeindex/datacube/startree/builder/StarTreeBuilderTestCase.java
@@ -33,10 +33,10 @@
 import org.opensearch.index.compositeindex.datacube.DateDimension;
 import org.opensearch.index.compositeindex.datacube.Dimension;
 import org.opensearch.index.compositeindex.datacube.IpDimension;
-import org.opensearch.index.compositeindex.datacube.KeywordDimension;
 import org.opensearch.index.compositeindex.datacube.Metric;
 import org.opensearch.index.compositeindex.datacube.MetricStat;
 import org.opensearch.index.compositeindex.datacube.NumericDimension;
+import org.opensearch.index.compositeindex.datacube.OrdinalDimension;
 import org.opensearch.index.compositeindex.datacube.startree.StarTreeDocument;
 import org.opensearch.index.compositeindex.datacube.startree.StarTreeField;
 import org.opensearch.index.compositeindex.datacube.startree.StarTreeFieldConfiguration;
@@ -354,8 +354,8 @@ protected StarTreeMetadata getStarTreeMetadata(
     }
 
     protected StarTreeField getStarTreeFieldWithKeywords(boolean ip) {
-        Dimension d1 = ip ? new IpDimension("field1") : new KeywordDimension("field1");
-        Dimension d2 = ip ? new IpDimension("field3") : new KeywordDimension("field3");
+        Dimension d1 = ip ? new IpDimension("field1") : new OrdinalDimension("field1");
+        Dimension d2 = ip ? new IpDimension("field3") : new OrdinalDimension("field3");
         Metric m1 = new Metric("field2", List.of(MetricStat.VALUE_COUNT, MetricStat.SUM));
         List<Dimension> dims = List.of(d1, d2);
         List<Metric> metrics = List.of(m1);