fixed some suggestions from AI pr review

vinaykpud · vinaykpud · commit 09f793b430b0 · 2026-03-31T21:48:44.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/AggregationTreeWalker.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationTreeWalker.java
@@ -21,6 +21,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 
 /**
  * Recursively walks the DSL aggregation tree and produces one {@link AggregationMetadata}
@@ -76,12 +77,14 @@ private void walkRecursive(
         for (AggregationBuilder aggBuilder : aggs) {
             AggregationType<?> type = registry.get(aggBuilder.getClass());
 
-            if (type instanceof BucketTranslator) {
+            if (type == null) {
+                throw new ConversionException("No translator registered for aggregation type: " + aggBuilder.getClass().getSimpleName());
+            } else if (type instanceof BucketTranslator) {
                 handleBucket((BucketTranslator<AggregationBuilder>) type, aggBuilder, currentGroupings, granularities, rowType);
             } else if (type instanceof MetricTranslator) {
                 handleMetric((MetricTranslator<AggregationBuilder>) type, aggBuilder, currentGroupings, granularities, rowType);
             } else {
-                throw new ConversionException("Unsupported aggregation type: " + aggBuilder.getClass().getSimpleName());
+                throw new ConversionException("Unsupported aggregation translator kind: " + type.getClass().getSimpleName());
             }
         }
     }
@@ -144,6 +147,8 @@ private static String granularityKey(List<GroupingInfo> groupings) {
         if (groupings.isEmpty()) {
             return "";
         }
-        return groupings.stream().flatMap(g -> g.getFieldNames().stream()).collect(Collectors.joining(","));
+        return IntStream.range(0, groupings.size())
+            .mapToObj(i -> i + ":" + String.join(",", groupings.get(i).getFieldNames()))
+            .collect(Collectors.joining("|"));
     }
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/metric/AbstractMetricTranslator.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/metric/AbstractMetricTranslator.java
@@ -48,6 +48,7 @@ public AggregateCall toAggregateCall(T agg, RelDataType rowType) throws Conversi
             throw new ConversionException("Aggregation field '" + fieldName + "' not found in schema");
         }
 
+        // Calcite enforces the return type to be same as input type; eg: AVG int→double coercion happens in response layer.
         return AggregateCall.create(
             getAggFunction(),
             false,
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/converter/ProjectConverter.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/converter/ProjectConverter.java
@@ -61,14 +61,18 @@ private RelNode createProjection(RelNode input, String[] includes, String[] excl
         RelDataType rowType = input.getRowType();
         List<RexNode> projects = new ArrayList<>();
         List<String> fieldNames = new ArrayList<>();
+        Set<String> seen = new HashSet<>();
 
         if (includes != null && includes.length > 0) {
-            // Include mode: only listed fields
+            // Include mode: only listed fields, then apply excludes as post-filter
+            Set<String> excludeSet = (excludes != null && excludes.length > 0) ? buildExcludeSet(excludes, rowType) : Set.of();
             for (String pattern : includes) {
                 if (pattern.contains("*")) {
-                    resolveWildcard(pattern, rowType, rexBuilder, projects, fieldNames);
+                    resolveWildcard(pattern, rowType, rexBuilder, projects, fieldNames, seen, excludeSet);
                 } else {
-                    resolveField(pattern, rowType, rexBuilder, projects, fieldNames);
+                    if (!excludeSet.contains(pattern)) {
+                        resolveField(pattern, rowType, rexBuilder, projects, fieldNames, seen);
+                    }
                 }
             }
         } else {
@@ -85,11 +89,23 @@ private RelNode createProjection(RelNode input, String[] includes, String[] excl
         return LogicalProject.create(input, List.of(), projects, fieldNames);
     }
 
+    private static Pattern toWildcardRegex(String wildcardPattern) {
+        String[] parts = wildcardPattern.split("\\*", -1);
+        StringBuilder regex = new StringBuilder();
+        for (int i = 0; i < parts.length; i++) {
+            if (i > 0) {
+                regex.append(".*");
+            }
+            regex.append(Pattern.quote(parts[i]));
+        }
+        return Pattern.compile(regex.toString());
+    }
+
     private Set<String> buildExcludeSet(String[] excludes, RelDataType rowType) {
         Set<String> excludeSet = new HashSet<>();
         for (String pattern : excludes) {
             if (pattern.contains("*")) {
-                Pattern compiled = Pattern.compile(pattern.replace(".", "\\.").replace("*", ".*"));
+                Pattern compiled = toWildcardRegex(pattern);
                 for (RelDataTypeField field : rowType.getFieldList()) {
                     if (compiled.matcher(field.getName()).matches()) {
                         excludeSet.add(field.getName());
@@ -107,25 +123,35 @@ private void resolveWildcard(
         RelDataType rowType,
         RexBuilder rexBuilder,
         List<RexNode> projects,
-        List<String> fieldNames
+        List<String> fieldNames,
+        Set<String> seen,
+        Set<String> excludeSet
     ) {
-        Pattern compiled = Pattern.compile(pattern.replace(".", "\\.").replace("*", ".*"));
+        Pattern compiled = toWildcardRegex(pattern);
         for (RelDataTypeField field : rowType.getFieldList()) {
-            if (compiled.matcher(field.getName()).matches()) {
+            if (compiled.matcher(field.getName()).matches() && !excludeSet.contains(field.getName()) && seen.add(field.getName())) {
                 projects.add(rexBuilder.makeInputRef(field.getType(), field.getIndex()));
                 fieldNames.add(field.getName());
             }
         }
         // No error if nothing matches — consistent with OpenSearch core, which returns empty _source
     }
 
-    private void resolveField(String fieldName, RelDataType rowType, RexBuilder rexBuilder, List<RexNode> projects, List<String> fieldNames)
-        throws ConversionException {
+    private void resolveField(
+        String fieldName,
+        RelDataType rowType,
+        RexBuilder rexBuilder,
+        List<RexNode> projects,
+        List<String> fieldNames,
+        Set<String> seen
+    ) throws ConversionException {
         RelDataTypeField field = rowType.getField(fieldName, false, false);
         if (field == null) {
             throw new ConversionException("Field '" + fieldName + "' not found in schema");
         }
-        projects.add(rexBuilder.makeInputRef(field.getType(), field.getIndex()));
-        fieldNames.add(field.getName());
+        if (seen.add(field.getName())) {
+            projects.add(rexBuilder.makeInputRef(field.getType(), field.getIndex()));
+            fieldNames.add(field.getName());
+        }
     }
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/converter/SearchSourceConverter.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/converter/SearchSourceConverter.java
@@ -105,7 +105,8 @@ public QueryPlans convert(SearchSourceBuilder searchSource, String indexName) th
         QueryPlans.Builder builder = new QueryPlans.Builder();
 
         // Hits path: Scan → Filter → Project → Sort
-        if (size > 0 || !hasAggs) {
+        // size=0 skips hits — total doc count comes from analytics plugin metadata
+        if (size > 0) {
             RelNode hits = projectConverter.convert(base, ctx);
             hits = sortConverter.convert(hits, ctx);
             builder.add(new QueryPlans.QueryPlan(QueryPlans.Type.HITS, hits));
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/executor/QueryPlans.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/executor/QueryPlans.java
@@ -46,18 +46,6 @@ public record QueryPlan(Type type, RelNode relNode) {
             Objects.requireNonNull(type, "type must not be null");
             Objects.requireNonNull(relNode, "relNode must not be null");
         }
-
-        /** Returns what part of the response this plan produces. */
-        @Override
-        public Type type() {
-            return type;
-        }
-
-        /** Returns the Calcite logical plan to execute. */
-        @Override
-        public RelNode relNode() {
-            return relNode;
-        }
     }
 
     private final List<QueryPlan> plans;
@@ -106,11 +94,8 @@ public Builder add(QueryPlan plan) {
             return this;
         }
 
-        /** Builds the plans. At least one must have been added. */
+        /** Builds the plans. May be empty (e.g. size=0, no aggs — only metadata response). */
         public QueryPlans build() {
-            if (plans.isEmpty()) {
-                throw new IllegalStateException("QueryPlans must have at least one plan");
-            }
             return new QueryPlans(plans);
         }
     }
diff --git a/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/SearchResponseBuilder.java b/sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/SearchResponseBuilder.java
@@ -29,10 +29,14 @@ private SearchResponseBuilder() {}
      * @param convertTimeNanos time spent in DSL-to-RelNode conversion, in nanoseconds
      * @return a SearchResponse
      */
-    // TODO: planExecutor.execute() should return execution timing so we can add it here
-    // and construct tookInMillis = convertTime + executionTime + responseBuildTime.
+    // TODO: Analytics plugin should return execution metadata alongside Iterable<Object[]> rows:
+    //   - executionTimeNanos: query execution time (tookInMillis = convertTime + executionTime + responseBuildTime)
+    //   - totalDocCount: total matching documents for hits.total (similar to Lucene's TotalHitCountCollector)
+    //   - terminatedEarly: whether execution was terminated early
+    //   - timedOut: whether execution timed out
+    //   - shardInfo: total/successful/skipped/failed shard counts
     public static SearchResponse build(List<ExecutionResult> results, long convertTimeNanos) {
-        // TODO: populate hits and aggregations from results
+        // TODO: populate HITS and AGGREGATION plan types from results
         long tookInMillis = convertTimeNanos / 1_000_000;
         SearchHits hits = SearchHits.empty(true);
         SearchResponseSections sections = new SearchResponseSections(hits, null, null, false, null, null, 0);
diff --git a/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/aggregation/metric/MetricTranslatorTests.java b/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/aggregation/metric/MetricTranslatorTests.java
@@ -25,7 +25,9 @@ public class MetricTranslatorTests extends OpenSearchTestCase {
 
     public void testAvgTranslator() throws ConversionException {
         AvgMetricTranslator translator = new AvgMetricTranslator();
-        AggregateCall call = translator.toAggregateCall(new AvgAggregationBuilder("avg_price").field("price"), ctx.getRowType());
+        AggregateCall call = translator.toAggregateCall(
+            new AvgAggregationBuilder("avg_price")
+                .field("price"), ctx.getRowType());
 
         assertEquals(SqlKind.AVG, call.getAggregation().getKind());
         assertEquals("avg_price", call.getName());
@@ -35,23 +37,29 @@ public void testAvgTranslator() throws ConversionException {
 
     public void testSumTranslator() throws ConversionException {
         SumMetricTranslator translator = new SumMetricTranslator();
-        AggregateCall call = translator.toAggregateCall(new SumAggregationBuilder("total").field("price"), ctx.getRowType());
+        AggregateCall call = translator.toAggregateCall(
+            new SumAggregationBuilder("total")
+                .field("price"), ctx.getRowType());
 
         assertEquals(SqlKind.SUM, call.getAggregation().getKind());
         assertEquals("total", call.getName());
     }
 
     public void testMinTranslator() throws ConversionException {
         MinMetricTranslator translator = new MinMetricTranslator();
-        AggregateCall call = translator.toAggregateCall(new MinAggregationBuilder("min_price").field("price"), ctx.getRowType());
+        AggregateCall call = translator.toAggregateCall(
+            new MinAggregationBuilder("min_price")
+                .field("price"), ctx.getRowType());
 
         assertEquals(SqlKind.MIN, call.getAggregation().getKind());
         assertEquals("min_price", call.getName());
     }
 
     public void testMaxTranslator() throws ConversionException {
         MaxMetricTranslator translator = new MaxMetricTranslator();
-        AggregateCall call = translator.toAggregateCall(new MaxAggregationBuilder("max_price").field("price"), ctx.getRowType());
+        AggregateCall call = translator.toAggregateCall(
+            new MaxAggregationBuilder("max_price")
+                .field("price"), ctx.getRowType());
 
         assertEquals(SqlKind.MAX, call.getAggregation().getKind());
         assertEquals("max_price", call.getName());
@@ -62,7 +70,8 @@ public void testThrowsForUnknownField() {
 
         expectThrows(
             ConversionException.class,
-            () -> translator.toAggregateCall(new AvgAggregationBuilder("bad").field("nonexistent"), ctx.getRowType())
+            () -> translator.toAggregateCall(new AvgAggregationBuilder("bad")
+                .field("nonexistent"), ctx.getRowType())
         );
     }
 
diff --git a/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/converter/ProjectConverterTests.java b/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/converter/ProjectConverterTests.java
@@ -109,4 +109,55 @@ public void testWildcardNoMatchReturnsEmptyProjection() throws ConversionExcepti
         assertTrue(result instanceof LogicalProject);
         assertEquals(0, result.getRowType().getFieldCount());
     }
+
+    public void testIncludesWithExcludesAppliesPostFilter() throws ConversionException {
+        SearchSourceBuilder source = new SearchSourceBuilder().fetchSource(
+            new FetchSourceContext(true, new String[] { "name", "price", "brand" }, new String[] { "price" })
+        );
+        ConversionContext ctx = TestUtils.createContext(source);
+        RelNode result = converter.convert(scan, ctx);
+
+        assertTrue(result instanceof LogicalProject);
+        assertEquals(2, result.getRowType().getFieldCount());
+        assertEquals("name", result.getRowType().getFieldNames().get(0));
+        assertEquals("brand", result.getRowType().getFieldNames().get(1));
+    }
+
+    public void testWildcardIncludesWithExcludes() throws ConversionException {
+        // Include all fields matching "* ", exclude "rating"
+        SearchSourceBuilder source = new SearchSourceBuilder().fetchSource(
+            new FetchSourceContext(true, new String[] { "*" }, new String[] { "rating" })
+        );
+        ConversionContext ctx = TestUtils.createContext(source);
+        RelNode result = converter.convert(scan, ctx);
+
+        assertTrue(result instanceof LogicalProject);
+        assertEquals(3, result.getRowType().getFieldCount());
+        assertFalse(result.getRowType().getFieldNames().contains("rating"));
+    }
+
+    public void testOverlappingWildcardsDoNotProduceDuplicates() throws ConversionException {
+        // Both "n*" and "na*" match "name" — should only appear once
+        SearchSourceBuilder source = new SearchSourceBuilder().fetchSource(
+            new FetchSourceContext(true, new String[] { "n*", "na*" }, null)
+        );
+        ConversionContext ctx = TestUtils.createContext(source);
+        RelNode result = converter.convert(scan, ctx);
+
+        assertTrue(result instanceof LogicalProject);
+        assertEquals(1, result.getRowType().getFieldCount());
+        assertEquals("name", result.getRowType().getFieldNames().get(0));
+    }
+
+    public void testDuplicateExactFieldsProjectedOnce() throws ConversionException {
+        SearchSourceBuilder source = new SearchSourceBuilder().fetchSource(
+            new FetchSourceContext(true, new String[] { "name", "name" }, null)
+        );
+        ConversionContext ctx = TestUtils.createContext(source);
+        RelNode result = converter.convert(scan, ctx);
+
+        assertTrue(result instanceof LogicalProject);
+        assertEquals(1, result.getRowType().getFieldCount());
+        assertEquals("name", result.getRowType().getFieldNames().get(0));
+    }
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/converter/SearchSourceConverterTests.java b/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/converter/SearchSourceConverterTests.java
@@ -92,4 +92,14 @@ public void testNoAggsProducesOnlyHitsPlan() throws ConversionException {
         assertTrue(plans.has(QueryPlans.Type.HITS));
         assertFalse(plans.has(QueryPlans.Type.AGGREGATION));
     }
+
+    public void testSizeZeroNoAggsProducesNoPlans() throws ConversionException {
+        // size=0 with no aggs produces no plans — total doc count comes from analytics plugin metadata
+        SearchSourceBuilder source = new SearchSourceBuilder().size(0);
+        QueryPlans plans = converter.convert(source, "test-index");
+
+        assertEquals(0, plans.getAll().size());
+        assertFalse(plans.has(QueryPlans.Type.HITS));
+        assertFalse(plans.has(QueryPlans.Type.AGGREGATION));
+    }
 }
diff --git a/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/executor/QueryPlansTests.java b/sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/executor/QueryPlansTests.java
@@ -50,8 +50,10 @@ public void testGetReturnsMultiplePlansOfSameType() {
         assertEquals(2, plans.get(QueryPlans.Type.AGGREGATION).size());
     }
 
-    public void testBuilderThrowsOnEmpty() {
-        expectThrows(IllegalStateException.class, () -> new QueryPlans.Builder().build());
+    public void testBuilderAllowsEmpty() {
+        // Empty plans are valid (e.g. size=0, no aggs — metadata-only response)
+        QueryPlans plans = new QueryPlans.Builder().build();
+        assertEquals(0, plans.getAll().size());
     }
 
     public void testGetReturnsEmptyForMissingType() {

Original file line number	Diff line number	Diff line change
`@@ -21,6 +21,7 @@`
`21`	`21`	`import java.util.List;`
`22`	`22`	`import java.util.Map;`
`23`	`23`	`import java.util.stream.Collectors;`
	`24`	`+import java.util.stream.IntStream;`
`24`	`25`
`25`	`26`	`/**`
`26`	`27`	`* Recursively walks the DSL aggregation tree and produces one {@link AggregationMetadata}`
`@@ -76,12 +77,14 @@ private void walkRecursive(`
`76`	`77`	`for (AggregationBuilder aggBuilder : aggs) {`
`77`	`78`	`AggregationType<?> type = registry.get(aggBuilder.getClass());`
`78`	`79`
`79`		`- if (type instanceof BucketTranslator) {`
	`80`	`+ if (type == null) {`
	`81`	`+ throw new ConversionException("No translator registered for aggregation type: " + aggBuilder.getClass().getSimpleName());`
	`82`	`+ } else if (type instanceof BucketTranslator) {`
`80`	`83`	`handleBucket((BucketTranslator<AggregationBuilder>) type, aggBuilder, currentGroupings, granularities, rowType);`
`81`	`84`	`} else if (type instanceof MetricTranslator) {`
`82`	`85`	`handleMetric((MetricTranslator<AggregationBuilder>) type, aggBuilder, currentGroupings, granularities, rowType);`
`83`	`86`	`} else {`
`84`		`- throw new ConversionException("Unsupported aggregation type: " + aggBuilder.getClass().getSimpleName());`
	`87`	`+ throw new ConversionException("Unsupported aggregation translator kind: " + type.getClass().getSimpleName());`
`85`	`88`	`}`
`86`	`89`	`}`
`87`	`90`	`}`
`@@ -144,6 +147,8 @@ private static String granularityKey(List<GroupingInfo> groupings) {`
`144`	`147`	`if (groupings.isEmpty()) {`
`145`	`148`	`return "";`
`146`	`149`	`}`
`147`		`- return groupings.stream().flatMap(g -> g.getFieldNames().stream()).collect(Collectors.joining(","));`
	`150`	`+ return IntStream.range(0, groupings.size())`
	`151`	`+ .mapToObj(i -> i + ":" + String.join(",", groupings.get(i).getFieldNames()))`
	`152`	`+ .collect(Collectors.joining("\|"));`
`148`	`153`	`}`
`149`	`154`	`}`
Original file line number	Diff line number	Diff line change
`@@ -48,6 +48,7 @@ public AggregateCall toAggregateCall(T agg, RelDataType rowType) throws Conversi`
`48`	`48`	`throw new ConversionException("Aggregation field '" + fieldName + "' not found in schema");`
`49`	`49`	`}`
`50`	`50`
	`51`	`+ // Calcite enforces the return type to be same as input type; eg: AVG int→double coercion happens in response layer.`
`51`	`52`	`return AggregateCall.create(`
`52`	`53`	`getAggFunction(),`
`53`	`54`	`false,`
Original file line number	Diff line number	Diff line change
`@@ -46,18 +46,6 @@ public record QueryPlan(Type type, RelNode relNode) {`
`46`	`46`	`Objects.requireNonNull(type, "type must not be null");`
`47`	`47`	`Objects.requireNonNull(relNode, "relNode must not be null");`
`48`	`48`	`}`
`49`		`-`
`50`		`- /** Returns what part of the response this plan produces. */`
`51`		`- @Override`
`52`		`- public Type type() {`
`53`		`- return type;`
`54`		`- }`
`55`		`-`
`56`		`- /** Returns the Calcite logical plan to execute. */`
`57`		`- @Override`
`58`		`- public RelNode relNode() {`
`59`		`- return relNode;`
`60`		`- }`
`61`	`49`	`}`
`62`	`50`
`63`	`51`	`private final List<QueryPlan> plans;`
`@@ -106,11 +94,8 @@ public Builder add(QueryPlan plan) {`
`106`	`94`	`return this;`
`107`	`95`	`}`
`108`	`96`
`109`		`- /** Builds the plans. At least one must have been added. */`
	`97`	`+ /** Builds the plans. May be empty (e.g. size=0, no aggs — only metadata response). */`
`110`	`98`	`public QueryPlans build() {`
`111`		`- if (plans.isEmpty()) {`
`112`		`- throw new IllegalStateException("QueryPlans must have at least one plan");`
`113`		`- }`
`114`	`99`	`return new QueryPlans(plans);`
`115`	`100`	`}`
`116`	`101`	`}`
Original file line number	Diff line number	Diff line change
`@@ -50,8 +50,10 @@ public void testGetReturnsMultiplePlansOfSameType() {`
`50`	`50`	`assertEquals(2, plans.get(QueryPlans.Type.AGGREGATION).size());`
`51`	`51`	`}`
`52`	`52`
`53`		`- public void testBuilderThrowsOnEmpty() {`
`54`		`- expectThrows(IllegalStateException.class, () -> new QueryPlans.Builder().build());`
	`53`	`+ public void testBuilderAllowsEmpty() {`
	`54`	`+ // Empty plans are valid (e.g. size=0, no aggs — metadata-only response)`
	`55`	`+ QueryPlans plans = new QueryPlans.Builder().build();`
	`56`	`+ assertEquals(0, plans.getAll().size());`
`55`	`57`	`}`
`56`	`58`
`57`	`59`	`public void testGetReturnsEmptyForMissingType() {`