raminqaf opened a new pull request, #29370:
URL: https://github.com/apache/flink/pull/29370
## What is the purpose of the change
`CAST(s AS VARIANT)` decoded the UTF-8 bytes of `s` into a Java `String` and
then encoded the `String` back to the same bytes. `CAST(d AS VARIANT)` built a
`BigDecimal` from a compact decimal only for the builder to take it apart
again. This PR copies the string bytes and writes the unscaled long directly.
The stored `VARIANT` does not change:
| Input | Stored before and after
|
|-----------------------------|----------------------------------------------|
| `47 72 C3 BC C3 9F 65` | `47 72 C3 BC C3 9F 65`, the string `Grüße`
|
| `61 FF 62` | `61 EF BF BD 62`, so `FF` becomes U+FFFD
|
| `1.50` as `DECIMAL(10, 2)` | decimal4 with unscaled value 150 and scale 2
|
A `BinaryStringData` can hold invalid UTF-8, but the Variant spec requires
valid strings. The decode used to hide this by replacing every malformed
sequence with U+FFFD. The new path checks the bytes in one pass with
`StringUtf8Utils#firstInvalidUtf8ByteIndex`, which applies the same checks as
the strict decoder. Invalid bytes still go through the decode, so they are
stored exactly as before. Rejecting invalid UTF-8 instead would make every
string cast to `VARIANT` fallible, so I left it for a separate decision.
JMH, JDK 17, 3 to 5 forks. Each call reads a fresh `BinaryStringData` or
`DecimalData` from a `MemorySegment` at an offset, like `BinaryRowData` does.
| Cast to `VARIANT` | Size | Before | After | Change |
|--------------------------|--------|---------|---------|--------|
| `STRING`, ASCII | 16 B | 31.6 ns | 23.0 ns | -27% |
| `STRING`, ASCII | 1 KiB | 325 ns | 227 ns | -30% |
| `STRING`, ASCII | 64 KiB | 21.0 µs | 11.1 µs | -47% |
| `STRING`, non-ASCII | 16 B | 37.3 ns | 25.5 ns | -32% |
| `STRING`, non-ASCII | 1 KiB | 1.33 µs | 0.87 µs | -35% |
| `STRING`, non-ASCII | 64 KiB | 84.8 µs | 47.8 µs | -44% |
| `STRING`, invalid UTF-8 | 1 KiB | 1.89 µs | 1.84 µs | -2% |
| `DECIMAL(9, 2)` | | 25.7 ns | 15.4 ns | -40% |
| `DECIMAL(18, 2)` | | 27.4 ns | 15.5 ns | -44% |
| `DECIMAL(38, 2)` | | 39.6 ns | 36.8 ns | -7% |
A 64 KiB string allocates 263 KB per call instead of 459 KB for ASCII and
623 KB for non-ASCII. The invalid UTF-8 row puts the invalid byte last, so the
check reads the whole string before it falls back. `DECIMAL(38, 2)` is not
compact and takes the unchanged `BigDecimal` path, so its difference is noise.
## Brief change log
- `BinaryVariantInternalBuilder#appendString(byte[])` copies UTF-8 bytes
as they are. `appendString(String)` encodes and delegates to it.
- `BinaryVariantInternalBuilder#appendDecimal(long, int)` writes a
decimal4 or a decimal8 from the unscaled value and picks the width like
`appendDecimal(BigDecimal)`. Anything wider, or with a negative scale, goes
through `appendDecimal(BigDecimal)`.
- `VariantCastUtils#fromString` and `#fromDecimal` use the two new methods.
## Verifying this change
This change added tests and can be verified as follows:
- `BinaryVariantInternalBuilderTest`: `appendString(byte[])` stores the
bytes as they are under a short and a long string header. `appendDecimal(long,
int)` produces the same bytes as `appendDecimal(BigDecimal)` and the expected
width at 10^9, 10^18, scales 9, 10, 18 and 19, `Long.MIN_VALUE`, and a negative
scale.
- `VariantCastUtilsTest`: a string read from a segment offset stores its
UTF-8 bytes. Four invalid inputs store the same U+FFFD value as before: a stray
`FF`, a truncated `C3`, the overlong `C0 AF`, and the surrogate `ED A0 80`.
Compact and non-compact decimals at precisions 9, 10, 18, 19 and 38 match the
`BigDecimal` result.
- These tests fail if the UTF-8 check is skipped or if the decimal4 bound
is off by one.
- Existing `CastRulesTest` and `CastFunctionITCase` pass.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): yes. Casting
a `STRING` or a `DECIMAL` to `VARIANT` runs per record. The benchmark above
covers both.
- Anything that affects deployment or recovery: JobManager (and its
components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: Claude Opus 5.5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
