[BugFix] Fix COALESCE(null, 42) returning wrong type string instead of int (#5175)

[qianheng-aws]

Description

Fix COALESCE returning wrong type string instead of int when called with null and integer literals.

Root Cause: In PPL, null is parsed as a QualifiedName (field name), not a Literal. When COALESCE encounters an unresolvable field name like null, it falls back to replaceWithNullLiteralInCoalesce() which incorrectly created a null literal typed as VARCHAR. This caused leastRestrictive([VARCHAR, INTEGER]) to return VARCHAR, making COALESCE(null, 42) return string type.

Fix: Two-part fix:

1. Primary (QualifiedNameResolver.java): Changed the null literal type from SqlTypeName.VARCHAR to SqlTypeName.NULL (Calcite's bottom type, compatible with any type). This makes leastRestrictive([NULL, INTEGER]) correctly return INTEGER.
2. Defense-in-depth (EnhancedCoalesceFunction.java): Filter out NULL-typed operands in the return type inference before calling leastRestrictive, ensuring correct type even if NULL types slip through.

Related Issues

Resolves #5175

Check List

• New functionality includes testing
• Javadoc added for new public API
• Commits signed per DCO (-s)
• spotlessCheck passed
• Unit tests passed
• Integration tests passed
• Grammar changes: all three .g4 files synced (N/A - no grammar changes)

[qianheng-aws]

Decision Log

Root Cause: In PPL, null is not a reserved literal keyword — it's parsed as a QualifiedName (field name). When COALESCE encounters the unresolvable field name "null", QualifiedNameResolver.replaceWithNullLiteralInCoalesce() creates a null literal typed as VARCHAR instead of NULL. This causes leastRestrictive([VARCHAR, INTEGER]) to return VARCHAR, making COALESCE(null, 42) return string type instead of int.

Approach: Two-part fix:

1. Changed SqlTypeName.VARCHAR to SqlTypeName.NULL in replaceWithNullLiteralInCoalesce(). NULL is Calcite's bottom type — it's compatible with any other type, so leastRestrictive([NULL, INTEGER]) correctly returns INTEGER.
2. Added defense-in-depth in EnhancedCoalesceFunction.getReturnTypeInference(): filter out NULL-typed operands before calling leastRestrictive, handle the all-NULL case explicitly, and ensure the result is always nullable.

Alternatives Rejected:

• Making null a reserved keyword in the PPL parser: This would be a much larger change affecting the grammar and many other commands. The current approach of handling unresolvable names in COALESCE is the established pattern.
• Only fixing EnhancedCoalesceFunction without the QualifiedNameResolver change: This would leave the incorrect VARCHAR typing for non-existent fields, which could cause issues in other contexts.

Pitfalls:

• Existing tests reference null:VARCHAR in their expected logical plans for non-existent fields inside COALESCE. These needed updating to null:NULL.
• The PPL response uses PPL_SPEC.typeName() which maps INTEGER to "int" (not "integer"), so integration/YAML test assertions must use int.

Things to Watch:

• The nomv command rewrites to use COALESCE internally. The testNoMvNonExistentField test was verified to not be affected because the non-existent field error occurs at the array_compact level, before COALESCE.
• The fillnull command also uses COALESCE but doesn't reference null:VARCHAR in its tests, so no impact.

[github-actions]

PR Reviewer Guide 🔍

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: Core fix and unit tests for COALESCE null type inference Relevant files: core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java core/src/main/java/org/opensearch/sql/expression/function/udf/condition/EnhancedCoalesceFunction.java core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEnhancedCoalesceTest.java Sub-PR theme: Integration and REST API tests for COALESCE null type fix Relevant files: integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEnhancedCoalesceIT.java integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5175.yml

⚡ Recommended focus areas for review Fallback Type When leastRestrictive returns null for non-null operands (line 119), the code falls back to SqlTypeName.VARCHAR. This fallback may still produce incorrect types for non-VARCHAR operand combinations where Calcite cannot determine a common type. Consider whether a more appropriate fallback or an exception would be better here. return opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); Nullability Loss The old code returned commonType directly (which may already carry nullability info from Calcite), while the new code always wraps it with createTypeWithNullability(commonType, true). This is correct for COALESCE semantics, but it's worth verifying that forcing nullable on the result doesn't break downstream type checks or comparisons that relied on the previous non-nullable result type. return opBinding.getTypeFactory().createTypeWithNullability(commonType, true); NULL Type Compatibility Changing the null literal type from VARCHAR to NULL (Calcite's bottom type) in replaceWithNullLiteralInCoalesce may affect other code paths that call this method outside of the COALESCE context or that depend on the previous VARCHAR behavior. Verify that makeNullLiteral with SqlTypeName.NULL is well-supported across all Calcite versions and backends used in this project. return Optional.of( context.rexBuilder.makeNullLiteral( context.rexBuilder.getTypeFactory().createSqlType(SqlTypeName.NULL)));

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Ensure fallback VARCHAR type is also nullable The fallback return when commonType is null still returns a non-nullable VARCHAR type, which may reintroduce the original bug for incompatible mixed types. For consistency with the rest of the method, the fallback should also be wrapped with createTypeWithNullability(..., true) to ensure the result is always nullable. core/src/main/java/org/opensearch/sql/expression/function/udf/condition/EnhancedCoalesceFunction.java [114-119] var commonType = opBinding.getTypeFactory().leastRestrictive(nonNullTypes); if (commonType != null) { // Ensure the result is nullable since COALESCE can return NULL return opBinding.getTypeFactory().createTypeWithNullability(commonType, true); } -return opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); +return opBinding.getTypeFactory().createTypeWithNullability( + opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true); Suggestion importance[1-10]: 5 __ Why: The fallback VARCHAR return when commonType is null is not wrapped with createTypeWithNullability(..., true), which is inconsistent with the rest of the method and could cause nullability issues. This is a valid minor correctness concern, though the fallback path is rarely hit.	Low
	Reset transient setting to null instead of false The setup block enables plugins.calcite.enabled as a transient setting, but if the test fails before teardown runs, the setting may remain enabled and affect other tests. Consider using a finally-equivalent pattern or ensure the teardown is robust. Additionally, the teardown block disables the setting with false, but if it was already false before the test, this could unintentionally leave it in the correct state only by coincidence — storing and restoring the original value would be safer. integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5175.yml [1-6] -setup: +teardown: + - do: + indices.delete: + index: issue5175 + ignore_unavailable: true - do: query.settings: body: transient: - plugins.calcite.enabled: true + plugins.calcite.enabled: null Suggestion importance[1-10]: 3 __ Why: The suggestion to reset plugins.calcite.enabled to null instead of false in teardown is a reasonable practice for transient settings, but the improved_code only shows the teardown block while the existing_code shows the setup block, making the mapping inconsistent. The concern about test isolation is valid but minor.	Low