fix(isthmus): keep the declared length of a wide character or binary type - #1169
Open
alexandrefimov wants to merge 3 commits into
Open
fix(isthmus): keep the declared length of a wide character or binary type#1169alexandrefimov wants to merge 3 commits into
alexandrefimov wants to merge 3 commits into
Conversation
…type 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. fixedchar and fixed_binary lose their length the same way. Those three are the types whose length crosses the boundary, and Substrait declares each as a 32-bit integer, so they now report Integer.MAX_VALUE. VARBINARY is left alone: Substrait's binary carries no length for it to lose. Closes substrait-io#1167.
The cap reaches the conversion from SQL too, which the tests did not say: Calcite narrows a declared width to its maximum silently, so a cast wider than the default used to leave the conversion as a varchar<65536>.
The conversion takes whatever RelDataTypeFactory it is handed, and a factory caps a width at its type system's maximum without saying so, so raising the maximum fixes the narrowing only for the factory isthmus builds. A fixedchar, varchar or fixed_binary the factory narrows is now reported instead of returned.
alexandrefimov
force-pushed
the
issue-1167-varchar-precision
branch
from
August 28, 2026 08:27
895bc23 to
cdca654
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SubstraitTypeSystemoverridesgetMaxPrecisionfor the temporal types andDECIMAL;CHAR,VARCHARandBINARYfell through toRelDataTypeSystemImpl, whose default caps them at 65536. The type factory then narrowed anything wider without raising an error, so a plan declaringvarchar<2147483647>came back asVARCHAR(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 avarchar<65536>. It now comes out asvarchar<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,varcharandfixed_binary— and Substrait declares each length as a 32-bit integer, so they now reportInteger.MAX_VALUE.VARBINARYis left alone because Substrait'sbinarycarries no length for it to lose. The issue was filed for the character types;fixed_binaryturned out to have the same defect at the same boundary.One consequence worth deciding on rather than discovering later.
CHARandBINARYare 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)))throwsOutOfMemoryError: Required length exceeds implementation limit, where the old cap silently made itCHAR(65536)and it converted.CHAR(100000)is fine, and the varying types are unaffected at any width. That is a genuine consequence of declaring a two-billion-character fixed-width value rather than something introduced here, but it does trade a silent wrong answer for a crash in that one case, and it is the ceiling question of #1124 with a number attached. If you would rather this landed as a narrower cap, or as a conversion that fails instead of narrowing, say which and I will redo it.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.
A
RelDataTypeFactorynarrows 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.The widths this admits are the spec's, not ones Calcite can always materialize:
CHAR(2147483647)is a legalfixedcharand a legal Calcite type, but building a literal of it throwsOutOfMemoryErrorinsideRexBuilder.makeLiteral, which pads the value to the declared width. Before this, such a plan came back silently narrowed to 65536; now it either round-trips or fails where the width is materialized.