Build aggregation responses from execution results

[sachin-27]

Convert flat per-granularity execution results into the client's nested aggregation response using the original request as template.

Description

Converts analytics engine execution results into OpenSearch InternalAggregations format, enabling aggregation responses in the DSL query executor. Supersedes #21346.

Changes

• AggregationResponseBuilder: Converts flat execution results to nested aggregation structure using granularity-based matching
• ComparisonUtils: Type coercion for numeric comparisons
• Metric/Terms translators: Implement response conversion (InternalAvg/Sum/Min/Max, StringTerms)
• TransportDslExecuteAction / SearchResponseBuilder: Wire the response builder into the search flow

Verified end to end on a live composite index: terms + metrics, filtered global metrics, and two-level nested aggregations return correct legacy-shaped responses.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 85dcbf4)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/AggregationResponseBuilder.java [250]: Avoid re-scanning row list on every recursion; index rows by parent bucket key. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/SearchResponseBuilder.java [54]: Shard counts, timed_out, and engine-side took require execution metadata from analytics plugin. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/SearchResponseBuilder.java [61]: Build hits from HITS results.

🔀 No multiple PR themes

⚡ Recommended focus areas for review Truncation Before Sub-Aggregation Consideration Truncation to size happens after sorting by this level's order only, and sub-aggregations already computed for truncated buckets are discarded. When a parent terms aggregation truncates, any child aggregation results computed for dropped buckets are lost, and sum_other_doc_count sums doc counts of dropped buckets but child metrics/buckets under them are silently discarded without any indication. This matches legacy shard-level behavior superficially but may cause discrepancies since the analytics engine computes exact results across all buckets. long otherDocCount = 0; if (termBuckets.size() > agg.size()) { for (int i = agg.size(); i < termBuckets.size(); i++) { otherDocCount += termBuckets.get(i).getDocCount(); } termBuckets = new ArrayList<>(termBuckets.subList(0, agg.size())); } Performance Concern filterRows and findMatchingRow perform a full linear scan of the result rows on every recursion level and for every parent bucket. For a two-level nesting with N outer buckets and M rows, this becomes O(N*M) work per sub-aggregation. The TODO acknowledges this, but for realistically sized aggregation results (thousands of buckets) response building could become a significant bottleneck. Consider indexing rows by parent key prefix upfront. // TODO: Avoid re-scanning the full row list on every recursion (here and in findMatchingRow). // Index each granularity's rows by parent bucket key once, then look buckets up directly. /** * Filters rows to only those matching all filter criteria. * Ensures nested aggregations only process rows belonging to their parent bucket. */ private static List<Object[]> filterRows(List<Object[]> rows, Map<String, Integer> colIndex, Map<String, Object> filter) { if (filter.isEmpty()) { return rows; } return rows.stream().filter(row -> matchesFilter(row, colIndex, filter)).collect(Collectors.toList()); } Precision Loss in Avg Encoding Encoding (sum=value, count=1) means InternalAvg.getValue() returns the pre-computed average correctly for a single shard, but this InternalAvg cannot be reduced/combined correctly with other InternalAvg instances (sum and count no longer represent the underlying data). If reduction across shards or aggregators is ever invoked on these results, the average will be incorrect. Since the analytics engine appears to produce a single already-reduced result this may be fine, but any future path that calls InternalAvg.reduce will produce wrong values. @Override public InternalAggregation toInternalAggregation(String name, Object value, Map<String, Object> metadata) { if (value == null) { return new InternalAvg(name, 0.0, 0, DocValueFormat.RAW, metadata); } return new InternalAvg(name, toDouble(value), 1, DocValueFormat.RAW, metadata); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 85dcbf4

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Document reduction-safety limitation for average Encoding the final average as (sum=value, count=1) breaks any downstream reduction/merge of InternalAvg (e.g., cross-shard or pipeline aggregations), because InternalAvg.reduce sums both sum and count across shards, yielding an incorrect combined average. Since the engine returns the final value, prefer a mechanism that preserves the computed value under reduction (or explicitly document/assert that these results are never re-reduced). sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/metric/AvgMetricTranslator.java [47-52] @Override public InternalAggregation toInternalAggregation(String name, Object value, Map<String, Object> metadata) { if (value == null) { return new InternalAvg(name, 0.0, 0, DocValueFormat.RAW, metadata); } - return new InternalAvg(name, toDouble(value), 1, DocValueFormat.RAW, metadata); + double v = toDouble(value); + // NOTE: (sum=v, count=1) is only correct when this result will not be reduced with siblings. + return new InternalAvg(name, v, 1, DocValueFormat.RAW, metadata); } Suggestion importance[1-10]: 5 __ Why: Valid concern about InternalAvg reduction semantics with (sum=v, count=1) encoding across shards. However, the improved code only adds a comment without a functional fix, so the impact is limited to documentation.	Low
General	Guard bucket truncation against invalid size agg.size() is called repeatedly and, more importantly, TermsAggregationBuilder#size() may return a negative or zero value in some legacy call paths, which would cause subList(0, agg.size()) to throw or produce an empty result unexpectedly. Cache the size once and guard against non-positive values to avoid IndexOutOfBoundsException and to make truncation logic deterministic. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/bucket/TermsBucketTranslator.java [83-89] +int size = agg.size(); long otherDocCount = 0; -if (termBuckets.size() > agg.size()) { - for (int i = agg.size(); i < termBuckets.size(); i++) { +if (size > 0 && termBuckets.size() > size) { + for (int i = size; i < termBuckets.size(); i++) { otherDocCount += termBuckets.get(i).getDocCount(); } - termBuckets = new ArrayList<>(termBuckets.subList(0, agg.size())); + termBuckets = new ArrayList<>(termBuckets.subList(0, size)); } Suggestion importance[1-10]: 4 __ Why: Caching agg.size() is a minor readability improvement. The concern about negative/zero values is speculative since TermsAggregationBuilder typically validates size at construction, but adds some defensive robustness.	Low
	Validate doc count cell before cast A null _count cell (possible if a row is malformed or the engine emits null for empty groups) will throw an opaque NullPointerException here. Validate the value and produce a clear ConversionException referencing the aggregation name to aid debugging. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/AggregationResponseBuilder.java [211-212] Object[] firstRowInGroup = entry.getValue().get(0); -long docCount = ((Number) firstRowInGroup[countIdx]).longValue(); +Object rawCount = firstRowInGroup[countIdx]; +if (!(rawCount instanceof Number)) { + throw new ConversionException( + "Missing or non-numeric " + AggregationMetadataBuilder.IMPLICIT_COUNT_NAME + + " for bucket in aggregation [" + agg.getName() + "]" + ); +} +long docCount = ((Number) rawCount).longValue(); Suggestion importance[1-10]: 3 __ Why: Minor defensive error handling suggestion. The _count column is guaranteed by the engine contract, so a NPE would already indicate a broken invariant; the improvement is a clearer error message.	Low
	Ensure converter reference is safely captured converter is captured by the lambda passed to planExecutor.execute below. Because it is assigned inside a try and used later in a callback, the compiler may complain (or require it to be effectively final via a different structure). More importantly, if resolveToSingleIndex or contextProvider.getContext() throws, converter remains uninitialized — ensure the subsequent lambda cannot observe an uninitialized reference. Consider initializing converter before the try or capturing an explicit final local within the success path. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/action/TransportDslExecuteAction.java [82-94] threadPool.executor(ThreadPool.Names.SEARCH).execute(() -> { final long startNanos = System.nanoTime(); final QueryPlans plans; final SearchSourceConverter converter; try { String indexName = resolveToSingleIndex(request); converter = new SearchSourceConverter(contextProvider.getContext().schema()); plans = converter.convert(request.source(), indexName); } catch (Exception e) { logger.error("DSL conversion failed", e); listener.onFailure(e); return; + } + final SearchSourceConverter finalConverter = converter; Suggestion importance[1-10]: 2 __ Why: The concern is unfounded: the try block either fully assigns converter or returns via listener.onFailure, so the subsequent lambda can never observe an uninitialized reference. Java's definite assignment rules handle this correctly.	Low

---

Previous suggestions

Suggestions up to commit f030201

Category	Suggestion	Impact
General	Hoist invariant lookup out of loop The _count column lookup and null-check are performed inside the per-bucket loop, but the result is invariant across buckets of a granularity. Hoist the lookup out of the loop to avoid redundant work and to fail fast before any bucket construction begins. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/AggregationResponseBuilder.java [206-213] Integer countIdx = colIndex.get(AggregationMetadataBuilder.IMPLICIT_COUNT_NAME); if (countIdx == null) { throw new ConversionException( "Missing " + AggregationMetadataBuilder.IMPLICIT_COUNT_NAME + " column in aggregation result" ); } -Object[] firstRowInGroup = entry.getValue().get(0); -long docCount = ((Number) firstRowInGroup[countIdx]).longValue(); +for (Map.Entry<List<Object>, List<Object[]>> entry : grouped.entrySet()) { + Map<String, Object> childFilter = new HashMap<>(parentKeyFilter); + for (int i = 0; i < currentGroupColumns.size(); i++) { + childFilter.put(currentGroupColumns.get(i), entry.getKey().get(i)); + } + Object[] firstRowInGroup = entry.getValue().get(0); + long docCount = ((Number) firstRowInGroup[countIdx]).longValue(); Suggestion importance[1-10]: 4 __ Why: Valid minor refactor: the _count column lookup is invariant across buckets, so hoisting it improves clarity and fails fast. Impact is minor since the map lookup is cheap.	Low
	Guard against non-positive size values agg.size() returns an int, but calling it repeatedly in the loop and using it as a size limit is fine — however, if size is very large (near Integer.MAX_VALUE) or negative, the truncation logic could behave incorrectly. More importantly, when agg.size() <= 0 (which is technically invalid but possible), the loop and subList could throw. Consider validating or guarding against non-positive size values, or at minimum documenting the precondition. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/bucket/TermsBucketTranslator.java [83-89] long otherDocCount = 0; -if (termBuckets.size() > agg.size()) { - for (int i = agg.size(); i < termBuckets.size(); i++) { +int size = agg.size(); +if (size > 0 && termBuckets.size() > size) { + for (int i = size; i < termBuckets.size(); i++) { otherDocCount += termBuckets.get(i).getDocCount(); } - termBuckets = new ArrayList<>(termBuckets.subList(0, agg.size())); + termBuckets = new ArrayList<>(termBuckets.subList(0, size)); } Suggestion importance[1-10]: 3 __ Why: TermsAggregationBuilder validates size to be positive at builder time, so a non-positive value cannot reach this code path. The suggestion is defensive but of low practical impact.	Low
	Document non-reducibility of InternalAvg encoding Encoding the final average as (sum=value, count=1) is fragile: any downstream reducer or partial-reduce path that tries to combine InternalAvg values (e.g., cross-shard reduction) will compute an incorrect average since it will re-divide sum by summed counts. If this response ever flows through additional reduction, results will be silently wrong. Consider documenting that these InternalAvg values are terminal and must not be reduced, or use a subclass/format that preserves the final value directly. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/metric/AvgMetricTranslator.java [46-52] +// NOTE: This InternalAvg is terminal — sum=value, count=1 reproduces the final average +// via getValue(), but any further reduce() with other InternalAvg instances will corrupt +// the result. Callers must not pass these through additional reduction phases. @Override public InternalAggregation toInternalAggregation(String name, Object value, Map<String, Object> metadata) { if (value == null) { return new InternalAvg(name, 0.0, 0, DocValueFormat.RAW, metadata); } return new InternalAvg(name, toDouble(value), 1, DocValueFormat.RAW, metadata); } Suggestion importance[1-10]: 3 __ Why: Suggestion only adds a comment documenting a known encoding constraint; it doesn't fix a bug. Reasonable observation but low impact.	Low

Suggestions up to commit 7017942

Category	Suggestion	Impact
Possible issue	Sum doc counts across rows in group When sub-aggregations exist, docCount should be summed across all rows in the group (each row corresponds to a distinct combination of deeper group fields), not taken from the first row only. Using only the first row underreports the bucket's doc_count whenever the granularity is finer than the current bucket level. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/AggregationResponseBuilder.java [206-213] Integer countIdx = colIndex.get(AggregationMetadataBuilder.IMPLICIT_COUNT_NAME); if (countIdx == null) { throw new ConversionException( "Missing " + AggregationMetadataBuilder.IMPLICIT_COUNT_NAME + " column in aggregation result" ); } -Object[] firstRowInGroup = entry.getValue().get(0); -long docCount = ((Number) firstRowInGroup[countIdx]).longValue(); +long docCount = 0L; +for (Object[] r : entry.getValue()) { + docCount += ((Number) r[countIdx]).longValue(); +} Suggestion importance[1-10]: 8 __ Why: Valid concern: when a bucket has finer-grained sub-groupings, each row in entry.getValue() represents a distinct sub-group with its own _count. Taking only the first row's count underreports the parent bucket's doc_count. This is a correctness bug.	Medium
General	Ensure captured variable lifetime is safe converter is captured by the inner ActionListener.wrap lambda but is only assigned inside the try; if the compiler still treats it as effectively final it's fine, but marking it final at declaration prevents any accidental reassignment and confirms the intent. More importantly, ensure converter is not null in the callback since assignment failure paths return early — this is already safe, but a defensive check avoids future regressions. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/action/TransportDslExecuteAction.java [83-94] +final long startNanos = System.nanoTime(); +final QueryPlans plans; +final SearchSourceConverter converter; +try { + String indexName = resolveToSingleIndex(request); + converter = new SearchSourceConverter(contextProvider.getContext().schema()); + plans = converter.convert(request.source(), indexName); +} catch (Exception e) { + logger.error("DSL conversion failed", e); + listener.onFailure(e); + return; - Suggestion importance[1-10]: 1 __ Why: The converter is already declared final and the improved_code is identical to existing_code. The suggestion provides no actual change and only asks the user to verify existing behavior.	Low

Suggestions up to commit 843ee01

Category	Suggestion	Impact
Possible issue	Sum doc counts across grouped rows When multiple rows share the same bucket key (e.g., when grouping columns are a subset of the current grouping context due to nested groupings not fully filtered), taking docCount from only the first row will undercount. Aggregate the count across all rows in the group to preserve total document count semantics. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/AggregationResponseBuilder.java [206-213] Integer countIdx = colIndex.get(AggregationMetadataBuilder.IMPLICIT_COUNT_NAME); if (countIdx == null) { throw new ConversionException( "Missing " + AggregationMetadataBuilder.IMPLICIT_COUNT_NAME + " column in aggregation result" ); } -Object[] firstRowInGroup = entry.getValue().get(0); -long docCount = ((Number) firstRowInGroup[countIdx]).longValue(); +long docCount = 0L; +for (Object[] r : entry.getValue()) { + docCount += ((Number) r[countIdx]).longValue(); +} Suggestion importance[1-10]: 5 __ Why: The suggestion raises a plausible concern that docCount may be undercounted if grouped rows share the same bucket key but represent multiple sub-group rows. However, given the granularity-based design where each granularity has its own result set with exact grouping, rows within a group should share the same _count, making the current logic potentially correct. The concern is worth verifying but not clearly a bug.	Low
General	Guard against non-string terms keys Using key.toString() for non-string bucket keys (e.g., Long, Double, Boolean) produces StringTerms buckets with textual keys that differ from legacy behavior, which returns LongTerms/DoubleTerms with typed keys. This yields incorrect key_as_string formatting and JSON output types. Consider guarding against non-string keys until per-type variants are implemented, or at least verify the key type matches the aggregation's value source. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/bucket/TermsBucketTranslator.java [72-73] +if (!(key instanceof String) && !(key instanceof BytesRef)) { + throw new UnsupportedOperationException( + "Non-string terms keys are not yet supported (got " + key.getClass().getSimpleName() + ")" + ); +} BytesRef term = new BytesRef(key.toString()); termBuckets.add(new StringTerms.Bucket(term, entry.docCount(), entry.subAggs(), false, 0, DocValueFormat.RAW)); Suggestion importance[1-10]: 5 __ Why: The javadoc already acknowledges that per-key-type variants (Long/Double) are follow-up work, so the current use of key.toString() on non-string keys is a known limitation. Adding an explicit guard would fail loudly instead of silently producing incorrect output types, which is a modest improvement in correctness/safety.	Low
	Document non-reducible average encoding Encoding the final average as (sum=value, count=1) breaks any downstream reduce/merge across shards or partial results, because InternalAvg.reduce sums sum and count fields — merging multiple such encodings will produce incorrect averages. If cross-shard reduction is ever wired in, this will silently corrupt results. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/metric/AvgMetricTranslator.java [45-50] @Override public InternalAggregation toInternalAggregation(String name, Object value) { if (value == null) { return new InternalAvg(name, 0.0, 0, DocValueFormat.RAW, null); } + // TODO: propagate actual sum and count from the engine to support proper reduction. return new InternalAvg(name, toDouble(value), 1, DocValueFormat.RAW, null); } Suggestion importance[1-10]: 3 __ Why: The suggestion is mostly documentation-oriented (adding a TODO comment) and the existing javadoc already explains the encoding. The concern about cross-shard reduce is valid but low priority given the current single-node analytics context.	Low

Suggestions up to commit 518338d

Category	Suggestion	Impact
Possible issue	Guard against non-string terms keys Coercing every key via toString() will produce incorrect string representations for non-string types (e.g. numeric terms aggregations render Long/Double values into a StringTerms instead of the appropriate LongTerms/DoubleTerms), and will conflict with the TermsAggregationBuilder's configured value type. Consider dispatching on key type to build LongTerms/DoubleTerms where applicable, or explicitly validating that the field is string-typed here. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/bucket/TermsBucketTranslator.java [65-74] for (BucketEntry entry : buckets) { Object key = entry.keys().get(0); if (key == null) { - // SQL GROUP BY emits a NULL group; legacy terms excludes docs with a - // missing field entirely (no bucket) unless "missing" is configured. continue; + } + if (!(key instanceof CharSequence)) { + throw new IllegalStateException( + "TermsBucketTranslator currently supports only string keys; got " + key.getClass().getSimpleName() + ); } BytesRef term = new BytesRef(key.toString()); termBuckets.add(new StringTerms.Bucket(term, entry.docCount(), entry.subAggs(), false, 0, DocValueFormat.RAW)); } Suggestion importance[1-10]: 6 __ Why: The class Javadoc already acknowledges that per-key-type Long/Double variants are follow-up work, so failing loudly on non-string keys is a reasonable defensive guard to avoid silently producing incorrect output.	Low
	Fail fast on missing aggregate node When the plan has no LogicalAggregate (e.g. a metric-only aggregation without GROUP BY), this method returns NO_GROUPING_KEY, which collides with the key used for both no-grouping metrics and any aggregate that produced an empty group list. However, if multiple ExecutionResults produce the same key (e.g. a top-level metric plan and a plan whose aggregate could not be located), the last one silently overwrites earlier ones in granularityMap. Consider detecting a collision in the constructor and failing fast, or preserving all results per key, to prevent silent result loss. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/AggregationResponseBuilder.java [285-297] private static String computeGranularityKey(ExecutionResult result) { RelNode node = result.getPlan().relNode(); while (node != null && !(node instanceof LogicalAggregate)) { node = node.getInputs().isEmpty() ? null : node.getInput(0); } if (node instanceof LogicalAggregate agg) { - // Resolve group field names against the aggregate's INPUT row type via the group - // set — robust against output-side renames, unlike the aggregate's own row type. List<String> inputFields = agg.getInput().getRowType().getFieldNames(); return granularityKey(agg.getGroupSet().asList().stream().map(inputFields::get).collect(Collectors.toList())); } - return NO_GROUPING_KEY; + throw new IllegalStateException("Aggregation ExecutionResult has no LogicalAggregate in plan"); } Suggestion importance[1-10]: 4 __ Why: Reasonable defensive change, but NO_GROUPING_KEY is also used intentionally for the metric-only no-grouping case, so throwing might be too strict. The suggestion notes a plausible edge case but its impact is limited.	Low
General	Detect duplicate granularity keys granularityMap.put silently overwrites when two ExecutionResults have the same granularity key (which can happen if two subtrees produce the same sorted set of group fields, e.g. two sibling terms aggs on the same field). Detect duplicates and reject them explicitly instead of dropping one silently, otherwise nested/sibling aggregations may return incorrect buckets. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/AggregationResponseBuilder.java [45-52] public AggregationResponseBuilder(AggregationRegistry registry, List<ExecutionResult> aggResults) { this.registry = registry; this.granularityMap = new HashMap<>(); for (ExecutionResult result : aggResults) { String key = computeGranularityKey(result); - granularityMap.put(key, result); + if (granularityMap.putIfAbsent(key, result) != null) { + throw new IllegalStateException("Duplicate granularity key for aggregation results: [" + key + "]"); + } } } Suggestion importance[1-10]: 6 __ Why: Valid concern — sibling aggregations sharing the same group-field set would collide and silently overwrite. Failing fast improves correctness detection, though it may not fully solve the underlying design issue.	Low
	Avg encoding may break reduction Encoding the final average as (sum=value, count=1) breaks aggregation reduction: if InternalAvg.reduce is ever invoked (e.g. cross-cluster or pipeline aggregations that consume InternalAvg), it will sum values and counts across shards and produce an incorrect average. Consider documenting/enforcing that these results are terminal and never reduced, or emitting a subclass/format that guards against reduction. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/metric/AvgMetricTranslator.java [45-50] @Override public InternalAggregation toInternalAggregation(String name, Object value) { + // NOTE: terminal-only encoding — must not be passed to InternalAvg.reduce(). if (value == null) { return new InternalAvg(name, 0.0, 0, DocValueFormat.RAW, null); } return new InternalAvg(name, toDouble(value), 1, DocValueFormat.RAW, null); } Suggestion importance[1-10]: 5 __ Why: Legitimate concern about reduction semantics, but the improved code only adds a comment. The suggestion doesn't materially fix the issue.	Low

Suggestions up to commit c6a65de

Category	Suggestion	Impact
Possible issue	Use input field names for grouping key Using getRowType().getFieldNames() from the LogicalAggregate may return output field names (renamed via aliases), while the request-side walk accumulates original schema field names. This can cause granularity key mismatches. Consider retrieving group field names from the aggregate's input row type at the grouped column indices instead. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/AggregationResponseBuilder.java [284-294] private static String computeGranularityKey(ExecutionResult result) { RelNode node = result.getPlan().relNode(); while (node != null && !(node instanceof LogicalAggregate)) { node = node.getInputs().isEmpty() ? null : node.getInput(0); } if (node instanceof LogicalAggregate agg) { - int groupCount = agg.getGroupCount(); - return granularityKey(agg.getRowType().getFieldNames().stream().limit(groupCount).collect(Collectors.toList())); + List<String> inputFields = agg.getInput().getRowType().getFieldNames(); + List<String> groupFields = agg.getGroupSet().asList().stream() + .map(inputFields::get) + .collect(Collectors.toList()); + return granularityKey(groupFields); } return NO_GROUPING_KEY; } Suggestion importance[1-10]: 6 __ Why: Potentially valid concern about alias/rename mismatches between aggregate output field names and the request-side accumulated field names, which could affect granularity key matching. Correctness depends on plan structure not shown.	Low
General	Preserve distinction between null and empty keys Rendering null keys as empty string "" is indistinguishable from a bucket with an actual empty-string key, which can cause data collisions when both null and empty values exist. Consider filtering out null keys, or use a distinct representation (e.g., using DocValueFormat's null handling) to preserve the distinction, matching OpenSearch's native missing-value bucket semantics. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/bucket/TermsBucketTranslator.java [64-69] List<StringTerms.Bucket> termBuckets = new ArrayList<>(); for (BucketEntry entry : buckets) { Object key = entry.keys().get(0); - BytesRef term = new BytesRef(key == null ? "" : key.toString()); + if (key == null) { + continue; // null keys are excluded unless a "missing" value is configured + } + BytesRef term = new BytesRef(key.toString()); termBuckets.add(new StringTerms.Bucket(term, entry.docCount(), entry.subAggs(), false, 0, DocValueFormat.RAW)); } Suggestion importance[1-10]: 5 __ Why: Valid observation that mapping null to empty string may cause key collisions with actual empty strings, though the correct handling depends on OpenSearch's missing-value semantics for terms aggregations.	Low
	Guard against null _count values The _count column is looked up inside the group loop but is invariant per result — hoist it above the loop to fail fast and avoid repeated map lookups. Additionally, if the value at countIdx is null (possible with outer joins or empty groups), the cast to Number will throw NPE with an unhelpful message. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/result/AggregationResponseBuilder.java [182-187] Integer countIdx = colIndex.get("_count"); if (countIdx == null) { throw new ConversionException("Missing _count column in aggregation result"); } +... Object[] firstRowInGroup = entry.getValue().get(0); -long docCount = ((Number) firstRowInGroup[countIdx]).longValue(); +Object countValue = firstRowInGroup[countIdx]; +long docCount = countValue == null ? 0L : ((Number) countValue).longValue(); Suggestion importance[1-10]: 4 __ Why: Defensive null-check for _count value; unlikely in aggregation results but a minor robustness improvement. The hoisting claim is inaccurate since the lookup is already outside the inner logic.	Low
	Avoid NPE in error message construction Calling value.getClass() when value is null will throw a NullPointerException masking the actual issue. Since callers already handle null before calling this method, but a defensive null check in the error message would prevent confusing NPE stack traces if null slips through. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/metric/AbstractMetricTranslator.java [75-80] protected static double toDouble(Object value) { if (value instanceof Number number) { return number.doubleValue(); } - throw new IllegalStateException("Expected numeric aggregation result but got " + value.getClass().getSimpleName()); + throw new IllegalStateException( + "Expected numeric aggregation result but got " + (value == null ? "null" : value.getClass().getSimpleName()) + ); } Suggestion importance[1-10]: 3 __ Why: Minor defensive improvement; callers guard against null before invoking toDouble, so the NPE risk is low. Still, a small readability/robustness win.	Low

[github-actions]

❌ Gradle check result for c6a65de: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 518338d

[github-actions]

❌ Gradle check result for 518338d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 843ee01

[github-actions]

❌ Gradle check result for 843ee01: TIMEOUT

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[nssuresh2007]

Lets use System.nanoTime() for latency calculations

[sachin-27]

done

[nssuresh2007]

At every recursion, we again filter through the complete list of rows. Can we instead index them in a map so that iteration becomes linear even when recursed multiple times?
(We can add TODO for that and optimize in follow up PR)

[sachin-27]

added a TODO for now. Will raise a separate PR for this

[ask-kamal-nayan]

We are passing here null for metadata parameter public InternalAvg(String name, double sum, long count, DocValueFormat format, Map<String, Object> metadata). Do we need to pass the metadata values as well and process that to be consistent with OpenSearch?

[sachin-27]

updated

[ask-kamal-nayan]

If the metric's column is missing from the result, this silently returns an empty metric , but a missing column means the plan/metadata invariant broke, and the analogous cases (groupRowsByKeys missing group column, buildBucket missing count column) throw ConversionException. Should this throw too, so pipeline bugs surface instead of rendering as "value": null?

[sachin-27]

updated

[github-actions]

Persistent review updated to latest commit 7017942

[github-actions]

Persistent review updated to latest commit f030201

[github-actions]

Persistent review updated to latest commit 85dcbf4

[github-actions]

❌ Gradle check result for 85dcbf4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[nssuresh2007]

LGTM. Just a nitpick comment

[nssuresh2007]

I think for pipeline aggregations, where we need to do post processing on the response we would still need some handling which is not covered in this PR, is that understanding correct?
If yes, please add a TODO for the same.