fix(isthmus)!: keep the declared length of a wide character or binary type

[alexandrefimov]

SubstraitTypeSystem overrides getMaxPrecision for the temporal types and DECIMAL; CHAR, VARCHAR and BINARY fell through to RelDataTypeSystemImpl, whose default caps them at 65536. The type factory then narrowed anything wider without raising an error, so a plan declaring varchar<2147483647> came back as VARCHAR(65536) and a consumer building a Calcite tree from it had no way to notice. The boundary is exact: 65536 round-trips, 65537 does not.

The cap reaches the conversion from SQL too, which the issue does not mention: Calcite narrows a declared width there the same silent way, so SELECT CAST(a AS VARCHAR(100000)) came out of the conversion as a varchar<65536>. It now comes out as varchar<100000>. What SQL is accepted does not change -- Calcite never rejected the wider declaration, it just did not keep it.

Those three are the types whose length crosses the boundary — holding fixedchar, varchar and fixedbinary — and Substrait declares each length as a 32-bit integer, so they now report Integer.MAX_VALUE. VARBINARY is raised with them for a different reason: Substrait's binary carries no length of its own, but Calcite unifies a union of fixed-width binaries of different widths into a VARBINARY of the widest, so leaving it at the default reimposed the cap one step later — leastRestrictive(BINARY(100000), BINARY(5)) returned VARBINARY(65536), narrower than one of its own inputs. That shape was unreachable before this change, since no BINARY wider than 65536 could exist. Raising it opens no new failure at the extreme: a VARBINARY is not padded, so cast(x'01' AS VARBINARY(2147483647)) projects fine, where the fixed-width BINARY(2147483647) runs out of memory exactly as CHAR does. The issue was filed for the character types; fixedbinary turned out to have the same defect at the same boundary.

One consequence, and where the ceiling belongs. CHAR and BINARY are fixed-width, so Calcite pads a literal cast to them out to the declared length, and the padding is now allowed to reach the declared width. At the extreme, RelBuilder.project(cast('a' AS CHAR(2147483647))) throws OutOfMemoryError: Required length exceeds implementation limit, where the old cap silently made it CHAR(65536) and it converted; CHAR(100000) is fine, and the varying types are unaffected at any width. No literal need appear in the plan for this either: RexBuilder.zeroValue allocates new byte[precision] for a BINARY, so makeZeroLiteral(BINARY(2147483647)) throws the same way and a planner rule reaches it from a column type alone, where the old cap made it a 65536-byte array. A VARBINARY's zero value is ByteString.EMPTY, so raising that type costs nothing here. Integer.MAX_VALUE stays here because it is the spec's own bound on a length -- a narrower one is a dialect's answer, which SupportedType.maxLength() already carries for FIXED_CHAR, VARCHAR and FIXED_BINARY, and reading it is #1124. A cap on this type system would not bound all of it in any case: UserTypeMapper.toSubstrait returns a Substrait type without going through the type factory at all. The padding isthmus does itself, on the LogicalValues path where Calcite does none, is bounded in #1171.

One more thing the raise changes, and it reaches plans carrying no wide type at all: getMaxPrecision(VARCHAR) is Calcite's overflow threshold for concatenation as well, in ReturnTypes.DYADIC_STRING_SUM_PRECISION. A sum of widths past it falls back to an unparameterised type, so a || b over two varchar<40000> columns converts to varchar<80000> where it converted to string; CONCAT does the same. Neither operand is anywhere near the old cap.

A RelDataTypeFactory narrows a width past its own maximum without saying so, and this conversion takes whatever factory it is handed, so raising the maximum fixes the narrowing only for the factory isthmus builds. A declared length the factory cannot hold is now reported instead of returned quietly, and a negative one is refused before the factory is asked at all. Asking cannot tell the two apart: with assertions off — the normal production JVM — the factory stores the negative and reports it back, so varChar(-5) certified itself as VARCHAR(-5), while under -ea Calcite raised a bare AssertionError in place of the message. A length of -1 is Calcite's unspecified precision besides, so varChar(-1) passed and became a Substrait string where fixedChar(-1) was reported against a bound its length is below. A zero length is left to the factory, for the reason #1171 leaves it alone: ordinary SQL produces a CHAR(0) today, and substrait-io/substrait#1199 has the floor open. A decimal went the same silent way and now reports too: decimal<38,10> through a factory on Calcite's default type system came back DECIMAL(19,10) and read back as a precision the plan never declared. Its two ends go with it, both found checking the guard's boundaries rather than in review: decimal<-1,0> converted to a bare DECIMAL and read back as the maximum through any factory, -1 being Calcite's unspecified precision here as well, and a scale above the precision was built as asked -- decimal<19,25> came out as DECIMAL(19,25). That one is bounded by the spec rather than by the factory: scale sits in 0 <= S <= P, and no maximum catches it, since both type systems here set maxScale equal to maxPrecision, so a scale within the precision is within the maximum too. Calcite reports a zero precision and a negative scale itself, so those are left to it. The precision's own ceiling is the spec's too, not the factory's: a type system allowing more than 38 -- a getMaxPrecision(DECIMAL) of 76 -- let decimal<45,2> convert to DECIMAL(45,2), which toSubstrait then refused with "unsupported decimal precision 45", so the type converted in with no way back. Both ends of that clause read one constant now.

The conversion test asserts the resulting precision directly rather than through testType, whose expected type is built with the same type factory and would be narrowed alongside the value under test.

Closes #1167.

BREAKING CHANGE: TypeConverter.toCalcite throws IllegalArgumentException where it previously returned a narrowed type — for a declared length the given type factory cannot hold, and for a negative length, which a factory running without assertions accepted as a type of that width. It also throws for a decimal precision the given factory cannot hold and for a negative one, where it returned a narrowed type, for a scale above the precision, which it built as asked, and for a precision above the 38 the spec allows, which only a type factory more permissive than the spec could reach. Converted output types change with the raised bound: SELECT CAST(a AS VARCHAR(100000)) converts to varchar<100000> where it converted to varchar<65536>, a || b over two varchar<40000> columns converts to varchar<80000> where it converted to string, and the least restrictive type of BINARY(100000) and BINARY(5) is VARBINARY(100000) where it was VARBINARY(65536).

[nielspardon]

The ceiling question you flag belongs to #1124 — SupportedType.maxLength() already carries a dialect's answer for FIXED_CHAR, VARCHAR and FIXED_BINARY, the exact three types this touches, and nothing in isthmus reads it. Deriving Integer.MAX_VALUE from the proto's int32 length makes the limit a property of the spec rather than of the engine being targeted, which is the same hardcoding as the 65536 it replaces, just at the permissive end. Whether that means holding this or landing it and letting #1124 narrow it later is a maintainer call.

[alexandrefimov]

Measured on this branch, since the ceiling reaching values turns out to have sharper consequences than a wrong type.

SELECT CAST(x'01' AS BINARY(2147483647)) and SELECT CAST('a' AS CHAR(2147483647)) both end in OutOfMemoryError, and in Calcite rather than here: RexSimplify.simplifyCast reduces the cast through RexExecutable, and SqlFunctions.truncateOrPad materialises the padded value — Spaces.padRight for the char, Arrays.copyOf for the binary. Under Calcite's default type system the width is capped to 65536 before any of that, so the same literal is built without trouble; the cap is what bounds the padding today.

The other cases are unaffected: CAST('a' AS VARCHAR(2147483647)) converts fine since varchar is not padded, and so do column casts to BINARY(2147483647) or CHAR(2147483647), which have no constant to reduce. So it is the two padded types, and only for literals.

That reads to me as an argument for taking the limit from SupportedType.maxLength() in one go rather than landing Integer.MAX_VALUE and narrowing in #1124 later. Happy either way — say which and I will hold this or bound it here.

[nielspardon]

The padding on that path is Calcite's, agreed.

One thing to weigh before you pick, though it doesn't change the direction: a cap on SubstraitTypeSystem won't bound all of it. UserTypeMapper.toSubstrait returns a Substrait Type directly, so a mapper answering TypeCreator.REQUIRED.fixedChar(Integer.MAX_VALUE) never goes through the type factory and no getMaxPrecision ceiling sees it. On the LogicalValues path Calcite never pads either — it builds a CHAR literal at its text length, and #1171 is what widens it to the row field — so that pad wants its own bound whichever way this lands.

[alexandrefimov]

Keeping Integer.MAX_VALUE, then, and the description says why rather than asking.

The reasoning I landed on: the number here is the spec's bound on a length, and a narrower one is a dialect's, which is SupportedType.maxLength() and #1124 — putting a second hardcoded ceiling in SubstraitTypeSystem now would only be something to take out there. Your point that a cap here does not bound the mapper path settles the other half: UserTypeMapper.toSubstrait hands back a Substrait type without the type factory seeing it, so whatever number this class carries, a mapper can ask for a fixedchar<2147483647> anyway. The pad that isthmus does itself, on the LogicalValues path, is refused in #1171 at the largest array a JVM allocates.

Rebased on main while I was here.

[nielspardon]

Mark this breaking and add the footer — public toCalcite now throws where it previously returned a narrowed type, and SQL→Substrait output types change, which is the shape #1120 and #1121 both carried as fix(isthmus)!:. Keeping Integer.MAX_VALUE with the reasoning now in the description reads right to me. The three comments below are asymmetries in the raise I would fix before it lands.

[nielspardon]

Five smaller things below, plus two I will leave unless you want them: assertTrue(e.getMessage().contains("65536")) passes on the VARCHAR(65536) already in that message rather than on the bound it means to pin, and both public toCalcite overloads still document only @throws UnsupportedOperationException now that two conditions raise IllegalArgumentException.

[alexandrefimov]

Took both of the two you left rather than leaving them: the length test asserts allows up to 65536, which the narrowed VARCHAR(65536) in the same message no longer satisfies, and both public toCalcite overloads document the IllegalArgumentException alongside the existing UnsupportedOperationException.

[nielspardon]

looks like this needs to be reconciled with latest main

[alexandrefimov]

Rebased onto main at 85c03c7. One conflict, in CalciteTypeTest: #1171 added aJavaCharColumnTakesCalcitesDefaultWidth where this branch adds wideLengthCarryingTypesKeepTheirLength — both kept, each with its own javadoc.

./gradlew build and ./gradlew integrationTest are both green on the rebased branch, which should also clear the red Integration tests job: it failed on 429 Too Many Requests from Maven Central resolving spotless-lib while configuring :build-logic, not on anything here.

[nielspardon]

Everything from round 2 is in, and the description now carries the zero-literal and concatenation paths. Two small things left: a stray Javadoc block in the new test, and a scale check no valid decimal can reach.

[nielspardon]

Delete this block — it documents aWideVarcharDeclaredInSqlKeepsItsLength at the bottom of the file, not this test. Of two consecutive doc comments only the second applies, so the first is dead text.

Suggested change

	/**
	* The cap reaches the conversion from SQL as well. Calcite narrows a declared width to its
	* maximum silently rather than reporting that it cannot hold it, so before this a cast wider than
	* the default came out of the conversion as a {@code varchar<65536>}.
	*/

[alexandrefimov]

Moved rather than deleted -- it is aWideVarcharDeclaredInSqlKeepsItsLength's own javadoc, added with that test in the SQL-cast commit, and the decimal test landed between the two. The test at the bottom of the file had none left, so the block goes back to it.

[nielspardon]

Both fixes read right, and moving the stray Javadoc onto its own test beats deleting it. One thing left — the other half of the spec clause the scale bound came from.

[nielspardon]

LGTM