Expression.VarCharLiteral carries value() and length() independently and reports varchar<length()> from getType(). Nothing checks that the value fits the length it declares: not the builder, not ExpressionCreator.varChar, and neither proto converter — ExpressionProtoConverter writes value and length verbatim, ProtoExpressionConverter reads them back the same way. So ExpressionCreator.varChar(false, "abcdef", 3) builds a literal whose value is twice the maximum its own accessor documents, and that literal round-trips through proto unchanged.
The spec leaves no room here: VARCHAR<L> is "A unicode string of at most L characters. L must be within [1..2,147,483,647]" (type classes). A value longer than L is malformed input, and the POJO is the only place that can say so — everything below it has to guess. Isthmus converts such a literal to CAST('abcdef'):VARCHAR(3), a cast whose target is narrower than the value it wraps, with the outcome left to whatever executes the plan; LiteralNullabilityRoundtripTest.tupleUnwrapLeavesATruncatingCastAlone pins that cast because it is what the conversion decides, not as a contract for the value.
A @Value.Check on VarCharLiteral is the fix, in the shape NullLiteral already uses in the same file. The rest of the model does not have this hole: FixedCharLiteral and FixedBinaryLiteral derive their type from the value itself (value().length(), value().size()), so they cannot contradict themselves. DecimalLiteral keeps precision and scale beside a ByteString value, which is the same shape, but a separate question from this one.
Two things worth settling before the fix, because both outlive it:
Reading a plan that carries such a literal turns into an error where today it converts silently, so this is breaking for anyone whose producer writes them.
What counts as a character. String.length() counts UTF-16 code units, so "😀".length() is 2 where counting characters would give 1. The spec says "characters" without saying which of the two it means for one outside the basic plane, and nothing in this repo counts code points — ExpressionCreator.fixedChar(false, "😀") already reports fixedchar<2>. Checking with length() keeps the two literals consistent with each other and both off by the same measure for astral characters; checking with codePointCount fixes VarCharLiteral alone and makes the pair disagree. I lean towards length() and raising the character-counting question on its own if it is worth fixing, but I would rather have your read before picking.
The lower bound is a smaller version of the same thing: the spec puts L at 1 or more, and varChar(0) builds today -- as a literal here, and as a type, since Type.VarChar carries no check either. Folding it into the same check looks right unless you see a reason to keep it open.
Follow-up from the review of #1153. Checked on main @ e07d093.
Expression.VarCharLiteralcarriesvalue()andlength()independently and reportsvarchar<length()>fromgetType(). Nothing checks that the value fits the length it declares: not the builder, notExpressionCreator.varChar, and neither proto converter —ExpressionProtoConverterwritesvalueandlengthverbatim,ProtoExpressionConverterreads them back the same way. SoExpressionCreator.varChar(false, "abcdef", 3)builds a literal whose value is twice the maximum its own accessor documents, and that literal round-trips through proto unchanged.The spec leaves no room here:
VARCHAR<L>is "A unicode string of at most L characters. L must be within [1..2,147,483,647]" (type classes). A value longer thanLis malformed input, and the POJO is the only place that can say so — everything below it has to guess. Isthmus converts such a literal toCAST('abcdef'):VARCHAR(3), a cast whose target is narrower than the value it wraps, with the outcome left to whatever executes the plan;LiteralNullabilityRoundtripTest.tupleUnwrapLeavesATruncatingCastAlonepins that cast because it is what the conversion decides, not as a contract for the value.A
@Value.CheckonVarCharLiteralis the fix, in the shapeNullLiteralalready uses in the same file. The rest of the model does not have this hole:FixedCharLiteralandFixedBinaryLiteralderive their type from the value itself (value().length(),value().size()), so they cannot contradict themselves.DecimalLiteralkeepsprecisionandscalebeside aByteStringvalue, which is the same shape, but a separate question from this one.Two things worth settling before the fix, because both outlive it:
Reading a plan that carries such a literal turns into an error where today it converts silently, so this is breaking for anyone whose producer writes them.
What counts as a character.
String.length()counts UTF-16 code units, so"😀".length()is 2 where counting characters would give 1. The spec says "characters" without saying which of the two it means for one outside the basic plane, and nothing in this repo counts code points —ExpressionCreator.fixedChar(false, "😀")already reportsfixedchar<2>. Checking withlength()keeps the two literals consistent with each other and both off by the same measure for astral characters; checking withcodePointCountfixesVarCharLiteralalone and makes the pair disagree. I lean towardslength()and raising the character-counting question on its own if it is worth fixing, but I would rather have your read before picking.The lower bound is a smaller version of the same thing: the spec puts
Lat 1 or more, andvarChar(0)builds today -- as a literal here, and as a type, sinceType.VarCharcarries no check either. Folding it into the same check looks right unless you see a reason to keep it open.Follow-up from the review of #1153. Checked on
main@ e07d093.