Skip to content

Commit fc2dd11

Browse files
songkant-awssongkant-aws
authored
[BugFix] Fix COALESCE(null, int) returning string type (#5175) (#5382)
* [BugFix] Fix COALESCE(null, int) returning string type (#5175) Unresolved identifiers (including the literal `null`) encountered inside COALESCE were substituted with a null literal typed as VARCHAR. That made leastRestrictive(VARCHAR, INTEGER) fall back to VARCHAR, so COALESCE(null, 42) inferred a VARCHAR result and EnhancedCoalesceFunction coerced 42 to the string "42". Use SqlTypeName.NULL for the substituted literal so Calcite's type promotion picks the other operands' type (e.g. INTEGER), and the numeric value is preserved end-to-end. Signed-off-by: Songkan Tang <songkant@amazon.com> * [BugFix] Update testNoMvMissingField to reflect new COALESCE behavior (#5175) After switching COALESCE's unresolved-identifier fallback from VARCHAR to NULL, Calcite promotes the null literal to the array type expected by ARRAY_COMPACT. That means `nomv does_not_exist` no longer produces a planning error; it returns the COALESCE empty-string fallback instead. Update the IT to assert the new successful behavior so CI passes on this PR. Signed-off-by: Songkan Tang <songkant@amazon.com> --------- Signed-off-by: Songkan Tang <songkant@amazon.com>
1 parent 3b16951 commit fc2dd11

6 files changed

Lines changed: 202 additions & 32 deletions

File tree

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,9 +317,12 @@ private static Optional<RexNode> resolveLambdaVariable(
317317
private static Optional<RexNode> replaceWithNullLiteralInCoalesce(CalcitePlanContext context) {
318318
log.debug("replaceWithNullLiteralInCoalesce() called");
319319
if (context.isInCoalesceFunction()) {
320+
// Use SqlTypeName.NULL so the resulting literal does not bias the least-restrictive
321+
// common-type computation toward VARCHAR. See issue #5175: previously VARCHAR was used,
322+
// which caused COALESCE(null, 42) to be inferred as VARCHAR and returned as "42".
320323
return Optional.of(
321324
context.rexBuilder.makeNullLiteral(
322-
context.rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR)));
325+
context.rexBuilder.getTypeFactory().createSqlType(SqlTypeName.NULL)));
323326
}
324327
return Optional.empty();
325328
}

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

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -209,24 +209,15 @@ public void testNoMvResultUsedInComparison() throws IOException {
209209
}
210210

211211
@Test
212-
public void testNoMvMissingFieldShouldReturn4xx() throws IOException {
213-
ResponseException ex =
214-
Assertions.assertThrows(
215-
ResponseException.class,
216-
() -> executeQuery("source=" + TEST_INDEX_BANK + " | nomv does_not_exist"));
217-
218-
int status = ex.getResponse().getStatusLine().getStatusCode();
212+
public void testNoMvMissingField() throws IOException {
213+
// After issue #5175 was fixed, unresolved identifiers inside COALESCE (the rewrite target
214+
// of `nomv`) resolve to a null literal of SqlTypeName.NULL. Calcite can then promote that
215+
// null to the array type expected by ARRAY_COMPACT, so the query succeeds instead of
216+
// returning 4xx. The resulting column is the COALESCE empty-string fallback.
217+
JSONObject result =
218+
executeQuery("source=" + TEST_INDEX_BANK + " | nomv does_not_exist | head 1");
219219

220-
Assertions.assertEquals(400, status, "Unexpected status. ex=" + ex.getMessage());
221-
222-
String msg = ex.getMessage();
223-
Assertions.assertTrue(
224-
msg.contains("does_not_exist")
225-
|| msg.contains("field")
226-
|| msg.contains("Field")
227-
|| msg.contains("ARRAY_COMPACT")
228-
|| msg.contains("ARRAY"),
229-
msg);
220+
Assertions.assertTrue(result.getJSONArray("datarows").length() > 0);
230221
}
231222

232223
@Test

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

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,10 +171,63 @@ public void testCoalesceWithAllNonExistentFields() throws IOException {
171171
+ " head 1",
172172
TEST_INDEX_STATE_COUNTRY_WITH_NULL));
173173

174-
verifySchema(actual, schema("name", "string"), schema("result", "string"));
174+
// When every COALESCE operand is missing/null, the result has no known type (see #5175).
175+
verifySchema(actual, schema("name", "string"), schema("result", "undefined"));
175176
verifyDataRows(actual, rows("Jake", null));
176177
}
177178

179+
@Test
180+
public void testCoalesceWithNullLiteralAndInteger() throws IOException {
181+
// Bug #5175: COALESCE(null, 42) must return the integer 42, not the string "42".
182+
JSONObject actual =
183+
executeQuery(
184+
String.format(
185+
"source=%s | eval result = coalesce(null, 42) | fields result | head 1",
186+
TEST_INDEX_STATE_COUNTRY_WITH_NULL));
187+
188+
verifySchema(actual, schema("result", "int"));
189+
verifyDataRows(actual, rows(42));
190+
}
191+
192+
@Test
193+
public void testCoalesceWithIntegerAndNullLiteral() throws IOException {
194+
// Bug #5175: COALESCE(42, null) must return the integer 42, not the string "42".
195+
JSONObject actual =
196+
executeQuery(
197+
String.format(
198+
"source=%s | eval result = coalesce(42, null) | fields result | head 1",
199+
TEST_INDEX_STATE_COUNTRY_WITH_NULL));
200+
201+
verifySchema(actual, schema("result", "int"));
202+
verifyDataRows(actual, rows(42));
203+
}
204+
205+
@Test
206+
public void testCoalesceWithNullLiteralAndDouble() throws IOException {
207+
// Bug #5175: COALESCE(null, 3.14) must return a numeric double, not a string.
208+
JSONObject actual =
209+
executeQuery(
210+
String.format(
211+
"source=%s | eval result = coalesce(null, 3.14) | fields result | head 1",
212+
TEST_INDEX_STATE_COUNTRY_WITH_NULL));
213+
214+
verifySchema(actual, schema("result", "double"));
215+
verifyDataRows(actual, rows(3.14));
216+
}
217+
218+
@Test
219+
public void testCoalesceWithNullLiteralAndIntegerField() throws IOException {
220+
// Bug #5175: COALESCE(null, age) on an int field must keep the integer type.
221+
JSONObject actual =
222+
executeQuery(
223+
String.format(
224+
"source=%s | eval result = coalesce(null, age) | fields age, result | head 3",
225+
TEST_INDEX_STATE_COUNTRY_WITH_NULL));
226+
227+
verifySchema(actual, schema("age", "int"), schema("result", "int"));
228+
verifyDataRows(actual, rows(70, 70), rows(30, 30), rows(25, 25));
229+
}
230+
178231
@Test
179232
public void testCoalesceWithEmptyString() throws IOException {
180233

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
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) returns integer 42":
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) returns integer 42":
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 ] ] }
70+
71+
---
72+
"Issue 5175: COALESCE(null, 3.14) returns double":
73+
- skip:
74+
features:
75+
- headers
76+
- do:
77+
headers:
78+
Content-Type: 'application/json'
79+
ppl:
80+
body:
81+
query: source=issue5175 | eval x = COALESCE(null, 3.14) | fields x | head 1
82+
83+
- match: { total: 1 }
84+
- match: { schema: [ { name: x, type: double } ] }
85+
- match: { datarows: [ [ 3.14 ] ] }

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

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ public void testCoalesceWithNonExistentField() {
138138
RelNode root = getRelNode(ppl);
139139
String expectedLogical =
140140
"LogicalSort(fetch=[2])\n"
141-
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:VARCHAR, $1)])\n"
141+
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, $1)])\n"
142142
+ " LogicalTableScan(table=[[scott, EMP]])\n";
143143
verifyLogical(root, expectedLogical);
144144

@@ -155,7 +155,7 @@ public void testCoalesceWithMultipleNonExistentFields() {
155155
RelNode root = getRelNode(ppl);
156156
String expectedLogical =
157157
"LogicalSort(fetch=[1])\n"
158-
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:VARCHAR, null:VARCHAR, $1,"
158+
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, null:NULL, $1,"
159159
+ " 'fallback':VARCHAR)])\n"
160160
+ " LogicalTableScan(table=[[scott, EMP]])\n";
161161
verifyLogical(root, expectedLogical);
@@ -175,8 +175,8 @@ public void testCoalesceWithAllNonExistentFields() {
175175
RelNode root = getRelNode(ppl);
176176
String expectedLogical =
177177
"LogicalSort(fetch=[1])\n"
178-
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:VARCHAR, null:VARCHAR,"
179-
+ " null:VARCHAR)])\n"
178+
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, null:NULL,"
179+
+ " null:NULL)])\n"
180180
+ " LogicalTableScan(table=[[scott, EMP]])\n";
181181
verifyLogical(root, expectedLogical);
182182

@@ -235,4 +235,38 @@ public void testCoalesceTypeInferenceWithNonNullableOperands() {
235235
+ "LIMIT 2";
236236
verifyPPLToSparkSQL(root, expectedSparkSql);
237237
}
238+
239+
@Test
240+
public void testCoalesceWithNullLiteralAndInteger() {
241+
// Bug #5175: COALESCE(null, 42) previously inferred VARCHAR because the NULL identifier
242+
// was replaced with null:VARCHAR. The result type should be INTEGER so the value comes
243+
// back as an int.
244+
String ppl = "source=EMP | eval result = coalesce(null, 42) | fields EMPNO, result | head 1";
245+
RelNode root = getRelNode(ppl);
246+
String expectedLogical =
247+
"LogicalSort(fetch=[1])\n"
248+
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(null:NULL, 42)])\n"
249+
+ " LogicalTableScan(table=[[scott, EMP]])\n";
250+
verifyLogical(root, expectedLogical);
251+
252+
String expectedSparkSql =
253+
"SELECT `EMPNO`, COALESCE(NULL, 42) `result`\n" + "FROM `scott`.`EMP`\n" + "LIMIT 1";
254+
verifyPPLToSparkSQL(root, expectedSparkSql);
255+
}
256+
257+
@Test
258+
public void testCoalesceWithIntegerAndNullLiteral() {
259+
// Bug #5175: COALESCE(42, null) should also be typed as INTEGER, not VARCHAR.
260+
String ppl = "source=EMP | eval result = coalesce(42, null) | fields EMPNO, result | head 1";
261+
RelNode root = getRelNode(ppl);
262+
String expectedLogical =
263+
"LogicalSort(fetch=[1])\n"
264+
+ " LogicalProject(EMPNO=[$0], result=[COALESCE(42, null:NULL)])\n"
265+
+ " LogicalTableScan(table=[[scott, EMP]])\n";
266+
verifyLogical(root, expectedLogical);
267+
268+
String expectedSparkSql =
269+
"SELECT `EMPNO`, COALESCE(42, NULL) `result`\n" + "FROM `scott`.`EMP`\n" + "LIMIT 1";
270+
verifyPPLToSparkSQL(root, expectedSparkSql);
271+
}
238272
}

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

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -190,17 +190,21 @@ public void testNoMvInPipeline() {
190190

191191
@Test
192192
public void testNoMvNonExistentField() {
193+
// After issue #5175 was fixed, missing identifiers inside COALESCE resolve to a null
194+
// literal of SqlTypeName.NULL (instead of VARCHAR). This lets Calcite promote the null
195+
// to the expected array type in ARRAY_COMPACT, so the plan builds successfully and the
196+
// nomv column evaluates to the empty-string fallback from COALESCE.
193197
String ppl = "source=EMP | eval arr = array('a', 'b') | nomv does_not_exist | head 1";
198+
RelNode root = getRelNode(ppl);
194199

195-
Exception ex = assertThrows(Exception.class, () -> getRelNode(ppl));
196-
197-
String msg = String.valueOf(ex.getMessage());
198-
org.junit.Assert.assertTrue(
199-
"Expected error message to mention missing field or type error. Actual: " + msg,
200-
msg.toLowerCase().contains("does_not_exist")
201-
|| msg.toLowerCase().contains("field")
202-
|| msg.contains("ARRAY_COMPACT")
203-
|| msg.contains("ARRAY"));
200+
String expectedLogical =
201+
"LogicalSort(fetch=[1])\n"
202+
+ " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],"
203+
+ " SAL=[$5], COMM=[$6], DEPTNO=[$7], arr=[array('a', 'b')],"
204+
+ " does_not_exist=[COALESCE(ARRAY_JOIN(ARRAY_COMPACT(null:ANY ARRAY), '\n"
205+
+ "'), '':VARCHAR)])\n"
206+
+ " LogicalTableScan(table=[[scott, EMP]])\n";
207+
verifyLogical(root, expectedLogical);
204208
}
205209

206210
@Test

0 commit comments

Comments
 (0)