From b01dae816018e80423a9b370430f51c46c4d0a2e Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Fri, 19 Jun 2026 16:37:19 -0700 Subject: [PATCH] Fix NPE on case() with incompatible branch types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CalciteRexNodeVisitor.visitCase passed the THEN/ELSE operands straight to rexBuilder.makeCall(CASE, ...) without checking that the result types have a common supertype. When the types have none (e.g. VARCHAR + INTEGER + DECIMAL), Calcite's return-type inference calls leastRestrictive, gets null back, and downstream code dereferences it — which surfaces as an opaque NullPointerException at HTTP 500. The dev comment at PPLFuncImpTable.java:1153-1155 anticipated this for the 3-arg IF form but the multi-arg case() bypasses that checker. Pre-validate types in visitCase and throw a clean ExpressionEvaluationException ("case branches must have a common type, but got [VARCHAR, INTEGER, DECIMAL(2,1), NULL]") before reaching the broken makeCall path. Maps to HTTP 400 with a readable message. Tests: - Unit test in CalcitePPLCaseFunctionTest exercises the mixed-type case() form and asserts the clean exception type + message. - IT in CalcitePPLCaseFunctionIT confirms the end-to-end HTTP response is 4xx rather than 500. Signed-off-by: Jialiang Liang --- .../sql/calcite/CalciteRexNodeVisitor.java | 14 +++++++++++++- .../remote/CalcitePPLCaseFunctionIT.java | 17 +++++++++++++++++ .../ppl/calcite/CalcitePPLCaseFunctionTest.java | 16 ++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) 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 44c6d87da12..3c37a11ba5b 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -835,6 +835,7 @@ public RexNode visitCast(Cast node, CalcitePlanContext context) { @Override public RexNode visitCase(Case node, CalcitePlanContext context) { List caseOperands = new ArrayList<>(); + List resultTypes = new ArrayList<>(); for (When when : node.getWhenClauses()) { RexNode condition = analyze(when.getCondition(), context); if (!SqlTypeUtil.isBoolean(condition.getType())) { @@ -843,11 +844,22 @@ public RexNode visitCase(Case node, CalcitePlanContext context) { "Condition expected a boolean type, but got %s", condition.getType())); } caseOperands.add(condition); - caseOperands.add(analyze(when.getResult(), context)); + RexNode result = analyze(when.getResult(), context); + caseOperands.add(result); + resultTypes.add(result.getType()); } RexNode elseExpr = node.getElseClause().map(e -> analyze(e, context)).orElse(context.relBuilder.literal(null)); caseOperands.add(elseExpr); + resultTypes.add(elseExpr.getType()); + + // Pre-validate the THEN/ELSE result types so an unsupertyped mix surfaces as a clean + // 400 here instead of an opaque NPE deep in Calcite's makeCall return-type inference. + RelDataType commonType = context.rexBuilder.getTypeFactory().leastRestrictive(resultTypes); + if (commonType == null) { + throw new ExpressionEvaluationException( + StringUtils.format("case branches must have a common type, but got %s", resultTypes)); + } return context.rexBuilder.makeCall(SqlStdOperatorTable.CASE, caseOperands); } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLCaseFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLCaseFunctionIT.java index bc9d4388d5b..8bba907e2e2 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLCaseFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLCaseFunctionIT.java @@ -537,4 +537,21 @@ public void testNestedCaseAggWithAutoDateHistogram() throws IOException { schema("flags", "bigint")); verifyNumOfRows(actual2, 32); } + + /** Case with no common branch supertype must return a clean 4xx, not a 500. */ + @Test + public void testCaseWithIncompatibleBranchTypesRejectsCleanly() { + org.opensearch.client.ResponseException e = + org.junit.Assert.assertThrows( + org.opensearch.client.ResponseException.class, + () -> + executeQuery( + String.format( + "source=%s | eval x = case(age > 30, 'old', age > 20, 1 else 0.0) | fields" + + " x", + TEST_INDEX_BANK))); + org.junit.Assert.assertTrue( + "expected 400 status, got: " + e.getMessage(), + e.getMessage().contains("status line [HTTP/1.1 400")); + } } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCaseFunctionTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCaseFunctionTest.java index 5cc257b0b0e..2788f7a9cfe 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCaseFunctionTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCaseFunctionTest.java @@ -5,9 +5,13 @@ package org.opensearch.sql.ppl.calcite; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + import org.apache.calcite.rel.RelNode; import org.apache.calcite.test.CalciteAssert; import org.junit.Test; +import org.opensearch.sql.exception.ExpressionEvaluationException; public class CalcitePPLCaseFunctionTest extends CalcitePPLAbstractTest { @@ -101,4 +105,16 @@ public void testCaseWhenInSubquery() { + "FROM `scott`.`EMP`)"; verifyPPLToSparkSQL(root, expectedSparkSql); } + + /** Case branches with no common supertype must be rejected cleanly, not NPE. */ + @Test + public void testCaseWithIncompatibleBranchTypesRejectsCleanly() { + String ppl = + "source=EMP | eval x = case(DEPTNO > 20, 'big'," + " DEPTNO > 10, 1 else 0.0) | fields x"; + ExpressionEvaluationException e = + assertThrows(ExpressionEvaluationException.class, () -> getRelNode(ppl)); + assertTrue( + "expected message to list incompatible types, got: " + e.getMessage(), + e.getMessage().contains("case branches must have a common type")); + } }