feat(isthmus): convert REVERSE and INITCAP from Calcite - #1251
Conversation
FunctionConverter builds its signature map from the FunctionMappings list alone, with no fall-back matching an operator to a same-named Substrait function, so an operator missing from that table has no finder and every call to it fails. Both functions are declared in functions_string.yaml with character implementations and both have a Calcite operator, so the entry joining them is all that was missing. The round-trip tests fail without the two entries with "Unable to convert call REVERSE(char<16>?)" and the same for INITCAP. Closes substrait-io#1222
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Map reverse to an Isthmus-local operator instead of SqlLibraryOperators.REVERSE — as written it is unreachable in an ORDER BY and emits a non-conformant output type, and one change fixes both. initcap is correct as it stands.
One note on the PR body, since title + body become the squash-merge commit that semantic-release turns into CHANGELOG.md: "exactly the ones you named" has no referent in git log, and the round-trip-test paragraph is what the diff already shows. The FunctionConverter-builds-its-map-from-FunctionMappings-alone sentence is the rationale worth keeping, and Unable to convert call REVERSE(char<16>?) is worth keeping as the symptom.
SqlLibraryOperators.REVERSE was wrong on two counts. Its ReturnTypes.ARG0_NULLABLE_VARYING widens a CHAR to VARCHAR, so reverse(c16) emitted reverse:fchar with output_type varchar<16> where the declaration says fixedchar<L1>, against algebra.proto's "exactly as derived using the declaration in the extension". And Calcite names the Spark library's operator REVERSE too, so with both libraries enabled a lookup by that name returns two candidates, filterOperatorRoutinesByKind keeps neither, and every ORDER BY over it fails validation with "No match found for function signature REVERSE(<CHARACTER>)". An Isthmus-local operator with ARG0_NULLABLE fixes the return, and registering it in SUBSTRAIT_SCALAR_OPERATOR_TABLE, which is consulted first, fixes the lookup. This is the shape RIGHTSHIFT already uses in the same file. A round trip cannot see either fault: both directions carry the recorded output_type verbatim, so the widened call round-trips green. The two new tests assert the invocation and the three ORDER BY forms directly, and all four cases fail on the library operator. The third column of the fixture is covered too, so initcap:str and reverse:str are no longer the only implementations untested.
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Verified the fix: the local operator and the SUBSTRAIT_SCALAR_OPERATOR_TABLE entry both landed as discussed, and I reproduced your negative result — reverting just the two main-source files fails all four new cases, expected: <FixedChar{nullable=true, length=16}> but was: <VarChar{nullable=true, length=16}> plus the three ORDER BY forms. :isthmus:check and :isthmus-cli:check are green on the merge with current main, which has since moved to substrait-packaging 0.102.0 (functions_string.yaml is byte-identical to 0.101.0, so the declarations the change relies on are unchanged).
One small documentation point inline. The rewritten PR body reads well and Unable to convert call REVERSE(char<16>?) is the right thing to have kept.
Calcite's Enumerable convention cannot execute it: unlike the library operator it replaces, it has no RexImpTable implementor. RIGHTSHIFT and DATETIME_SUBTRACT replaced nothing, so their own lack of one costs nothing; this one displaces an operator that had it, and that belongs in the Javadoc rather than in a stack trace.
2 participants
REVERSEandINITCAPhad noFunctionMappingsentry, andFunctionConverterbuilds its signature map from that list alone, so neither had aFunctionFinderand no call to either converted.functions_string.yamldeclares both, so the mapping was the whole gap; without it a call fails withUnable to convert call REVERSE(char<16>?).INITCAPbinds Calcite's operator.REVERSEbinds a localSqlFunctionwithARG0_NULLABLE, registered inSUBSTRAIT_SCALAR_OPERATOR_TABLEwhere a lookup reaches it before the libraries, because Calcite's is wrong twice over.SqlLibraryOperators.REVERSEinfers its return withARG0_NULLABLE_VARYING, which widens aCHAR:reverse(c16)bound thefixedcharvariant and then recordedvarchar<16>as its output type, where the declaredfixedchar<L1>makes itfixedchar<16>. And Calcite gives the Spark library's operator the same name, so with both libraries enabled the validator finds two candidates and discards both, which is why anORDER BYover the call failed to validate. The same collision catchesleastandgreatest, filed as #1274.Neither fault is visible to a round trip, which carries the recorded
output_typeverbatim in both directions, so the two new tests assert the invocation's output type and the threeORDER BYforms directly.Closes #1222