Skip to content

Commit 5607bdc

Browse files
qianheng-awsqianheng-aws
committed
[BugFix] Fix COALESCE(null, 42) returning wrong type string instead of int (#5175)
In PPL, null is parsed as a QualifiedName (field name), not a literal. When COALESCE encounters an unresolvable field, it falls back to replaceWithNullLiteralInCoalesce() which created a null literal typed as VARCHAR. This caused leastRestrictive([VARCHAR, INTEGER]) to return VARCHAR, making COALESCE(null, 42) return string type instead of int. Fix: Change the null literal type from SqlTypeName.VARCHAR to SqlTypeName.NULL (Calcite's bottom type, compatible with any type). Also add defense-in-depth: filter out NULL-typed operands in EnhancedCoalesceFunction's return type inference before calling leastRestrictive. Signed-off-by: Heng Qian <qianheng@amazon.com>
1 parent c2c97db commit 5607bdc

File tree

6 files changed

+273
-10
lines changed

6 files changed

+273
-10
lines changed

‎core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,7 @@ private static Optional<RexNode> replaceWithNullLiteralInCoalesce(CalcitePlanCon
317317
if (context.isInCoalesceFunction()) {
318318
return Optional.of(
319319
context.rexBuilder.makeNullLiteral(
320-
context.rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR)));
320+
context.rexBuilder.getTypeFactory().createSqlType(SqlTypeName.NULL)));
321321
}
322322
return Optional.empty();
323323
}

‎core/src/main/java/org/opensearch/sql/expression/function/udf/condition/EnhancedCoalesceFunction.java‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import java.util.List;
99
import java.util.function.Supplier;
10+
import java.util.stream.Collectors;
1011
import org.apache.calcite.adapter.enumerable.NotNullImplementor;
1112
import org.apache.calcite.adapter.enumerable.NullPolicy;
1213
import org.apache.calcite.linq4j.tree.Expression;
@@ -93,11 +94,29 @@ public SqlReturnTypeInference getReturnTypeInference() {
9394
return opBinding -> {
9495
var operandTypes = opBinding.collectOperandTypes();
9596

96-
// Let Calcite determine the least restrictive common type
97-
var commonType = opBinding.getTypeFactory().leastRestrictive(operandTypes);
98-
return commonType != null
99-
? commonType
100-
: opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR);
97+
// Filter out NULL-typed operands so they don't influence type inference.
98+
// NULL is Calcite's bottom type and should be compatible with any type;
99+
// without filtering, leastRestrictive may fall back to VARCHAR.
100+
var nonNullTypes =
101+
operandTypes.stream()
102+
.filter(t -> t.getSqlTypeName() != SqlTypeName.NULL)
103+
.collect(Collectors.toList());
104+
105+
if (nonNullTypes.isEmpty()) {
106+
// All operands are NULL — return nullable NULL type
107+
return opBinding
108+
.getTypeFactory()
109+
.createTypeWithNullability(
110+
opBinding.getTypeFactory().createSqlType(SqlTypeName.NULL), true);
111+
}
112+
113+
// Let Calcite determine the least restrictive common type among non-null operands
114+
var commonType = opBinding.getTypeFactory().leastRestrictive(nonNullTypes);
115+
if (commonType != null) {
116+
// Ensure the result is nullable since COALESCE can return NULL
117+
return opBinding.getTypeFactory().createTypeWithNullability(commonType, true);
118+
}
119+
return opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR);
101120
};
102121
}
103122

‎core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,4 +126,48 @@ public void testLeastRestrictiveDelegatesToSuperForPlainTypes() {
126126
assertNotNull(result);
127127
assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName());
128128
}
129+
130+
@Test
131+
public void testLeastRestrictiveNullAndIntegerReturnsInteger() {
132+
RelDataType nullType = TYPE_FACTORY.createSqlType(SqlTypeName.NULL);
133+
RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER);
134+
135+
RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(nullType, intType));
136+
137+
assertNotNull("leastRestrictive(NULL, INTEGER) should not be null", result);
138+
assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName());
139+
}
140+
141+
@Test
142+
public void testLeastRestrictiveNullAndDecimalReturnsDecimal() {
143+
RelDataType nullType = TYPE_FACTORY.createSqlType(SqlTypeName.NULL);
144+
RelDataType decType = TYPE_FACTORY.createSqlType(SqlTypeName.DECIMAL, 10, 0);
145+
146+
RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(nullType, decType));
147+
148+
assertNotNull("leastRestrictive(NULL, DECIMAL) should not be null", result);
149+
assertEquals(SqlTypeName.DECIMAL, result.getSqlTypeName());
150+
}
151+
152+
@Test
153+
public void testLeastRestrictiveNullAndBooleanReturnsBoolean() {
154+
RelDataType nullType = TYPE_FACTORY.createSqlType(SqlTypeName.NULL);
155+
RelDataType boolType = TYPE_FACTORY.createSqlType(SqlTypeName.BOOLEAN);
156+
157+
RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(nullType, boolType));
158+
159+
assertNotNull("leastRestrictive(NULL, BOOLEAN) should not be null", result);
160+
assertEquals(SqlTypeName.BOOLEAN, result.getSqlTypeName());
161+
}
162+
163+
@Test
164+
public void testLeastRestrictiveMultipleNullsAndIntegerReturnsInteger() {
165+
RelDataType nullType = TYPE_FACTORY.createSqlType(SqlTypeName.NULL);
166+
RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER);
167+
168+
RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(nullType, nullType, intType));
169+
170+
assertNotNull("leastRestrictive(NULL, NULL, INTEGER) should not be null", result);
171+
assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName());
172+
}
129173
}

‎integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEnhancedCoalesceIT.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,4 +259,43 @@ public void testCoalesceWithCompatibleNumericAndTemporalTypes() throws IOExcepti
259259
schema("result", "int"));
260260
verifyDataRows(actual, rows(70, 2023, 4, 70), rows(30, 2023, 4, 30));
261261
}
262+
263+
/** Issue #5175: COALESCE(null, 42) should return int type, not string. */
264+
@Test
265+
public void testCoalesceNullLiteralWithIntegerReturnsIntegerType() throws IOException {
266+
JSONObject actual =
267+
executeQuery(
268+
String.format(
269+
"source=%s | eval x = coalesce(null, 42) | fields x | head 1",
270+
TEST_INDEX_STATE_COUNTRY_WITH_NULL));
271+
272+
verifySchema(actual, schema("x", "int"));
273+
verifyDataRows(actual, rows(42));
274+
}
275+
276+
/** Issue #5175: COALESCE(42, null) should return int type, not string. */
277+
@Test
278+
public void testCoalesceIntegerWithNullLiteralReturnsIntegerType() throws IOException {
279+
JSONObject actual =
280+
executeQuery(
281+
String.format(
282+
"source=%s | eval x = coalesce(42, null) | fields x | head 1",
283+
TEST_INDEX_STATE_COUNTRY_WITH_NULL));
284+
285+
verifySchema(actual, schema("x", "int"));
286+
verifyDataRows(actual, rows(42));
287+
}
288+
289+
/** Issue #5175: COALESCE(nonexistent_field, 42) should return int type, not string. */
290+
@Test
291+
public void testCoalesceNonExistentFieldWithIntegerReturnsIntegerType() throws IOException {
292+
JSONObject actual =
293+
executeQuery(
294+
String.format(
295+
"source=%s | eval x = coalesce(nonexistent_field, 42) | fields x | head 1",
296+
TEST_INDEX_STATE_COUNTRY_WITH_NULL));
297+
298+
verifySchema(actual, schema("x", "int"));
299+
verifyDataRows(actual, rows(42));
300+
}
262301
}

‎integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5175.yml‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
setup:
2+
- do:
3+
query.settings:
4+
body:
5+
transient:
6+
plugins.calcite.enabled: true
7+
8+
- do:
9+
indices.create:
10+
index: issue5175
11+
body:
12+
settings:
13+
number_of_shards: 1
14+
number_of_replicas: 0
15+
mappings:
16+
properties:
17+
dummy:
18+
type: keyword
19+
20+
- do:
21+
bulk:
22+
refresh: true
23+
body:
24+
- '{"index": {"_index": "issue5175", "_id": "1"}}'
25+
- '{"dummy": "row"}'
26+
27+
---
28+
teardown:
29+
- do:
30+
indices.delete:
31+
index: issue5175
32+
ignore_unavailable: true
33+
- do:
34+
query.settings:
35+
body:
36+
transient:
37+
plugins.calcite.enabled: false
38+
39+
---
40+
"Issue 5175: COALESCE(null, 42) should return int type not string":
41+
- skip:
42+
features:
43+
- headers
44+
- do:
45+
headers:
46+
Content-Type: 'application/json'
47+
ppl:
48+
body:
49+
query: source=issue5175 | eval x = coalesce(null, 42) | fields x | head 1
50+
51+
- match: { total: 1 }
52+
- match: { schema: [ { name: x, type: int } ] }
53+
- match: { datarows: [ [ 42 ] ] }
54+
55+
---
56+
"Issue 5175: COALESCE(42, null) should return int type not string":
57+
- skip:
58+
features:
59+
- headers
60+
- do:
61+
headers:
62+
Content-Type: 'application/json'
63+
ppl:
64+
body:
65+
query: source=issue5175 | eval x = coalesce(42, null) | fields x | head 1
66+
67+
- match: { total: 1 }
68+
- match: { schema: [ { name: x, type: int } ] }
69+
- match: { datarows: [ [ 42 ] ] }

‎ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEnhancedCoalesceTest.java‎

Lines changed: 96 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,14 @@
55

66
package org.opensearch.sql.ppl.calcite;
77

8+
import static org.junit.Assert.assertEquals;
9+
import static org.junit.Assert.assertNotEquals;
10+
import static org.junit.Assert.assertTrue;
11+
812
import org.apache.calcite.rel.RelNode;
13+
import org.apache.calcite.rel.type.RelDataType;
14+
import org.apache.calcite.sql.type.SqlTypeName;
15+
import org.apache.calcite.sql.type.SqlTypeUtil;
916
import org.apache.calcite.test.CalciteAssert;
1017
import org.junit.Test;
1118

@@ -138,7 +145,7 @@ public void testCoalesceWithNonExistentField() {
138145
RelNode root = getRelNode(ppl);
139146
String expectedLogical =
140147
"LogicalSort(fetch=[2])\n"
141-
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:VARCHAR, $1)])\n"
148+
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, $1)])\n"
142149
+ " LogicalTableScan(table=[[scott, EMP]])\n";
143150
verifyLogical(root, expectedLogical);
144151

@@ -155,7 +162,7 @@ public void testCoalesceWithMultipleNonExistentFields() {
155162
RelNode root = getRelNode(ppl);
156163
String expectedLogical =
157164
"LogicalSort(fetch=[1])\n"
158-
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:VARCHAR, null:VARCHAR, $1,"
165+
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, null:NULL, $1,"
159166
+ " 'fallback':VARCHAR)])\n"
160167
+ " LogicalTableScan(table=[[scott, EMP]])\n";
161168
verifyLogical(root, expectedLogical);
@@ -175,8 +182,8 @@ public void testCoalesceWithAllNonExistentFields() {
175182
RelNode root = getRelNode(ppl);
176183
String expectedLogical =
177184
"LogicalSort(fetch=[1])\n"
178-
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:VARCHAR, null:VARCHAR,"
179-
+ " null:VARCHAR)])\n"
185+
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, null:NULL,"
186+
+ " null:NULL)])\n"
180187
+ " LogicalTableScan(table=[[scott, EMP]])\n";
181188
verifyLogical(root, expectedLogical);
182189

@@ -235,4 +242,89 @@ public void testCoalesceTypeInferenceWithNonNullableOperands() {
235242
+ "LIMIT 2";
236243
verifyPPLToSparkSQL(root, expectedSparkSql);
237244
}
245+
246+
/** Verifies that COALESCE(null, 42) infers a numeric type, not VARCHAR (issue #5175). */
247+
@Test
248+
public void testCoalesceNullLiteralWithIntegerPreservesIntegerType() {
249+
String ppl = "source=EMP | eval x = coalesce(null, 42) | fields x | head 1";
250+
RelNode root = getRelNode(ppl);
251+
RelDataType rowType = root.getRowType();
252+
RelDataType xType = rowType.getField("x", true, false).getType();
253+
assertNotEquals(
254+
"COALESCE(null, 42) must not return VARCHAR",
255+
SqlTypeName.VARCHAR,
256+
xType.getSqlTypeName());
257+
assertTrue(
258+
"COALESCE(null, 42) should return a numeric type, got " + xType.getSqlTypeName(),
259+
SqlTypeUtil.isNumeric(xType));
260+
}
261+
262+
/** Verifies that COALESCE(42, null) infers a numeric type, not VARCHAR. */
263+
@Test
264+
public void testCoalesceIntegerWithNullLiteralPreservesIntegerType() {
265+
String ppl = "source=EMP | eval x = coalesce(42, null) | fields x | head 1";
266+
RelNode root = getRelNode(ppl);
267+
RelDataType rowType = root.getRowType();
268+
RelDataType xType = rowType.getField("x", true, false).getType();
269+
assertNotEquals(
270+
"COALESCE(42, null) must not return VARCHAR",
271+
SqlTypeName.VARCHAR,
272+
xType.getSqlTypeName());
273+
assertTrue(
274+
"COALESCE(42, null) should return a numeric type, got " + xType.getSqlTypeName(),
275+
SqlTypeUtil.isNumeric(xType));
276+
}
277+
278+
/** Verifies that COALESCE(null, 42) returns numeric 42, not string "42". */
279+
@Test
280+
public void testCoalesceNullAndIntegerLiteralReturnsCorrectValue() {
281+
String ppl = "source=EMP | eval x = coalesce(null, 42) | fields x | head 1";
282+
RelNode root = getRelNode(ppl);
283+
verifyResult(root, "x=42\n");
284+
}
285+
286+
/** Verifies that COALESCE(null, 3.14) infers a numeric type. */
287+
@Test
288+
public void testCoalesceNullLiteralWithDoublePreservesNumericType() {
289+
String ppl = "source=EMP | eval x = coalesce(null, 3.14) | fields x | head 1";
290+
RelNode root = getRelNode(ppl);
291+
RelDataType rowType = root.getRowType();
292+
RelDataType xType = rowType.getField("x", true, false).getType();
293+
assertNotEquals(
294+
"COALESCE(null, 3.14) must not return VARCHAR",
295+
SqlTypeName.VARCHAR,
296+
xType.getSqlTypeName());
297+
assertTrue(
298+
"COALESCE(null, 3.14) should return a numeric type, got " + xType.getSqlTypeName(),
299+
SqlTypeUtil.isNumeric(xType));
300+
}
301+
302+
/** Verifies that COALESCE(null, null, 42) still returns a numeric type. */
303+
@Test
304+
public void testCoalesceMultipleNullsWithIntegerPreservesIntegerType() {
305+
String ppl = "source=EMP | eval x = coalesce(null, null, 42) | fields x | head 1";
306+
RelNode root = getRelNode(ppl);
307+
RelDataType rowType = root.getRowType();
308+
RelDataType xType = rowType.getField("x", true, false).getType();
309+
assertNotEquals(
310+
"COALESCE(null, null, 42) must not return VARCHAR",
311+
SqlTypeName.VARCHAR,
312+
xType.getSqlTypeName());
313+
assertTrue(
314+
"COALESCE(null, null, 42) should return a numeric type, got " + xType.getSqlTypeName(),
315+
SqlTypeUtil.isNumeric(xType));
316+
}
317+
318+
/** Verifies that COALESCE(null, true) returns BOOLEAN type. */
319+
@Test
320+
public void testCoalesceNullLiteralWithBooleanPreservesBooleanType() {
321+
String ppl = "source=EMP | eval x = coalesce(null, true) | fields x | head 1";
322+
RelNode root = getRelNode(ppl);
323+
RelDataType rowType = root.getRowType();
324+
RelDataType xType = rowType.getField("x", true, false).getType();
325+
assertEquals(
326+
"COALESCE(null, true) should return BOOLEAN type",
327+
SqlTypeName.BOOLEAN,
328+
xType.getSqlTypeName());
329+
}
238330
}

0 commit comments

Comments
 (0)