Fix #5178: SUM of all-null column returns NULL per SQL standard

[opensearchpplteam]

Description

Root Cause: OpenSearch's sum aggregation returns 0.0 when all input values are null, which violates the SQL standard that requires SUM of an all-null column to return NULL.

Fix: Replace the sum aggregation with stats aggregation for SUM pushdown. The stats aggregation provides both sum and count fields. When count == 0, all input values were null, so the parser returns null instead of 0.0. This is implemented in a new SumStatsParser class that follows the existing MetricParser pattern.

Both the Calcite code path (AggregateAnalyzer) and the legacy code path (MetricAggregationBuilder) are updated consistently.

Related Issues

#5178

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

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 c1e2833)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Order Path Logic The resolveOrderPath method adds ".sum" suffix for StatsAggregationBuilder when ordering. Verify this works correctly with OpenSearch's bucket ordering API and that the path resolution handles all edge cases (e.g., nested aggregations, missing aggregations). /** * Resolve the order path for a bucket order. Stats aggregation is a multi-value metrics * aggregation, so when ordering on it, the sort path must specify the sub-metric name (e.g., * "sum(balance).sum"). */ private String resolveOrderPath(String aggName, CompositeAggregationBuilder composite) { AggregationBuilder sortMetric = composite.getSubAggregations().stream() .filter(sub -> sub.getName().equals(aggName)) .findFirst() .orElse(null); if (sortMetric instanceof StatsAggregationBuilder) { return aggName + ".sum"; } return aggName; } Null Handling The parser returns null when count == 0. Ensure this behavior is consistent with SQL standard across all data types and doesn't cause issues with downstream processing that might not expect null values from SUM aggregations. public List<Map<String, Object>> parse(Aggregation agg) { Stats stats = (Stats) agg; Object value = stats.getCount() == 0 ? null : stats.getSum(); return Collections.singletonList(new HashMap<>(Collections.singletonMap(agg.getName(), value))); Comment Accuracy Comment mentions "Use stats aggregation instead of sum to detect all-null input" but the implementation change affects performance (stats computes more metrics than sum). Consider documenting the performance trade-off. // Use stats aggregation instead of sum to detect all-null input (count == 0 -> null). case SUM -> Pair.of( helper.build(args.getFirst().getKey(), AggregationBuilders.stats(aggName)), new SumStatsParser(aggName));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to c1e2833

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid unnecessary computation when empty The resolveOrderPath method is called before checking if composite.getSubAggregations().isEmpty(). If the sub-aggregations are empty, resolveOrderPath will not find the aggregation and may return the original aggName, but this path is never used since BucketOrder.count(asc) is returned instead. Consider moving the resolveOrderPath call inside the else branch to avoid unnecessary computation. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [128-133] String aggName = getAggregationPath(collations, fieldNames, composite); -String orderPath = resolveOrderPath(aggName, composite); -BucketOrder bucketOrder = - composite.getSubAggregations().isEmpty() - ? BucketOrder.count(asc) - : BucketOrder.aggregation(orderPath, asc); +BucketOrder bucketOrder; +if (composite.getSubAggregations().isEmpty()) { + bucketOrder = BucketOrder.count(asc); +} else { + String orderPath = resolveOrderPath(aggName, composite); + bucketOrder = BucketOrder.aggregation(orderPath, asc); +} Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that resolveOrderPath is called even when composite.getSubAggregations().isEmpty() is true, making it unnecessary. Moving the call inside the else branch avoids this computation. However, the impact is minor as it's a small optimization that doesn't fix a bug or significantly improve performance.	Low

---

Previous suggestions

Suggestions up to commit e5560ec

Category	Suggestion	Impact
Possible issue	Handle missing aggregation case The method returns null when no matching sub-aggregation is found, but the caller uses the result directly in BucketOrder.aggregation(). This could cause a NullPointerException. Consider throwing an exception or returning a default value when the aggregation is not found. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [346-356] private String resolveOrderPath(String aggName, CompositeAggregationBuilder composite) { AggregationBuilder sortMetric = composite.getSubAggregations().stream() .filter(sub -> sub.getName().equals(aggName)) .findFirst() - .orElse(null); + .orElseThrow(() -> new IllegalStateException( + "Aggregation not found: " + aggName)); if (sortMetric instanceof StatsAggregationBuilder) { return aggName + ".sum"; } return aggName; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that orElse(null) could lead to a NullPointerException when the result is used in BucketOrder.aggregation(). However, this is more of a defensive programming improvement rather than a critical bug, as the code likely assumes the aggregation exists in the expected context. The suggestion to throw an exception is reasonable for fail-fast behavior.	Medium

Suggestions up to commit cc21a76

Category	Suggestion	Impact
Possible issue	Handle NaN sum values correctly The condition stats.getCount() == 0 may not correctly handle cases where all input values are null but documents exist. Consider checking if the sum is NaN or if both count and sum indicate no valid numeric values were aggregated. opensearch/src/main/java/org/opensearch/sql/opensearch/response/agg/SumStatsParser.java [31] -Object value = stats.getCount() == 0 ? null : stats.getSum(); +Object value = (stats.getCount() == 0 || Double.isNaN(stats.getSum())) ? null : stats.getSum(); Suggestion importance[1-10]: 7 __ Why: The suggestion to check for NaN values is valid and could improve robustness. However, the primary goal of the PR is to return null when count == 0, which correctly handles all-null inputs. The NaN check adds defensive programming but may not be strictly necessary if OpenSearch's Stats aggregation guarantees non-NaN sums when count > 0.	Medium

[github-actions]

Persistent review updated to latest commit e5560ec

[github-actions]

Persistent review updated to latest commit c1e2833

[opensearchpplteam]

The resolveOrderPath method uses .orElse(null) when looking up the sub-aggregation. If the aggregation name is not found (which should not happen in normal flow), the method silently returns aggName without the .sum suffix rather than failing fast. Consider using .orElseThrow() to surface unexpected state earlier, or add a brief comment explaining why the fallback is intentional.

[penghuo]

This is opensearch issue. opensearch-project/OpenSearch#17581.

[opensearchpplteam]

Thanks for flagging this, @penghuo. You're right that the root cause is in OpenSearch's sum aggregation, which returns 0.0 instead of null when all input values are null. OpenSearch/OpenSearch#17581 tracks that core fix.

This PR provides a SQL-plugin-level workaround so that SUM returns the correct SQL-standard result (NULL) now, by switching to the stats aggregation and using count == 0 to detect all-null input. Once the core issue is resolved, this can be simplified back to using the sum aggregation directly.