From 5cd60423de2d91b0501b74ddd8e44addb85a717d Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Sat, 4 Apr 2026 13:41:14 +0800 Subject: [PATCH] [BugFix] Fix COALESCE(null, 42) returning wrong type string instead of int (#5175) Root cause: In PPL, `null` is parsed as a QualifiedName (field name), not a literal. When COALESCE encounters an unresolvable field, the fallback method replaceWithNullLiteralInCoalesce() created a null literal typed as VARCHAR instead of NULL. This caused leastRestrictive([VARCHAR, INTEGER]) to return VARCHAR, producing string output for what should be an integer result. Fix (three parts): 1. QualifiedNameResolver: Use SqlTypeName.NULL instead of SqlTypeName.VARCHAR for null literals in COALESCE, so leastRestrictive([NULL, INTEGER]) returns INTEGER correctly. 2. EnhancedCoalesceFunction: Filter out NULL-typed operands before calling leastRestrictive in return type inference, with VARCHAR fallback when all operands are NULL. 3. CalciteRexNodeVisitor: Scope the inCoalesceFunction flag to direct COALESCE arguments only, preventing unresolved field names in nested functions from being silently replaced with null literals. Signed-off-by: Heng Qian --- .../sql/calcite/CalciteRexNodeVisitor.java | 10 +- .../sql/calcite/QualifiedNameResolver.java | 2 +- .../condition/EnhancedCoalesceFunction.java | 31 +++++- .../utils/OpenSearchTypeFactoryTest.java | 35 ++++++ .../remote/CalcitePPLEnhancedCoalesceIT.java | 55 ++++++++++ .../rest-api-spec/test/issues/5175.yml | 103 ++++++++++++++++++ .../CalcitePPLEnhancedCoalesceTest.java | 34 +++++- 7 files changed, 257 insertions(+), 13 deletions(-) create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5175.yml diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index c2ce4a740ec..838f1e223ce 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -499,8 +499,13 @@ public RexNode visitFunction(Function node, CalcitePlanContext context) { List arguments = new ArrayList<>(); boolean isCoalesce = "coalesce".equalsIgnoreCase(node.getFuncName()); + boolean wasInCoalesce = context.isInCoalesceFunction(); if (isCoalesce) { context.setInCoalesceFunction(true); + } else { + // Disable the flag for nested non-COALESCE functions so that unresolved field + // names inside e.g. array_compact(...) are not silently replaced with null. + context.setInCoalesceFunction(false); } List capturedVars = null; @@ -529,9 +534,8 @@ public RexNode visitFunction(Function node, CalcitePlanContext context) { } } } finally { - if (isCoalesce) { - context.setInCoalesceFunction(false); - } + // Restore the previous COALESCE context state + context.setInCoalesceFunction(wasInCoalesce); } // For transform/mvmap functions with captured variables, add them as additional arguments diff --git a/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java b/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java index 0e5ac4a6e05..4ff3c72896c 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java +++ b/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java @@ -317,7 +317,7 @@ private static Optional replaceWithNullLiteralInCoalesce(CalcitePlanCon if (context.isInCoalesceFunction()) { return Optional.of( context.rexBuilder.makeNullLiteral( - context.rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR))); + context.rexBuilder.getTypeFactory().createSqlType(SqlTypeName.NULL))); } return Optional.empty(); } diff --git a/core/src/main/java/org/opensearch/sql/expression/function/udf/condition/EnhancedCoalesceFunction.java b/core/src/main/java/org/opensearch/sql/expression/function/udf/condition/EnhancedCoalesceFunction.java index c6ff1a64478..40799705f26 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/udf/condition/EnhancedCoalesceFunction.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/udf/condition/EnhancedCoalesceFunction.java @@ -93,11 +93,32 @@ public SqlReturnTypeInference getReturnTypeInference() { return opBinding -> { var operandTypes = opBinding.collectOperandTypes(); - // Let Calcite determine the least restrictive common type - var commonType = opBinding.getTypeFactory().leastRestrictive(operandTypes); - return commonType != null - ? commonType - : opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); + // Filter out NULL-typed operands (from unresolved field names) so they don't + // influence the least-restrictive type. Without this, leastRestrictive([NULL, INT]) + // could return NULL instead of INT in edge cases. + var nonNullTypes = + operandTypes.stream() + .filter(t -> t.getSqlTypeName() != SqlTypeName.NULL) + .collect(java.util.stream.Collectors.toList()); + + // If all operands are NULL-typed, fall back to VARCHAR + if (nonNullTypes.isEmpty()) { + return opBinding + .getTypeFactory() + .createTypeWithNullability( + opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true); + } + + // Let Calcite determine the least restrictive common type from non-null operands + var commonType = opBinding.getTypeFactory().leastRestrictive(nonNullTypes); + if (commonType != null) { + // Result should be nullable since COALESCE may return null + return opBinding.getTypeFactory().createTypeWithNullability(commonType, true); + } + return opBinding + .getTypeFactory() + .createTypeWithNullability( + opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true); }; } diff --git a/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java b/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java index 583d1aea6cb..dc77ecc91e6 100644 --- a/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java +++ b/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java @@ -126,4 +126,39 @@ public void testLeastRestrictiveDelegatesToSuperForPlainTypes() { assertNotNull(result); assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName()); } + + @Test + public void testLeastRestrictiveNullAndIntegerReturnsInteger() { + // Regression test for https://github.com/opensearch-project/sql/issues/5175 + RelDataType nullType = TYPE_FACTORY.createSqlType(SqlTypeName.NULL); + RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(nullType, intType)); + + assertNotNull(result); + assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName()); + } + + @Test + public void testLeastRestrictiveIntegerAndNullReturnsInteger() { + // Regression test for https://github.com/opensearch-project/sql/issues/5175 + RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER); + RelDataType nullType = TYPE_FACTORY.createSqlType(SqlTypeName.NULL); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(intType, nullType)); + + assertNotNull(result); + assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName()); + } + + @Test + public void testLeastRestrictiveNullAndDoubleReturnsDouble() { + RelDataType nullType = TYPE_FACTORY.createSqlType(SqlTypeName.NULL); + RelDataType doubleType = TYPE_FACTORY.createSqlType(SqlTypeName.DOUBLE); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(nullType, doubleType)); + + assertNotNull(result); + assertEquals(SqlTypeName.DOUBLE, result.getSqlTypeName()); + } } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEnhancedCoalesceIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEnhancedCoalesceIT.java index f1b546a5681..77651b0c206 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEnhancedCoalesceIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEnhancedCoalesceIT.java @@ -242,6 +242,61 @@ public void testCoalesceTypeCoercionWithMixedTypes() throws IOException { verifyDataRows(actual, rows(70, "70"), rows(30, "30")); } + @Test + public void testCoalesceWithNullAndIntegerLiteral() throws IOException { + // Regression test for https://github.com/opensearch-project/sql/issues/5175 + // COALESCE(null, 42) should return integer type, not string + JSONObject actual = + executeQuery( + String.format( + "source=%s | eval x = coalesce(null, 42) | fields x | head 1", + TEST_INDEX_STATE_COUNTRY_WITH_NULL)); + + verifySchema(actual, schema("x", "int")); + verifyDataRows(actual, rows(42)); + } + + @Test + public void testCoalesceWithIntegerAndNullLiteral() throws IOException { + // Regression test for https://github.com/opensearch-project/sql/issues/5175 + // COALESCE(42, null) should return integer type, not string + JSONObject actual = + executeQuery( + String.format( + "source=%s | eval x = coalesce(42, null) | fields x | head 1", + TEST_INDEX_STATE_COUNTRY_WITH_NULL)); + + verifySchema(actual, schema("x", "int")); + verifyDataRows(actual, rows(42)); + } + + @Test + public void testCoalesceWithNonExistentFieldAndIntegerLiteral() throws IOException { + // Regression test for https://github.com/opensearch-project/sql/issues/5175 + // COALESCE(nonexistent_field, 42) should return integer type + 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)); + } + + @Test + public void testCoalesceWithNullAndDoubleLiteral() throws IOException { + // Regression test for https://github.com/opensearch-project/sql/issues/5175 + JSONObject actual = + executeQuery( + String.format( + "source=%s | eval x = coalesce(null, 3.14) | fields x | head 1", + TEST_INDEX_STATE_COUNTRY_WITH_NULL)); + + verifySchema(actual, schema("x", "double")); + verifyDataRows(actual, rows(3.14)); + } + @Test public void testCoalesceWithCompatibleNumericAndTemporalTypes() throws IOException { JSONObject actual = diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5175.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5175.yml new file mode 100644 index 00000000000..855d53535a8 --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5175.yml @@ -0,0 +1,103 @@ +# Issue: https://github.com/opensearch-project/sql/issues/5175 +# COALESCE(null, 42) returns wrong type string instead of int due to +# leastRestrictive type inference when null is typed as VARCHAR. + +setup: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled: true + - do: + indices.create: + index: test_issue_5175 + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + mappings: + properties: + dummy: + type: keyword + - do: + bulk: + index: test_issue_5175 + refresh: true + body: + - '{"index": {}}' + - '{"dummy": "row"}' + +--- +teardown: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled: false + - do: + indices.delete: + index: test_issue_5175 + +--- +"COALESCE(null, 42) should return integer type": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=test_issue_5175 | eval x = coalesce(null, 42) | fields x | head 1 + - match: { total: 1 } + - length: { datarows: 1 } + - match: { "schema": [{"name": "x", "type": "int"}] } + - match: { "datarows": [[42]] } + +--- +"COALESCE(42, null) should return integer type": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=test_issue_5175 | eval x = coalesce(42, null) | fields x | head 1 + - match: { total: 1 } + - length: { datarows: 1 } + - match: { "schema": [{"name": "x", "type": "int"}] } + - match: { "datarows": [[42]] } + +--- +"COALESCE(nonexistent_field, 42) should return integer type": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=test_issue_5175 | eval x = coalesce(nonexistent_field, 42) | fields x | head 1 + - match: { total: 1 } + - length: { datarows: 1 } + - match: { "schema": [{"name": "x", "type": "int"}] } + - match: { "datarows": [[42]] } + +--- +"COALESCE(null, 3.14) should return double type": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=test_issue_5175 | eval x = coalesce(null, 3.14) | fields x | head 1 + - match: { total: 1 } + - length: { datarows: 1 } + - match: { "schema": [{"name": "x", "type": "double"}] } + - match: { "datarows": [[3.14]] } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEnhancedCoalesceTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEnhancedCoalesceTest.java index 56141eae584..ae64ad81df7 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEnhancedCoalesceTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEnhancedCoalesceTest.java @@ -138,7 +138,7 @@ public void testCoalesceWithNonExistentField() { RelNode root = getRelNode(ppl); String expectedLogical = "LogicalSort(fetch=[2])\n" - + " LogicalProject(EMPNO=[$0], result=[COALESCE(null:VARCHAR, $1)])\n" + + " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, $1)])\n" + " LogicalTableScan(table=[[scott, EMP]])\n"; verifyLogical(root, expectedLogical); @@ -155,7 +155,7 @@ public void testCoalesceWithMultipleNonExistentFields() { RelNode root = getRelNode(ppl); String expectedLogical = "LogicalSort(fetch=[1])\n" - + " LogicalProject(EMPNO=[$0], result=[COALESCE(null:VARCHAR, null:VARCHAR, $1," + + " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, null:NULL, $1," + " 'fallback':VARCHAR)])\n" + " LogicalTableScan(table=[[scott, EMP]])\n"; verifyLogical(root, expectedLogical); @@ -175,8 +175,8 @@ public void testCoalesceWithAllNonExistentFields() { RelNode root = getRelNode(ppl); String expectedLogical = "LogicalSort(fetch=[1])\n" - + " LogicalProject(EMPNO=[$0], result=[COALESCE(null:VARCHAR, null:VARCHAR," - + " null:VARCHAR)])\n" + + " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, null:NULL," + + " null:NULL)])\n" + " LogicalTableScan(table=[[scott, EMP]])\n"; verifyLogical(root, expectedLogical); @@ -217,6 +217,32 @@ public void testCoalesceWithSpaceString() { verifyPPLToSparkSQL(root, expectedSparkSql); } + @Test + public void testCoalesceWithNullAndIntegerLiteral() { + // Regression test for https://github.com/opensearch-project/sql/issues/5175 + // COALESCE(null, 42) should return INTEGER, not VARCHAR + String ppl = "source=EMP | eval x = coalesce(null, 42) | fields EMPNO, x | head 1"; + RelNode root = getRelNode(ppl); + String expectedLogical = + "LogicalSort(fetch=[1])\n" + + " LogicalProject(EMPNO=[$0], x=[COALESCE(null:NULL, 42)])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + verifyLogical(root, expectedLogical); + } + + @Test + public void testCoalesceWithIntegerAndNullLiteral() { + // Regression test for https://github.com/opensearch-project/sql/issues/5175 + // COALESCE(42, null) should return INTEGER, not VARCHAR + String ppl = "source=EMP | eval x = coalesce(42, null) | fields EMPNO, x | head 1"; + RelNode root = getRelNode(ppl); + String expectedLogical = + "LogicalSort(fetch=[1])\n" + + " LogicalProject(EMPNO=[$0], x=[COALESCE(42, null:NULL)])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + verifyLogical(root, expectedLogical); + } + @Test public void testCoalesceTypeInferenceWithNonNullableOperands() { String ppl =