Fix COALESCE(null, 42) returning wrong type string instead of int

[qianheng-aws]

Summary

• Fix COALESCE(null, 42) returning string type instead of int by using SqlTypeName.NULL instead of SqlTypeName.VARCHAR for unresolved-field null literals in QualifiedNameResolver.replaceWithNullLiteralInCoalesce()
• Add fallback to VARCHAR in EnhancedCoalesceFunction.getReturnTypeInference() when all operands are NULL (leastRestrictive returns NULL type)
• Add unit, integration, and YAML REST tests for the fix

Resolves #5175

Root Cause

In PPL, null is parsed as a QualifiedName (field name), not a literal. When COALESCE encounters an unresolvable field, QualifiedNameResolver.replaceWithNullLiteralInCoalesce() creates a null literal typed as VARCHAR instead of NULL. This causes leastRestrictive([VARCHAR, INTEGER]) to return VARCHAR, producing string output for what should be an integer result.

Test plan

• Unit tests: CalcitePPLEnhancedCoalesceTest - new tests for COALESCE(null, 42) and COALESCE(42, null) asserting INTEGER type
• Unit tests: OpenSearchTypeFactoryTest - new tests for leastRestrictive with NULL type
• Unit tests: Updated existing CalcitePPLEnhancedCoalesceTest plans from null:VARCHAR to null:NULL
• Integration tests: CalcitePPLEnhancedCoalesceIT - new tests for null literal + integer, integer + null literal, nonexistent field + integer
• YAML REST test: issues/5175.yml - covers COALESCE(null, 42), COALESCE(42, null), COALESCE(nonexistent_field, 42), and COALESCE(null, 3.14)

[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: Fix COALESCE null type inference (core logic + unit tests) 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 YAML REST 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 Nullability Loss When leastRestrictive returns a non-null type, the result is returned directly without preserving nullability. The original code had the same issue, but it's worth verifying that the returned commonType has the correct nullability (nullable) set, especially since COALESCE can return NULL if all operands are NULL at runtime. var commonType = opBinding.getTypeFactory().leastRestrictive(operandTypes); if (commonType == null || commonType.getSqlTypeName() == SqlTypeName.NULL) { // Fall back to VARCHAR when all operands are NULL or no common type exists return opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); } return commonType; All-NULL Fallback When all operands are NULL type (e.g., COALESCE(null, null)), the fallback returns VARCHAR. This may be surprising behavior — the test testCoalesceWithAllNonExistentFields shows the logical plan has COALESCE(null:NULL, null:NULL, null:NULL) but the return type will be VARCHAR. It should be validated whether this is the intended behavior and whether it's consistent with what users expect. if (commonType == null || commonType.getSqlTypeName() == SqlTypeName.NULL) { // Fall back to VARCHAR when all operands are NULL or no common type exists return opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); } Test Assumption The test testCoalesceNonExistentFieldWithIntegerReturnsIntType assumes nonexistent_field resolves to NULL and that COALESCE returns 42 for all rows. This should be validated against the actual index schema to ensure nonexistent_field is truly absent and not accidentally mapped. public void testCoalesceNonExistentFieldWithIntegerReturnsIntType() throws IOException { // Regression test for https://github.com/opensearch-project/sql/issues/5175 JSONObject actual = executeQuery( String.format( "source=%s | eval x = coalesce(nonexistent_field, 42) | fields x | head 1", TEST_INDEX_STATE_COUNTRY_WITH_NULL)); verifySchema(actual, schema("x", "int")); verifyDataRows(actual, rows(42)); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Ensure fallback VARCHAR type is nullable When all operands are NULL type, the result should preserve nullability. The fallback VARCHAR type should be created with createTypeWithNullability to ensure the returned type is nullable, which is the correct behavior when all inputs are null literals. core/src/main/java/org/opensearch/sql/expression/function/udf/condition/EnhancedCoalesceFunction.java [98-102] if (commonType == null || commonType.getSqlTypeName() == SqlTypeName.NULL) { - // Fall back to VARCHAR when all operands are NULL or no common type exists - return opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); + // Fall back to nullable VARCHAR when all operands are NULL or no common type exists + return opBinding.getTypeFactory().createTypeWithNullability( + opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true); } return commonType; Suggestion importance[1-10]: 4 __ Why: While ensuring nullability for the fallback VARCHAR type is a valid concern, the suggestion has moderate impact. The createSqlType(SqlTypeName.VARCHAR) may already produce a nullable type by default in Calcite, and the primary fix in this PR is about type inference correctness rather than nullability. The improvement is minor.	Low
	Use persistent settings for test reliability Using transient cluster settings in test setup can persist across test runs if teardown fails. Consider using persistent settings or ensuring the teardown reliably resets the setting. Additionally, the teardown already resets it to false, but if setup fails mid-way, the index may not be created while the setting remains enabled. integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5175.yml [1-6] setup: - do: query.settings: body: - transient: + persistent: plugins.calcite.enabled: true Suggestion importance[1-10]: 3 __ Why: The suggestion to use persistent instead of transient settings has some merit for test reliability, but transient settings are commonly used in test setups and the teardown already resets the value. The risk of test pollution is low, and this is a minor style/reliability concern.	Low