Moritz Manner created FLINK-40832:
-------------------------------------
Summary: VariantBuilder.of(BigDecimal) writes malformed or wrong
decimals for negative scale or precision/scale > 38
Key: FLINK-40832
URL: https://issues.apache.org/jira/browse/FLINK-40832
Project: Flink
Issue Type: Bug
Components: API / Core
Reporter: Moritz Manner
h3. Problem
{{BinaryVariantInternalBuilder.appendDecimal(BigDecimal)}} writes {{(byte)
d.scale()}} without validating the decimal. There is no check for a negative
scale at all. Precision and scale above 38 are only guarded by a Java
{{assert}} in the DECIMAL16 branch, which only runs with {{-ea}} and is off by
default. The [Parquet variant
spec|https://github.com/apache/parquet-format/blob/master/VariantEncoding.md]
requires a scale in [0, 38] and a precision of at most 38 for decimal4/8/16.
This can be reached through the public {{{}VariantBuilder.of(BigDecimal){}}},
e.g. from a UDF that returns a variant:
{code:java}
Variant v = Variant.newBuilder().of(new BigDecimal("1e5")); // scale -5
v.getType(); // DECIMAL
v.getDecimal(); // VariantTypeException: MALFORMED_VARIANT
v.toJson(); // VariantTypeException: MALFORMED_VARIANT
// 41 digits: same as above without -ea, AssertionError with -ea
Variant.newBuilder().of(new BigDecimal("1" + "0".repeat(40)));
// the scale byte wraps, so this silently reads back as 0.1
Variant.newBuilder().of(new BigDecimal("1e2147483647")).toString();
{code}
The reader ({{{}BinaryVariantUtil.getDecimalWithOriginalScale{}}}) checks the
range, so a broken value is written without error and only fails later when
it's read. In the wrap-around case it doesn't fail at all and returns a wrong
value.
A negative scale is easy to get: {{Variant.getDecimal()}} strips trailing
zeros, so a variant holding 100 returns {{{}1E+2{}}}. Passing that back to
{{of(BigDecimal)}} gives a malformed variant.
PARSE_JSON is not affected. {{tryParseDecimal}} only accepts plain decimals
without an exponent and checks the range before calling {{{}appendDecimal{}}}.
Otherwise it falls back to double.
h3. How Spark handles it
Our builder is a port of Spark's {{{}VariantBuilder{}}}, and Spark's
{{appendDecimal}} has the same code: no negative scale check and the same
{{{}assert{}}}. Spark relies on the callers instead:
* {{parse_json}} uses the same {{tryParseDecimal}} check as we do.
* {{CAST(... AS VARIANT)}} relies on {{DecimalType}} (precision <= 38, scale
>= 0 unless the legacy negative scale flag is enabled).
* The CSV and XML parsers reject a scale below -38, rescale a negative scale
with {{{}setScale(0){}}}, and then require precision and scale <= 38
(SPARK-54099, SPARK-55932).
We can't rely on the callers in the same way because
{{VariantBuilder.of(BigDecimal)}} is public API and accepts any BigDecimal.
h3. Proposed fix
Validate in {{appendDecimal}} with the same rules as Spark's CSV/XML parsers:
* Rescale a negative scale to 0 with {{{}setScale(0){}}}. This keeps the
value. Non-zero values with a scale below -38 are rejected first. They can't
fit anyway, and {{setScale}} is expensive for huge exponents like
{{{}1e999999999{}}}.
* Throw a {{VariantTypeException}} if the precision or scale is still above
38.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
