From c89615f5a97bec8d1c2d7675fb8d1141dc25c419 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Sat, 4 Apr 2026 13:54:57 +0800 Subject: [PATCH] Fix COALESCE(null, 42) returning wrong type string instead of int (#5175) Three changes fix the type inference bug: 1. QualifiedNameResolver: Use NULL type (not VARCHAR) for null literals created from unresolved field names inside COALESCE. This allows the return type inference to correctly ignore NULL-typed operands. 2. EnhancedCoalesceFunction: Filter out NULL-typed operands before calling leastRestrictive, so COALESCE(null, 42) infers INTEGER from the non-null operand rather than falling back to VARCHAR. 3. CalciteRexNodeVisitor: Scope the inCoalesceFunction flag to direct COALESCE arguments only, clearing it for nested function calls. This prevents unresolved fields deep inside expressions like array_compact(does_not_exist) from silently becoming null. Signed-off-by: Heng Qian --- .../sql/calcite/CalciteRexNodeVisitor.java | 13 ++++- .../sql/calcite/QualifiedNameResolver.java | 4 +- .../condition/EnhancedCoalesceFunction.java | 25 +++++++-- .../CalcitePPLEnhancedCoalesceTest.java | 56 +++++++++++++++++-- 4 files changed, 85 insertions(+), 13 deletions(-) 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..977edb4b1ba 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,16 @@ public RexNode visitFunction(Function node, CalcitePlanContext context) { List arguments = new ArrayList<>(); boolean isCoalesce = "coalesce".equalsIgnoreCase(node.getFuncName()); + // Save the previous state so nested non-COALESCE functions don't inherit the flag. + // This ensures that only direct arguments of COALESCE get the null-replacement + // behavior for unresolved field names, not deeply nested expressions. + boolean wasInCoalesce = context.isInCoalesceFunction(); if (isCoalesce) { context.setInCoalesceFunction(true); + } else { + // Clear the flag for non-COALESCE nested functions so that unresolved fields + // inside e.g. array_compact(does_not_exist) still throw errors properly. + context.setInCoalesceFunction(false); } List capturedVars = null; @@ -529,9 +537,8 @@ public RexNode visitFunction(Function node, CalcitePlanContext context) { } } } finally { - if (isCoalesce) { - context.setInCoalesceFunction(false); - } + // Restore the previous inCoalesceFunction 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..9f84e507556 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java +++ b/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java @@ -315,9 +315,11 @@ private static Optional resolveLambdaVariable( private static Optional replaceWithNullLiteralInCoalesce(CalcitePlanContext context) { log.debug("replaceWithNullLiteralInCoalesce() called"); if (context.isInCoalesceFunction()) { + // Use NULL type instead of VARCHAR so that COALESCE return type inference + // can derive the correct type from the non-null operands (fixes #5175). 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..49bef244f36 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,26 @@ 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 so that leastRestrictive can find a common type + // among the concrete types. Without this, COALESCE(null, 42) would fall back to + // VARCHAR because leastRestrictive([NULL, INTEGER]) returns null. + var nonNullTypes = + operandTypes.stream().filter(t -> t.getSqlTypeName() != SqlTypeName.NULL).toList(); + + if (nonNullTypes.isEmpty()) { + // All operands are NULL literals -- return nullable NULL type + return opBinding + .getTypeFactory() + .createTypeWithNullability( + opBinding.getTypeFactory().createSqlType(SqlTypeName.NULL), true); + } + + var commonType = opBinding.getTypeFactory().leastRestrictive(nonNullTypes); + if (commonType != null) { + // Ensure the result is nullable since at least one operand could be NULL + return opBinding.getTypeFactory().createTypeWithNullability(commonType, true); + } + return opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); }; } 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..f1668ce8f61 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); @@ -187,6 +187,54 @@ public void testCoalesceWithAllNonExistentFields() { verifyPPLToSparkSQL(root, expectedSparkSql); } + @Test + public void testCoalesceNullWithInteger() { + // Regression test for #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); + // Verify the COALESCE return type is INTEGER, not VARCHAR + org.apache.calcite.rel.logical.LogicalSort sort = + (org.apache.calcite.rel.logical.LogicalSort) root; + org.apache.calcite.rel.logical.LogicalProject proj = + (org.apache.calcite.rel.logical.LogicalProject) sort.getInput(); + org.apache.calcite.rex.RexNode coalesceExpr = proj.getProjects().get(1); + org.junit.Assert.assertEquals( + org.apache.calcite.sql.type.SqlTypeName.INTEGER, coalesceExpr.getType().getSqlTypeName()); + } + + @Test + public void testCoalesceIntegerWithNull() { + // Regression test for #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); + // Verify the COALESCE return type is INTEGER, not VARCHAR + org.apache.calcite.rel.logical.LogicalSort sort = + (org.apache.calcite.rel.logical.LogicalSort) root; + org.apache.calcite.rel.logical.LogicalProject proj = + (org.apache.calcite.rel.logical.LogicalProject) sort.getInput(); + org.apache.calcite.rex.RexNode coalesceExpr = proj.getProjects().get(1); + org.junit.Assert.assertEquals( + org.apache.calcite.sql.type.SqlTypeName.INTEGER, coalesceExpr.getType().getSqlTypeName()); + } + + @Test + public void testCoalesceNullWithIntegerResult() { + // Regression test for #5175: COALESCE(null, 42) should return numeric 42, not string "42" + String ppl = "source=EMP | eval x = coalesce(null, 42) | fields x | head 1"; + RelNode root = getRelNode(ppl); + verifyResult(root, "x=42\n"); + } + @Test public void testCoalesceWithEmptyString() { String ppl = "source=EMP | eval result = coalesce('', ENAME) | fields EMPNO, result | head 1";