resolved PR comments from expani

vinaykpud · vinaykpud · commit b025fef7abef · 2026-04-12T18:56:32.000Z
Signed-off-by: Vinay Krishna Pudyodu &lt;vinkrish.neo@gmail.com&gt;
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationMetadata.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationMetadata.java
@@ -20,6 +20,9 @@
  *
  * <p>A multi-level aggregation tree (e.g., terms → terms → avg) produces
  * multiple metadata instances — one per distinct GROUP BY key set.
+ *
+ * <p>TODO: Add tree structure (parent-child links between granularity levels) and pipeline
+ * aggregation support.
  */
 public class AggregationMetadata {
 
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationMetadataBuilder.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationMetadataBuilder.java
@@ -12,6 +12,7 @@
 import org.apache.calcite.rel.core.AggregateCall;
 import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeField;
 import org.apache.calcite.sql.fun.SqlStdOperatorTable;
 import org.apache.calcite.sql.type.SqlTypeName;
 import org.apache.calcite.util.ImmutableBitSet;
@@ -103,7 +104,7 @@ public AggregationMetadata build(RelDataType inputRowType, RelDataTypeFactory ty
         List<Integer> allGroupIndices = new ArrayList<>();
         List<String> allGroupFieldNames = new ArrayList<>();
         for (GroupingInfo g : groupings) {
-            allGroupIndices.addAll(g.resolveIndices(inputRowType));
+            allGroupIndices.addAll(resolveFieldIndices(g, inputRowType));
             allGroupFieldNames.addAll(g.getFieldNames());
         }
 
@@ -152,4 +153,25 @@ public AggregationMetadata build(RelDataType inputRowType, RelDataTypeFactory ty
 
         return new AggregationMetadata(ImmutableBitSet.of(allGroupIndices), allGroupFieldNames, allCalls, allFieldNames, bucketOrders);
     }
+
+    /**
+     * Resolves field-based grouping names to column indices in the input schema.
+     *
+     * @param grouping the grouping info containing field names
+     * @param inputRowType the schema before aggregation
+     * @return column indices for each field name
+     * @throws ConversionException if a field name is not found in the schema
+     */
+    private static List<Integer> resolveFieldIndices(GroupingInfo grouping, RelDataType inputRowType) throws ConversionException {
+        List<String> fieldNames = grouping.getFieldNames();
+        List<Integer> indices = new ArrayList<>(fieldNames.size());
+        for (String name : fieldNames) {
+            RelDataTypeField field = inputRowType.getField(name, false, false);
+            if (field == null) {
+                throw new ConversionException("Group-by field '" + name + "' not found in schema");
+            }
+            indices.add(field.getIndex());
+        }
+        return indices;
+    }
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationRegistry.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationRegistry.java
@@ -22,7 +22,7 @@
  */
 public class AggregationRegistry {
 
-    private final Map<Class<? extends AggregationBuilder>, AggregationType<?>> translators = new HashMap<>();
+    private final Map<Class<? extends AggregationBuilder>, AggregationTranslator<?>> translators = new HashMap<>();
 
     /** Creates an empty registry. */
     public AggregationRegistry() {}
@@ -32,7 +32,7 @@ public AggregationRegistry() {}
      *
      * @param translator the translator to register
      */
-    public void register(AggregationType<?> translator) {
+    public void register(AggregationTranslator<?> translator) {
         translators.put(translator.getAggregationType(), translator);
     }
 
@@ -43,7 +43,7 @@ public void register(AggregationType<?> translator) {
      * @param aggClass the aggregation builder class
      * @return the translator, or null
      */
-    public AggregationType<?> get(Class<? extends AggregationBuilder> aggClass) {
+    public AggregationTranslator<?> get(Class<? extends AggregationBuilder> aggClass) {
         return translators.get(aggClass);
     }
 
@@ -55,7 +55,7 @@ public AggregationType<?> get(Class<? extends AggregationBuilder> aggClass) {
      */
     @SuppressWarnings("unchecked")
     public <T extends AggregationBuilder> MetricTranslator<T> getMetric(Class<? extends AggregationBuilder> aggClass) {
-        AggregationType<?> translator = translators.get(aggClass);
+        AggregationTranslator<?> translator = translators.get(aggClass);
         return translator instanceof MetricTranslator ? (MetricTranslator<T>) translator : null;
     }
 
@@ -67,7 +67,7 @@ public <T extends AggregationBuilder> MetricTranslator<T> getMetric(Class<? exte
      */
     @SuppressWarnings("unchecked")
     public <T extends AggregationBuilder> BucketTranslator<T> getBucket(Class<? extends AggregationBuilder> aggClass) {
-        AggregationType<?> translator = translators.get(aggClass);
+        AggregationTranslator<?> translator = translators.get(aggClass);
         return translator instanceof BucketTranslator ? (BucketTranslator<T>) translator : null;
     }
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationTranslator.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationTranslator.java
@@ -15,7 +15,7 @@
  * Provides type identification for the {@link AggregationRegistry}.
  * Bucket and metric subtypes define their own contracts.
  */
-public interface AggregationType<T extends AggregationBuilder> {
+public interface AggregationTranslator<T extends AggregationBuilder> {
 
     /** Returns the concrete AggregationBuilder class this type handles. */
     Class<T> getAggregationType();
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationTreeWalker.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationTreeWalker.java
@@ -75,7 +75,7 @@ private void walkRecursive(
         RelDataType rowType
     ) throws ConversionException {
         for (AggregationBuilder aggBuilder : aggs) {
-            AggregationType<?> type = registry.get(aggBuilder.getClass());
+            AggregationTranslator<?> type = registry.get(aggBuilder.getClass());
 
             if (type == null) {
                 throw new ConversionException("No translator registered for aggregation type: " + aggBuilder.getClass().getSimpleName());
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/FieldGrouping.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/FieldGrouping.java
@@ -8,11 +8,6 @@
 
 package org.opensearch.dsl.aggregation;
 
-import org.apache.calcite.rel.type.RelDataType;
-import org.apache.calcite.rel.type.RelDataTypeField;
-import org.opensearch.dsl.converter.ConversionException;
-
-import java.util.ArrayList;
 import java.util.List;
 
 /**
@@ -36,17 +31,4 @@ public FieldGrouping(List<String> fieldNames) {
     public List<String> getFieldNames() {
         return fieldNames;
     }
-
-    @Override
-    public List<Integer> resolveIndices(RelDataType inputRowType) throws ConversionException {
-        List<Integer> indices = new ArrayList<>(fieldNames.size());
-        for (String name : fieldNames) {
-            RelDataTypeField field = inputRowType.getField(name, false, false);
-            if (field == null) {
-                throw new ConversionException("Group-by field '" + name + "' not found in schema");
-            }
-            indices.add(field.getIndex());
-        }
-        return indices;
-    }
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/GroupingInfo.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/GroupingInfo.java
@@ -8,9 +8,6 @@
 
 package org.opensearch.dsl.aggregation;
 
-import org.apache.calcite.rel.type.RelDataType;
-import org.opensearch.dsl.converter.ConversionException;
-
 import java.util.List;
 
 /**
@@ -22,13 +19,4 @@ public interface GroupingInfo {
 
     /** Returns the logical field names this grouping contributes. */
     List<String> getFieldNames();
-
-    /**
-     * Resolves this grouping to column indices in the input schema.
-     *
-     * @param inputRowType the schema before aggregation
-     * @return column indices for the GROUP BY bit set
-     * @throws ConversionException if field lookup fails
-     */
-    List<Integer> resolveIndices(RelDataType inputRowType) throws ConversionException;
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/bucket/BucketTranslator.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/bucket/BucketTranslator.java
@@ -8,21 +8,20 @@
 
 package org.opensearch.dsl.aggregation.bucket;
 
-import org.opensearch.dsl.aggregation.AggregationType;
+import org.opensearch.dsl.aggregation.AggregationTranslator;
 import org.opensearch.dsl.aggregation.GroupingInfo;
 import org.opensearch.dsl.result.BucketEntry;
 import org.opensearch.search.aggregations.AggregationBuilder;
 import org.opensearch.search.aggregations.BucketOrder;
 import org.opensearch.search.aggregations.InternalAggregation;
 
 import java.util.Collection;
-import java.util.List;
 
 /**
  * Translates a bucket aggregation (terms, multi_terms, etc.) to a {@link GroupingInfo}
  * for GROUP BY resolution, and converts results back to InternalAggregation for response building.
  */
-public interface BucketTranslator<T extends AggregationBuilder> extends AggregationType<T> {
+public interface BucketTranslator<T extends AggregationBuilder> extends AggregationTranslator<T> {
 
     /**
      * Returns the grouping contribution for this bucket.
@@ -55,5 +54,5 @@ public interface BucketTranslator<T extends AggregationBuilder> extends Aggregat
      * @param buckets the bucket entries with keys, doc counts, and sub-aggs
      * @return the InternalAggregation
      */
-    InternalAggregation toBucketAggregation(T agg, List<BucketEntry> buckets);
+    InternalAggregation toBucketAggregation(T agg, Iterable<BucketEntry> buckets);
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/bucket/TermsBucketTranslator.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/bucket/TermsBucketTranslator.java
@@ -50,7 +50,7 @@ public BucketOrder getBucketOrder(TermsAggregationBuilder agg) {
 
     // TODO: implement response conversion
     @Override
-    public InternalAggregation toBucketAggregation(TermsAggregationBuilder agg, List<BucketEntry> buckets) {
+    public InternalAggregation toBucketAggregation(TermsAggregationBuilder agg, Iterable<BucketEntry> buckets) {
         throw new UnsupportedOperationException("toBucketAggregation not yet implemented");
     }
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/metric/MetricTranslator.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/metric/MetricTranslator.java
@@ -10,7 +10,7 @@
 
 import org.apache.calcite.rel.core.AggregateCall;
 import org.apache.calcite.rel.type.RelDataType;
-import org.opensearch.dsl.aggregation.AggregationType;
+import org.opensearch.dsl.aggregation.AggregationTranslator;
 import org.opensearch.dsl.converter.ConversionException;
 import org.opensearch.search.aggregations.AggregationBuilder;
 import org.opensearch.search.aggregations.InternalAggregation;
@@ -19,7 +19,7 @@
  * Translates a metric aggregation (AVG, SUM, MIN, MAX, etc.) to a Calcite AggregateCall,
  * and converts raw result values back to OpenSearch InternalAggregation for response building.
  */
-public interface MetricTranslator<T extends AggregationBuilder> extends AggregationType<T> {
+public interface MetricTranslator<T extends AggregationBuilder> extends AggregationTranslator<T> {
 
     /**
      * Converts the metric aggregation to a Calcite AggregateCall.
@@ -39,6 +39,9 @@ public interface MetricTranslator<T extends AggregationBuilder> extends Aggregat
      */
     String getAggregateFieldName(T agg);
 
+    // TODO: Revisit signature — accept a stream/iterator of <String,Object> for bulk conversion
+    // to avoid per-row virtual dispatch overhead, and use Arrow-native types once Analytics Core
+    // exposes them.
     /**
      * Converts a raw result value from execution into an OpenSearch InternalAggregation.
      *
diff --git a/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/aggregation/AggregationMetadataBuilderTests.java b/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/aggregation/AggregationMetadataBuilderTests.java
@@ -0,0 +1,54 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.dsl.aggregation;
+
+import org.opensearch.dsl.TestUtils;
+import org.opensearch.dsl.converter.ConversionContext;
+import org.opensearch.dsl.converter.ConversionException;
+import org.opensearch.test.OpenSearchTestCase;
+
+import java.util.List;
+
+public class AggregationMetadataBuilderTests extends OpenSearchTestCase {
+
+    private final ConversionContext ctx = TestUtils.createContext();
+
+    public void testResolvesFieldGroupingToCorrectIndex() throws ConversionException {
+        AggregationMetadataBuilder builder = new AggregationMetadataBuilder();
+        // brand is the 3rd field (index 2) in TestUtils schema: name, price, brand, rating
+        builder.addGrouping(new FieldGrouping(List.of("brand")));
+        builder.requestImplicitCount();
+
+        AggregationMetadata metadata = builder.build(ctx.getRowType(), ctx.getCluster().getTypeFactory());
+
+        assertTrue(metadata.getGroupByBitSet().get(2));
+        assertEquals(1, metadata.getGroupByBitSet().cardinality());
+    }
+
+    public void testResolvesMultipleFieldGroupings() throws ConversionException {
+        AggregationMetadataBuilder builder = new AggregationMetadataBuilder();
+        builder.addGrouping(new FieldGrouping(List.of("brand")));
+        builder.addGrouping(new FieldGrouping(List.of("name")));
+        builder.requestImplicitCount();
+
+        AggregationMetadata metadata = builder.build(ctx.getRowType(), ctx.getCluster().getTypeFactory());
+
+        assertTrue(metadata.getGroupByBitSet().get(0)); // name is index 0
+        assertTrue(metadata.getGroupByBitSet().get(2)); // brand is index 2
+        assertEquals(2, metadata.getGroupByBitSet().cardinality());
+    }
+
+    public void testThrowsForUnknownField() {
+        AggregationMetadataBuilder builder = new AggregationMetadataBuilder();
+        builder.addGrouping(new FieldGrouping(List.of("nonexistent")));
+        builder.requestImplicitCount();
+
+        expectThrows(ConversionException.class, () -> builder.build(ctx.getRowType(), ctx.getCluster().getTypeFactory()));
+    }
+}
diff --git a/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/aggregation/bucket/TermsBucketTranslatorTests.java b/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/aggregation/bucket/TermsBucketTranslatorTests.java
@@ -8,9 +8,6 @@
 
 package org.opensearch.dsl.aggregation.bucket;
 
-import org.opensearch.dsl.TestUtils;
-import org.opensearch.dsl.converter.ConversionContext;
-import org.opensearch.dsl.converter.ConversionException;
 import org.opensearch.search.aggregations.BucketOrder;
 import org.opensearch.search.aggregations.InternalOrder;
 import org.opensearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
@@ -22,19 +19,12 @@
 public class TermsBucketTranslatorTests extends OpenSearchTestCase {
 
     private final TermsBucketTranslator translator = new TermsBucketTranslator();
-    private final ConversionContext ctx = TestUtils.createContext();
     private final TermsAggregationBuilder brandAgg = new TermsAggregationBuilder("by_brand").field("brand");
 
     public void testGetGrouping() {
         assertEquals(List.of("brand"), translator.getGrouping(brandAgg).getFieldNames());
     }
 
-    public void testResolveGroupByIndices() throws ConversionException {
-        List<Integer> indices = translator.getGrouping(brandAgg).resolveIndices(ctx.getRowType());
-
-        assertEquals(List.of(2), indices); // brand is index 2
-    }
-
     public void testGetSubAggregations() {
         TermsAggregationBuilder aggWithSub = new TermsAggregationBuilder("by_brand").field("brand")
             .subAggregation(new AvgAggregationBuilder("avg_price").field("price"));
@@ -50,10 +40,11 @@ public void testReportsCorrectType() {
         assertEquals(TermsAggregationBuilder.class, translator.getAggregationType());
     }
 
-    public void testThrowsForUnknownField() {
+    public void testGroupingReturnsFieldNameAsIs() {
         TermsAggregationBuilder badAgg = new TermsAggregationBuilder("by_bad").field("nonexistent");
 
-        expectThrows(ConversionException.class, () -> translator.getGrouping(badAgg).resolveIndices(ctx.getRowType()));
+        // Translator just captures the field name; validation happens at build time in the builder
+        assertEquals(List.of("nonexistent"), translator.getGrouping(badAgg).getFieldNames());
     }
 
     public void testGetBucketOrderReturnsDefault() {

Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,9 @@`
`20`	`20`	`*`
`21`	`21`	`* <p>A multi-level aggregation tree (e.g., terms → terms → avg) produces`
`22`	`22`	`* multiple metadata instances — one per distinct GROUP BY key set.`
	`23`	`+ *`
	`24`	`+ * <p>TODO: Add tree structure (parent-child links between granularity levels) and pipeline`
	`25`	`+ * aggregation support.`
`23`	`26`	`*/`
`24`	`27`	`public class AggregationMetadata {`
`25`	`28`
Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,7 @@ public BucketOrder getBucketOrder(TermsAggregationBuilder agg) {`
`50`	`50`
`51`	`51`	`// TODO: implement response conversion`
`52`	`52`	`@Override`
`53`		`- public InternalAggregation toBucketAggregation(TermsAggregationBuilder agg, List<BucketEntry> buckets) {`
	`53`	`+ public InternalAggregation toBucketAggregation(TermsAggregationBuilder agg, Iterable<BucketEntry> buckets) {`
`54`	`54`	`throw new UnsupportedOperationException("toBucketAggregation not yet implemented");`
`55`	`55`	`}`
`56`	`56`	`}`