Repository navigation
Conversation
Calcite 1.42 allows a cast from a string to MAP or ARRAY, so leastRestrictive gave a string literal and an object or nested field the container as their common type. IN, BETWEEN and CASE then cast the string to a map or list, and the generated code failed to compile with HTTP 500. COALESCE declared a map column and failed with a ClassCastException when it took its string fallback. OpenSearchTypeFactory.leastRestrictive now returns null when a VARCHAR type meets an object or array type, which is what Calcite 1.41 returned. IN, BETWEEN and CASE return their existing 400 while analyzing. COALESCE returns a string column, as it already does for an object and a number. array() of an object and a string now fails while analyzing, as array() of an object and a number already does. Signed-off-by: Chayanin Noramuttha <cnoramut@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit 10b713d)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 10b713d
Previous suggestionsSuggestions up to commit d0063db
|
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (63.88%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #5870 +/- ##
============================================
+ Coverage 63.80% 63.88% +0.08%
- Complexity 9070 9107 +37
============================================
Files 962 962
Lines 40989 41063 +74
Branches 4660 4675 +15
============================================
+ Hits 26152 26235 +83
+ Misses 13960 13950 -10
- Partials 877 878 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // string value converts at run time. Returning null restores the Calcite 1.41 answer, so a | ||
| // string type and an object or array type have no common type. | ||
| if (mixesVarcharWithContainer(types)) { | ||
| return null; |
There was a problem hiding this comment.
This sends coalesce(obj, host) to EnhancedCoalesceFunction's fallback, createSqlType(VARCHAR), which is NOT NULL. When both fields are missing the value is null, but Calcite assumes it can't be, so where isnull(x) gets folded to false. Could the fallback be nullable when every operand is?
There was a problem hiding this comment.
Fixed. The VARCHAR fallback in EnhancedCoalesceFunction is now nullable unless some operand is NOT NULL, the same rule Calcite's own COALESCE uses, so coalesce(obj, 'z') stays NOT NULL. The fallback was already NOT NULL on main, where where isnull(x) on coalesce(obj, n) with a number field dropped the rows missing both fields, and this fixes that path too. The new case in issues/5856.yml runs both queries against a document with no fields. It fails on main at coalesce(obj, n) and on the previous head of this branch at coalesce(obj, host).
| query: "source=object_field_string_operand_5856 | eval x = coalesce(obj, 'z') | sort host | fields x" | ||
| - match: {"total": 2} | ||
| - match: {"schema": [{"name": "x", "type": "string"}]} | ||
| - match: {"datarows": [["{a=x}"], ["z"]]} |
There was a problem hiding this comment.
On 3.8 and 3.9, when every row has obj, this returned a struct ({"a": "x"}). Now it returns the string {a=x}, which is Java's Map.toString and isn't JSON. The table lists the struct → string change but not the new value format. Worth noting, or should objects render as JSON here?
There was a problem hiding this comment.
The table now shows the value as well as the type, {"a": "x", "b": 1} on 3.8, 3.9 and main against {a=x, b=1} on this branch. The struct came in with the Calcite 1.42 bump alone. 3.7 and main before the bump returned the same {a=x, b=1} string, and coalesce(obj, 1) still returns it on main. A struct column cannot hold the 'z' fallback either, which is why the query returns 500 once a row lacks obj. I kept Map.toString because JSON rendering would also change coalesce(obj, 1) and every other shape that prints {a=x} today, and I can switch it to JSON here if you would prefer that.
When leastRestrictive finds no common type, EnhancedCoalesceFunction falls back to VARCHAR. That type was NOT NULL, so Calcite folded isnull(x) on the result to false and dropped the rows where every operand was null. An object and a number already took this path on main, and the previous commit sent an object and a string down it too. The fallback is now nullable unless some operand is NOT NULL, the rule Calcite's own COALESCE uses, so coalesce(obj, 'z') stays NOT NULL. Signed-off-by: Chayanin Noramuttha <cnoramut@amazon.com>
|
Persistent review updated to latest commit 10b713d |
2 participants
Description
where f in ('x', 'y'),where f between 'a' and 'z'andeval x = case(host = 'a', f else 'z')return 500 whenfis anobjectornestedfield, withCannot cast "java.lang.String" to "java.util.Map".coalesce(f, 'z')returns 500 with aClassCastExceptiononce a row hasfmissing.Root cause. Calcite 1.42 added two branches to
SqlTypeUtil.canCastFromthat accept a string as the source of a cast toMAPorARRAY.leastRestrictiveByCastrelies on that check, so a string and an object or nested field now get the container as their common type.visitIn,visitBetweenandvisitCasereject a query only when no common type exists. They now cast the string to a map, and the generated code fails to compile.EnhancedCoalesceFunctiontakes its return type from the same call, so it declares a map column and then puts the string in it.Fix.
OpenSearchTypeFactory.leastRestrictivereturns null when a string type meets an object or array type, which is the answer Calcite 1.41 gave, so the three guards return their 400s again andcoalescefalls back to astringcolumn. That fallback is now nullable unless an operand is NOT NULL, since a NOT NULL fallback let Calcite foldwhere isnull(x)to false forx = coalesce(obj, n)onmain.Before,
source=objs | where f in ('x', 'y') | fields host, HTTP 500After, the same query, HTTP 400
Notes
OpenSearchTypeFactoryTestpins the type answers, andissues/5856.ymlruns each query end to end. Without the fix, 3 unit tests and 8 of the 9 YAML cases fail, and the keyword control passes.coalescenever goes throughCalciteRexNodeVisitor, and a stricterSqlTypeMappingRulecannot help, since the new branches return before the rule is read. The same method already special-casesVARBINARY, and anANYoperand is left to Calcite.coalesce(obj, 'z')returns astructwhile every row hasobjand 500 once a row lacks it, andarray(obj, 'z')returns an array thatmvindexfails on. This branch returns what 3.7 andmainbefore the bump in Bump Apache Calcite 1.41.0 -> 1.42.0 (CVE-2026-46718) #5619 returned, astringcolumn that renders an object as{a=x}, the same ascoalesce(obj, 1)onmaintoday.These queries ran on
3a837689c, the lastmaincommit before the Calcite 1.42 bump in #5619, onmainat779bdf5d2, and on this branch atd0063db46, each built from source with Calcite enabled. This branch returns a byte-identical response to the first column on every row. The bump commit6a07689a7gives the same status codes as779bdf5d2. The nullable fallback came afterd0063db46. By source reading it changes the declared nullability and not the values.3a837689c, before #5619779bdf5d2where obj in ('x', 'y')where obj between 'a' and 'z'case(host = 'a', obj else 'z')where items in ('x', 'y'), nestedAssertionErrorcoalesce(obj, 'z'), a row lacksobjstring,{a=x, b=1}andzstring,{a=x, b=1}andzcoalesce(obj, 'z'), every row hasobjstring,{a=x, b=1}struct,{"a": "x", "b": 1}string,{a=x, b=1}coalesce(items, 'z'), a row lacksitemsstring,[{a=x}]andzstring,[{a=x}]andzcoalesce(obj, 1)string,{a=x, b=1}and1string,{a=x, b=1}and1string,{a=x, b=1}and1array(obj, 'z')arrayarray(obj, 'z')thenmvindex(x, 1)where host in ('a', 'c'), keyword controlThe nested
inrow hits an engine assertion first on these assertion-enabled nodes and returns 400AssertionErroronmain. Without assertions the same query returns 500 onmain, reproduced at866dafcbd.lookupwithappendon a string field and an object field of the same name still returns 500, now while analyzing.unionof an object with a string returns 500 fromSchemaUnifier, which never asksleastRestrictive.Related Issues
Resolves #5856
Testing
A 105-query sweep at
866dafcbdchanged only the 500s and behaviour changes listed above and thearraycalls, and leftif,ifnull,mvappend,append,multisearchand the keyword, long and temporal paths byte-identical. The suites below ran on the final tree.:core:test,:ppl:test,:opensearch:testCalcitePPLEnhancedCoalesceIT,CalcitePPLConditionBuiltinFunctionIT,CalciteExplainIT,CalciteNoMvCommandIT,CalcitePPLLookupITandCalciteFillNullCommandIT, with their no-pushdown runs:integ-test:yamlRestTestCheck List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.