[BugFix] Fix bin/chart NPE with null values (#5174)

[qianheng-aws]

Description

Fix NullPointerException when using bin then chart with null values in the binned field.

Root cause: The bin bucket functions (SpanBucketFunction, MinspanBucketFunction, RangeBucketFunction) declared a non-nullable VARCHAR(2000) return type via ReturnTypes.VARCHAR_2000, even though they return null for null inputs. This caused Calcite's optimizer to remove the IS NOT NULL filters (added by visitAggregation's nonNullGroupMask logic) as trivially true, allowing null group keys to reach the Enumerable aggregation's TreeMap, which crashed with NPE in compareTo.

Fix:

1. Declare the return type as nullable using ReturnTypes.VARCHAR_2000.andThen(SqlTypeTransforms.FORCE_NULLABLE) so that the null-filtering logic in visitAggregation works correctly
2. Add nullsLast to chart command sort operations as a defensive measure

Related Issues

Resolves #5174

Check List

• New functionality includes testing
• Commits signed per DCO (-s)
• spotlessCheck passed
• Unit tests passed
• Integration tests passed

Test plan

• Unit test (CalcitePPLChartNullTest) verifying logical plan includes null filter and NULLS LAST
• Integration test (CalciteBinChartNullIT) verifying bin+chart with null values works end-to-end
• YAML REST test (5174.yml) reproducing the exact scenario from the issue

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ccb3283)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Fix nullable return type for bin bucket UDFs Relevant files: core/src/main/java/org/opensearch/sql/expression/function/udf/binning/MinspanBucketFunction.java core/src/main/java/org/opensearch/sql/expression/function/udf/binning/RangeBucketFunction.java core/src/main/java/org/opensearch/sql/expression/function/udf/binning/SpanBucketFunction.java Sub-PR theme: Add nullsLast sort to chart command with tests Relevant files: core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinChartNullIT.java ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLChartNullTest.java integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5174.yml Sub-PR theme: Update Claude agent harness and documentation Relevant files: .claude/commands/ppl-bugfix.md .claude/harness/ppl-bugfix-followup.md .claude/harness/ppl-bugfix-harness.md .claude/harness/ppl-bugfix-reference.md CLAUDE_GUIDE.md

⚡ Recommended focus areas for review Missing Sort In the early-return branch (when rowSplit == null || columnSplit == null || limit == 0), a relBuilder.sort() with nullsLast was added. However, the original code had no sort at all in this branch — the sort was only added in the two-split path. Verify whether adding a sort here is intentional and correct for all cases (e.g., when rowSplit == null), since sorting by field(0) when there may be no meaningful first field could produce unexpected results. relBuilder.sort(relBuilder.nullsLast(relBuilder.field(0))); return relBuilder.peek(); Hardcoded Expected Rows The integration test hardcodes specific row values (e.g., rows("0-10000", "M", 1)) that depend on the exact data in TEST_INDEX_BANK_WITH_NULL_VALUES. If the test index data changes, these assertions will silently break. Verify that the expected data matches the actual index contents and consider documenting the dependency. verifyDataRows( result, rows("0-10000", "M", 1), rows("30000-40000", "F", 1), rows("30000-40000", "M", 1), rows("40000-50000", "F", 1)); Brittle SQL Assertion The unit test uses exact full SQL string matching (verifyPPLToSparkSQL) against a multi-line expected string. This is fragile — any whitespace, alias, or optimizer change will break the test without indicating a real regression. Consider verifying only the key structural elements (presence of IS NOT NULL, NULLS LAST) rather than the entire SQL string. String expectedSparkSql = "SELECT `t2`.`val_bin`, CASE WHEN `t2`.`category` IS NULL THEN 'NULL' WHEN" + " `t10`.`_row_number_chart_` <= 10 THEN `t2`.`category` ELSE 'OTHER' END" + " `category`, SUM(`t2`.`count()`) `count()`\n" + "FROM (SELECT `val_bin`, `category`, COUNT(*) `count()`\n" + "FROM (SELECT `count`, `category`, `subcategory`, `value`," + " SPAN_BUCKET(`value`, 50) `val_bin`\n" + "FROM `scott`.`bounty_numbers`) `t`\n" + "WHERE `val_bin` IS NOT NULL\n" + "GROUP BY `val_bin`, `category`) `t2`\n" + "LEFT JOIN (SELECT `category`, SUM(`count()`) `__grand_total__`, ROW_NUMBER() OVER" + " (ORDER BY SUM(`count()`) DESC) `_row_number_chart_`\n" + "FROM (SELECT `category`, COUNT(*) `count()`\n" + "FROM (SELECT `count`, `category`, `subcategory`, `value`," + " SPAN_BUCKET(`value`, 50) `val_bin`\n" + "FROM `scott`.`bounty_numbers`) `t3`\n" + "WHERE `val_bin` IS NOT NULL\n" + "GROUP BY `val_bin`, `category`) `t7`\n" + "WHERE `category` IS NOT NULL\n" + "GROUP BY `category`) `t10` ON `t2`.`category` = `t10`.`category`\n" + "GROUP BY `t2`.`val_bin`, CASE WHEN `t2`.`category` IS NULL THEN 'NULL' WHEN" + " `t10`.`_row_number_chart_` <= 10 THEN `t2`.`category` ELSE 'OTHER' END\n" + "ORDER BY `t2`.`val_bin` NULLS LAST, 2 NULLS LAST"; verifyPPLToSparkSQL(root, expectedSparkSql);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ccb3283

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Verify assumed SQL filter is actually generated The test hardcodes WHERE </code>val_bin<code> IS NOT NULL in the expected SQL, but this filter is not explicitly added in the visitChart fix shown in the diff — the fix only adds NULLS LAST to the sort. If the IS NOT NULL filter is generated by Calcite automatically due to FORCE_NULLABLE + NullPolicy.ANY, this assumption should be verified. If the filter is not actually generated, the expected SQL will not match and the test will fail. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLChartNullTest.java [75-98] +// Verify the expected SQL matches what Calcite actually generates after the fix. +// The WHERE val_bin IS NOT NULL clause should be confirmed to be emitted by Calcite +// based on NullPolicy.ANY + FORCE_NULLABLE, not assumed. String expectedSparkSql = "SELECT `t2`.`val_bin`, CASE WHEN `t2`.`category` IS NULL THEN 'NULL' WHEN" + " `t10`.`_row_number_chart_` <= 10 THEN `t2`.`category` ELSE 'OTHER' END" - ... + + " `category`, SUM(`t2`.`count()`) `count()`\n" + + ... + "ORDER BY `t2`.`val_bin` NULLS LAST, 2 NULLS LAST"; verifyPPLToSparkSQL(root, expectedSparkSql); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about whether WHERE val_bin IS NOT NULL is actually generated by Calcite due to FORCE_NULLABLE + NullPolicy.ANY, but the improved_code only adds comments without changing the logic. If the filter is not generated, the test would fail, making this a potentially important correctness concern, though it's framed as a verification rather than a concrete fix.	Low
General	Validate test assertions against actual index data The test verifies that only non-null balance rows appear, but the comment says "4 records with balance" while the BANK_WITH_NULL_VALUES index may have a different distribution. The expected rows should be validated against the actual test index data to ensure the assertions are correct and won't produce false positives or false negatives. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinChartNullIT.java [44-49] +// Verify expected rows match the actual BANK_WITH_NULL_VALUES index data verifyDataRows( result, rows("0-10000", "M", 1), rows("30000-40000", "F", 1), rows("30000-40000", "M", 1), rows("40000-50000", "F", 1)); Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify that expected rows match actual test index data, but this is a validation concern rather than a code fix. The existing_code and improved_code are essentially identical (only a comment is added), and the test data appears to be derived from the known BANK_WITH_NULL_VALUES index.	Low
	Confirm total reflects result rows not input docs The total field is asserted as 2, but with chart count() over val_bin by category, the result should have rows for each (val_bin, category) combination. With values 10.5 and 20.3 both falling in the 0-50 bin under category "A", and 100.0 in 100-150 under "B", there are 2 distinct groups — this appears correct. However, the total assertion should reflect the number of result rows (2 groups), which matches. Confirm that total in the PPL response refers to the number of result rows and not the number of input documents. integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5174.yml [65-67] +- match: { total: 2 } +- match: { schema: [ { name: val_bin, type: string }, { name: category, type: string }, { name: "count()", type: bigint } ] } +- match: { datarows: [ [ "0-50", "A", 2 ], [ "100-150", "B", 1 ] ] } - Suggestion importance[1-10]: 2 __ Why: The existing_code and improved_code are identical — no actual change is proposed. This is just a verification request about whether total refers to result rows or input documents, which is a low-impact clarification question rather than a concrete fix.	Low

---

Previous suggestions

Suggestions up to commit d41e598

Category	Suggestion	Impact
General	Reset transient setting to null instead of false The teardown resets plugins.calcite.enabled to false, but the setup sets it to true as a transient setting. If the test fails mid-run, the teardown may not execute, leaving Calcite enabled for subsequent tests. Consider using a persistent setting or ensuring the teardown is robust, but more critically, the query.settings API call in teardown should be verified to actually reset the state correctly in the test environment. integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5174.yml [48-51] - do: query.settings: body: transient: - plugins.calcite.enabled: false + plugins.calcite.enabled: null Suggestion importance[1-10]: 3 __ Why: Setting plugins.calcite.enabled to null in teardown is a more correct way to clear a transient setting (removing it entirely) rather than setting it to false, which could affect other tests that rely on the default value. This is a minor but valid improvement to test isolation.	Low
	Verify absence of null bin rows in result The verifyDataRows assertion does not account for null bin values that may appear in the result set when NULLS LAST ordering is used. If any null bal_bin rows are returned (e.g., for documents without a balance field), the test will fail unexpectedly. Ensure the test explicitly verifies that null bin rows are absent, or add a filter step in the PPL query to exclude them. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinChartNullIT.java [44-49] + // Null bin values should be filtered out; verify exactly these 4 non-null rows verifyDataRows( result, rows("0-10000", "M", 1), rows("30000-40000", "F", 1), rows("30000-40000", "M", 1), rows("40000-50000", "F", 1)); + // Also verify total matches expected non-null count + assertEquals(4, result.getJSONArray("datarows").length()); Suggestion importance[1-10]: 2 __ Why: The suggestion adds a redundant assertEquals check that duplicates what verifyDataRows already validates. The PR's core fix (making bucket functions return nullable types and using NULLS LAST sorting) ensures null bin values are filtered out via WHERE val_bin IS NOT NULL in the generated SQL, so the existing assertion is already sufficient.	Low

Suggestions up to commit 1f7053d

Category	Suggestion	Impact
General	Verify test covers null gender combinations The test comment says "4 records with balance" but the BANK_WITH_NULL_VALUES index may have records with null gender values that would appear as null in the result. If any document has a non-null balance but null gender, those rows would be missing from the expected results, causing the test to fail. Verify that all expected rows account for every combination of non-null bal_bin and gender (including null gender) present in the test index. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinChartNullIT.java [44-49] +// Verify expected rows match all non-null bal_bin combinations including null gender if present verifyDataRows( result, rows("0-10000", "M", 1), rows("30000-40000", "F", 1), rows("30000-40000", "M", 1), rows("40000-50000", "F", 1)); Suggestion importance[1-10]: 3 __ Why: This suggestion asks the developer to verify the test data rather than proposing a concrete code fix. The improved_code is essentially identical to the existing_code with only a comment added, making it a low-impact suggestion.	Low
	Avoid fragile ordinal reference in ORDER BY The expected SQL string uses ordinal 2 in ORDER BY ... 2 NULLS LAST for the second sort key. This is fragile and may break if the query plan changes column ordering. It would be more robust to use the explicit column expression (e.g., the CASE expression alias) instead of a positional reference, or at minimum add a comment explaining why the ordinal is used here. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLChartNullTest.java [75-98] String expectedSparkSql = "SELECT `t2`.`val_bin`, CASE WHEN `t2`.`category` IS NULL THEN 'NULL' WHEN" + " `t10`.`_row_number_chart_` <= 10 THEN `t2`.`category` ELSE 'OTHER' END" - ... - + "ORDER BY `t2`.`val_bin` NULLS LAST, 2 NULLS LAST"; + + " `category`, SUM(`t2`.`count()`) `count()`\n" + + "FROM (SELECT `val_bin`, `category`, COUNT(*) `count()`\n" + + "FROM (SELECT `count`, `category`, `subcategory`, `value`," + + " SPAN_BUCKET(`value`, 50) `val_bin`\n" + + "FROM `scott`.`bounty_numbers`) `t`\n" + + "WHERE `val_bin` IS NOT NULL\n" + + "GROUP BY `val_bin`, `category`) `t2`\n" + + "LEFT JOIN (SELECT `category`, SUM(`count()`) `__grand_total__`, ROW_NUMBER() OVER" + + " (ORDER BY SUM(`count()`) DESC) `_row_number_chart_`\n" + + "FROM (SELECT `category`, COUNT(*) `count()`\n" + + "FROM (SELECT `count`, `category`, `subcategory`, `value`," + + " SPAN_BUCKET(`value`, 50) `val_bin`\n" + + "FROM `scott`.`bounty_numbers`) `t3`\n" + + "WHERE `val_bin` IS NOT NULL\n" + + "GROUP BY `val_bin`, `category`) `t7`\n" + + "WHERE `category` IS NOT NULL\n" + + "GROUP BY `category`) `t10` ON `t2`.`category` = `t10`.`category`\n" + + "GROUP BY `t2`.`val_bin`, CASE WHEN `t2`.`category` IS NULL THEN 'NULL' WHEN" + + " `t10`.`_row_number_chart_` <= 10 THEN `t2`.`category` ELSE 'OTHER' END\n" + // Ordinal 2 refers to the CASE expression (second SELECT column) + + "ORDER BY `t2`.`val_bin` NULLS LAST, `category` NULLS LAST"; verifyPPLToSparkSQL(root, expectedSparkSql); Suggestion importance[1-10]: 2 __ Why: The ordinal 2 in ORDER BY ... 2 NULLS LAST is generated by Calcite's SQL emitter and reflects the actual query plan output. Changing it to \category` NULLS LAST` would cause the test to fail since it wouldn't match the actual generated SQL, making this suggestion potentially incorrect and of low value.	Low

Suggestions up to commit 7edbc20

Category	Suggestion	Impact
General	Fix incorrect comment about expected row count The test verifies that only non-null balance rows appear, but with nullsLast ordering, null bin values would appear at the end rather than being excluded. If the intent is to assert that null bin values are filtered out entirely, the test should also verify the total row count matches exactly 4 (no null rows present). Additionally, the comment says "4 records with balance" but the test data in the YAML fixture only shows 3 non-null value records (IDs 1, 2, 3 have values; ID 4 has null value), so the expected row count in the comment may be incorrect. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinChartNullIT.java [43-49] +// Should only contain rows for non-null balance values (3 records with balance) verifyDataRows( result, rows("0-10000", "M", 1), rows("30000-40000", "F", 1), rows("30000-40000", "M", 1), rows("40000-50000", "F", 1)); Suggestion importance[1-10]: 3 __ Why: The comment says "4 records with balance" but the test data in the YAML fixture shows only 3 non-null value records (IDs 1, 2, 3 have values; ID 4 has null value). However, the verifyDataRows call itself has 4 rows (two different genders for the 30000-40000 range), so the comment is misleading but the actual test data assertions may still be correct depending on the test index data. This is a minor comment fix with low impact.	Low
	Fix incorrect table row count statistic Using 0d as the row count statistic for a table that has 4 rows may cause the query planner to make suboptimal decisions (e.g., choosing wrong join strategies). The row count should reflect the actual number of rows in the test table to produce reliable and deterministic query plans in unit tests. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLChartNullTest.java [148] -return Statistics.of(0d, ImmutableList.of(), RelCollations.createSingleton(0)); +return Statistics.of(4d, ImmutableList.of(), RelCollations.createSingleton(0)); Suggestion importance[1-10]: 3 __ Why: Using 0d as the row count for a 4-row table could lead to suboptimal query planning decisions. Changing to 4d would better reflect the actual data, though for unit tests focused on logical plan verification rather than execution, this has limited practical impact.	Low

Suggestions up to commit ac9092c

Category	Suggestion	Impact
General	Fix incorrect row count in table statistics Using 0d as the row count statistic for a table with 4 rows may cause the query planner to make suboptimal decisions or produce unexpected plan shapes, potentially causing the logical plan verification tests to fail. The row count should reflect the actual number of rows in the test table. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLChartNullTest.java [147-149] @Override public Statistic getStatistic() { - return Statistics.of(0d, ImmutableList.of(), RelCollations.createSingleton(0)); + return Statistics.of(4d, ImmutableList.of(), RelCollations.createSingleton(0)); } Suggestion importance[1-10]: 4 __ Why: Using 0d as the row count for a 4-row table could affect query planner decisions, but in unit tests with fixed expected SQL output, the planner's row count estimate typically doesn't change the logical plan structure being verified. The fix is reasonable but unlikely to cause test failures in practice.	Low
	Verify null gender values in expected test rows The test verifies that only non-null balance rows appear, but the BANK_WITH_NULL_VALUES index may contain documents with null gender as well. If gender can be null, the expected rows may not match the actual output, causing test failures. Verify that the expected rows account for all possible null combinations in both balance and gender fields. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinChartNullIT.java [44-49] +// Verify expected rows - ensure null gender values are also accounted for if present verifyDataRows( result, rows("0-10000", "M", 1), rows("30000-40000", "F", 1), rows("30000-40000", "M", 1), rows("40000-50000", "F", 1)); +// Note: if gender can be null in BANK_WITH_NULL_VALUES, add rows(...) for null gender cases Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about null gender values in BANK_WITH_NULL_VALUES, but the 'improved_code' only adds a comment without actually changing the test logic. The suggestion is speculative and doesn't provide a concrete fix, making it low-impact.	Low

[qianheng-aws]

Decision Log

Root Cause: Bin bucket functions (SpanBucketFunction, MinspanBucketFunction, RangeBucketFunction) declared non-nullable VARCHAR(2000) return type via ReturnTypes.VARCHAR_2000. When input is null, these functions return null, but Calcite's optimizer sees the return type as NOT NULL and removes IS NOT NULL filters (from visitAggregation's nonNullGroupMask) as trivially true. Null group keys then reach TreeMap in Enumerable aggregation, crashing with NPE in compareTo.

Approach:

1. Changed return type to ReturnTypes.VARCHAR_2000.andThen(SqlTypeTransforms.FORCE_NULLABLE) on all three bucket functions — this is the root fix that lets Calcite preserve the null filter
2. Added nullsLast to chart command sort operations as a defensive measure against null keys reaching sort

Alternatives Rejected:

• Adding null checks in the bucket function implementations themselves — treats the symptom, not the root cause; Calcite would still optimize away the filter
• Adding explicit WHERE ... IS NOT NULL in visitChart — redundant if the type is correctly declared nullable, since visitAggregation already adds nonNullGroupMask filters

Pitfalls: The non-nullable return type was inherited from ReturnTypes.VARCHAR_2000 which is Calcite's built-in constant. Easy to miss that UDFs returning null for null inputs need explicit nullable declaration.

Things to Watch: Other UDFs using ReturnTypes.VARCHAR_2000 or similar non-nullable return types that can actually return null — they may have the same latent issue.

[github-actions]

Persistent review updated to latest commit 7edbc20

[github-actions]

Persistent review updated to latest commit 1f7053d

[github-actions]

Persistent review updated to latest commit d41e598

[github-actions]

Persistent review updated to latest commit ccb3283

[LantaoJin]

Is this removing on purpose?

[qianheng-aws]

Yes, #5319 has migrate to reuse the workflow in opensearch-build. The original dedupe workflow has been migrated to that repo for reusing across openseach projects.

[qianheng-aws]

This change is from skill self-improving after receiving the feedback of PR_REVEW workflow comments.

[ahkcs]

Should we add CalciteBinChartNullIT to CalciteNoPushdownIT suite?