Ramin Gharib created FLINK-40319:
------------------------------------
Summary: Add a raw-bytes accessor for VARIANT string values, and
use it to validate UTF-8 on the character-string cast and to let a string
variant reach a binary target
Key: FLINK-40319
URL: https://issues.apache.org/jira/browse/FLINK-40319
Project: Flink
Issue Type: Improvement
Components: API / Core
Reporter: Ramin Gharib
Assignee: Ramin Gharib
h3. Background
{\{BinaryVariantUtil}} can return the raw payload of a binary value but not of
a string value. \{{getBinary}} copies the bytes out, while \{{getString}}
decodes them in the same step:
{code:java}
// getBinary
return Arrays.copyOfRange(value, start, start + length);
// getString
return new String(value, start, length);
{code}
Two things follow from there being no undecoded accessor.
*A character-string cast cannot reject invalid UTF-8 for a string value.*
FLINK-37925 added that check for a stored \{{BYTES}} value, where
\{{Variant#getBytes}} hands over the raw bytes. A stored \{{STRING}} value has
no equivalent, so the cast keeps the lenient decode and substitutes \{{U+FFFD}}
for malformed input. The value reaching the query is then a character the
stored bytes never held.
*A string variant cannot reach a binary target.* \{{VariantCastUtils#toBytes}}
calls \{{Variant#getBytes}}, which rejects every kind other than \{{BYTES}}, so
\{{CAST(v AS BYTES)}} fails for a string variant regardless of its content.
That is inconsistent in two ways. Regular SQL allows \{{STRING}} to
\{{BINARY}}, and the VARIANT rules already allow the mirror direction of
\{{BYTES}} to a character string, justified on the grounds that a cast renders
the way a regular cast of the stored kind would. It is also the infallible
direction, since encoding a string to UTF-8 always succeeds while decoding can
fail.
Together these leave a stored \{{STRING}} value with malformed bytes
unreachable. The character-string cast silently returns replacement characters
and the binary cast fails on a kind mismatch, so nothing shows what was
actually stored.
h3. Scope
# Add \{{BinaryVariantUtil#getStringBytes(byte[], int)}}, parsing the
\{{SHORT_STR}} and \{{LONG_STR}} header exactly as \{{getString}} does and
returning \{{Arrays.copyOfRange}} as \{{getBinary}} does. Keep the header
parsing duplicated instead of refactoring \{{getString}} to delegate. Spark
already duplicates it between \{{getBinary}} and \{{getString}}, and delegating
would add an array copy to every \{{getString}} call.
# Add \{{BinaryVariant#getStringBytes()}} guarded by \{{checkType(Type.STRING,
getType())}}, mirroring \{{getBytes()}}. Do not add it to \{{Variant}}, which
is \{{@PublicEvolving}}.
# In \{{VariantCastUtils#toStringValue}}, collapse the \{{STRING}} and
\{{BYTES}} branches. Both become read the raw bytes, validate, decode. One
message then covers both, and the advice to cast to \{{BYTES}} becomes true for
both.
# In \{{VariantCastUtils#toBytes}}, accept a stored \{{STRING}} value through
its UTF-8 bytes, and reject every other kind with \{{unsupportedKind}} instead
of letting \{{VariantTypeException}} escape and be rewrapped.
h3. Observable changes
|| Expression on a string variant || Before || After ||
| \{{CAST(v AS STRING)}}, malformed bytes | returns \{{U+FFFD}} | fails,
reporting the first invalid byte |
| \{{CAST(v AS BYTES)}} | fails on a kind mismatch | returns the stored bytes |
The second row flips an existing expectation in \{{CastRulesTest}}, which
asserts that a string variant fails a \{{BYTES}} cast. It becomes a passing
case, so it should be called out in the commit message rather than looking like
a test fixup. The prose table of cast targets in
\{{docs/content/docs/sql/reference/data-types.md}} needs a \{{STRING}} to
\{{BINARY}} and \{{VARBINARY}} entry. The support matrix in the same file
already documents VARIANT to \{{BYTES}} as fallible and needs no change.
h3. Notes
{\{Variant}} stays untouched, so \{{BinaryVariant}} has to be reached through
an \{{instanceof}} check in \{{VariantCastUtils}}. That is safe in practice.
\{{BinaryVariant}} is \{{final}} and the only implementation, and
\{{VariantType}} permits only \{{Variant}} and \{{BinaryVariant}} as conversion
classes.
Do not generalise the accessor to every stored kind. \{{BOOLEAN}} and \{{NULL}}
carry no payload bytes at all, because the value lives in the header type-info
bits. A numeric payload is a little-endian detail of the variant encoding
rather than a portable value. Regular SQL also rejects \{{INTEGER}} to
\{{BINARY}}, so accepting every kind would make a VARIANT cast more permissive
than a regular one. For inspecting the encoding of an arbitrary kind,
\{{BinaryVariant#getValue}} and \{{BinaryVariant#getMetadata}} already exist,
and a SQL-level equivalent belongs in a dedicated function rather than in
\{{CAST}}.
The character-string cast pays one extra array copy per row, since
\{{getStringBytes}} copies before the validation walk. Validation at ingest
would make the check redundant and allow removing it.
h3. Tests
{\{VariantBuilder}} takes a \{{String}}, so it cannot produce a string value
with malformed bytes. Build one directly with \{{new BinaryVariant(value,
metadata)}}, using \{{BinaryVariantUtil#shortStrHeader}} for the header byte
and a single \{{BinaryVariantUtil#VERSION}} byte as metadata. Cover a non-ASCII
string round-tripping through both targets, and a malformed string value
failing the character-string cast while still reaching \{{BYTES}} byte for byte.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
