[
https://issues.apache.org/jira/browse/FLINK-40832?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ramin Gharib reassigned FLINK-40832:
------------------------------------
Assignee: Moritz Manner
> 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
> Assignee: Moritz Manner
> Priority: Major
>
> 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://parquet.apache.org/docs/file-format/types/variantencoding/]
> 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)
