SavicStefan commented on code in PR #59927: URL: https://github.com/apache/spark/pull/59927#discussion_r4445176909
########## common/variant/src/main/java/org/apache/spark/types/variant/VariantUtil.java: ########## @@ -605,10 +605,12 @@ public static <T> T handleArray(byte[] value, int pos, ArrayHandler<T> handler) // Validate whether a variant is well-formed. Returns true if the variant binary is structurally // well-formed (all bounds and type-info checks pass), false if it is malformed. // - // This is close to, but not strictly equivalent to, "`toJson` does not throw": this function - // does not enforce the `SIZE_LIMIT` check that the `Variant` constructor applies (which throws - // `VARIANT_CONSTRUCTOR_SIZE_LIMIT`). The implementation otherwise has the same structure as - // `toJson` (see `Variant.toJsonImpl`). + // This is close to, but not strictly equivalent to, "`toJson` does not throw". It is more + // lenient in that it does not enforce the `SIZE_LIMIT` check that the `Variant` constructor + // applies (which throws `VARIANT_CONSTRUCTOR_SIZE_LIMIT`). It is stricter in that it requires Review Comment: `isValidVariant` works on the raw byte arrays and never calls `new Variant(...)`, which is the only place `SIZE_LIMIT` is checked so `VARIANT_CONSTRUCTOR_SIZE_LIMIT` can't be thrown from here. As far as I can tell there is no deliberate reason for the leniency. The original PR (#55599) described the function as "true" iff toJson doesn't throw"; the review noted that the Variant constructor also enforces `SIZE_LIMIT`, and the comment was updated to document the difference rather than close it. In practice it only affects external data: Spark can't produce an oversized variant because `VariantBuilder` enforces the limit, but e.g. Parquet written by another tool is wrapped into a `VariantVal` without a size check. For such a row `is_valid_variant` returns true while every other variant function throws `VARIANT_CONSTRUCTOR_SIZE_LIMIT`, which goes against the purpose of the function. I agree returning false there would be more consistent, but its behaviour agrees with other functions. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
