manner opened a new pull request, #29323:
URL: https://github.com/apache/flink/pull/29323
## What is the purpose of the change
`BinaryVariantInternalBuilder.appendDecimal` writes `(byte) d.scale()`
without validating the decimal. There is no check for a negative scale, and
precision/scale above 38 is only guarded by an `assert`. The variant spec
requires a scale in [0, 38] and a precision of at most 38.
So `VariantBuilder.of(BigDecimal)`, e.g. from a UDF, can write variants that
fail later on read with `MALFORMED_VARIANT`. When the scale byte wraps around,
it even reads back a different value: `of(new
BigDecimal("1e2147483647")).toJson()` returns `0.1`.
This PR validates in `appendDecimal`, so all callers are covered. It follows
the same rules Spark uses in its CSV/XML variant parsers
([SPARK-54099,](https://issues.apache.org/jira/browse/SPARK-54099)
[SPARK-55932](https://issues.apache.org/jira/browse/SPARK-55932)).
## Brief change log
- A negative scale is rescaled to 0 with `setScale(0)`, which keeps the
value. Non-zero values with a scale below -38 are rejected before that, because
they can't fit anyway.
- Decimals whose precision or scale is still above 38 are rejected with a
`VariantTypeException`. The builder already uses this exception for
out-of-range timestamps.
- Removed the `assert` and documented the behavior on
`VariantBuilder.of(BigDecimal)`.
- PARSE_JSON is unchanged. `tryParseDecimal` already checks the range and
falls back to double.
## Verifying this change
This change added tests and can be verified as follows:
- `BinaryVariantTest`: negative scale, including a round trip through
`getDecimal()`, which strips trailing zeros and returns `1E+2` for a variant
holding 100.
- `BinaryVariantTest`: precision and scale of exactly 38.
- `BinaryVariantTest`: out-of-range values: precision 39, scale 39,
`1e38`, `-1e999999999` and `1e2147483647`.
- `BinaryVariantInternalBuilderTest`: PARSE_JSON still stores numbers
outside the decimal range as double.
- The new builder tests fail without the fix.
## 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)`: yes. `VariantBuilder` is `@PublicEvolving`. The signature
is unchanged, but `of(BigDecimal)` now throws a `VariantTypeException` for
decimals that don't fit, instead of returning a broken variant.
- The serializers: no.
- The runtime per-record code paths (performance sensitive): yes,
`appendDecimal` runs per record. The added checks are a few int comparisons,
and `setScale` only runs for negative scales.
- 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? JavaDocs
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: Claude Code (Opus 5.5)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
