spark-test commented on code in PR #59669:
URL: https://github.com/apache/spark/pull/59669#discussion_r4492390781
##########
common/variant/src/main/java/org/apache/spark/types/variant/VariantBuilder.java:
##########
@@ -205,6 +205,229 @@ public static Variant pickAtPaths(Variant v, PickNode
tree) {
return builder.result();
}
+ /**
+ * Return {@code v} in canonical form, or {@code v} itself if it is already
canonical.
+ * <p>
+ * The variant binary encoding allows the same logical value to be encoded
in several ways
+ * (dictionary order, unused keys, integer and offset widths, decimal scale,
...). Canonical form
+ * picks one encoding per value, so that values which are equal under the
variant equality rules
+ * have identical {@code value} and {@code metadata} bytes and can be
compared, hashed and
+ * grouped bytewise. It is a Spark-internal normalization, not part of the
variant spec, and
+ * nothing outside Spark is expected to produce or require it.
+ * <p>
+ * A variant is canonical when all of the following hold.
+ * <ul>
+ * <li>Metadata header: version {@link VariantUtil#VERSION}, the minimal
offset size for
+ * {@code max(lastOffset, numKeys)}, i.e. 1, 2, 3 or 4 bytes as given
by
+ * {@code getIntegerSize}, and {@code sorted_strings} and the unused
bit 5 clear.
+ * {@code sorted_strings} stays clear although canonical keys are
sorted, so that builder
+ * output, which never sets it, is already canonical.</li>
+ * <li>Metadata dictionary: the first offset is 0, offsets are
non-decreasing, the key bytes
+ * end exactly at the last offset (no trailing bytes), keys are
strictly increasing under
+ * unsigned byte comparison (so they are unique), and every key is
referenced by at least
+ * one object field. Key bytes are preserved as is, including invalid
UTF-8.</li>
+ * <li>Value root: starts at position 0 and spans exactly {@code
value.length} bytes.</li>
+ * <li>Object: field ids strictly increasing (fields are stored in key
order and no key is
+ * repeated), field data stored contiguously in field order, the end
offset equal to the
+ * data size, and the header using the minimal id size for the largest
id, the minimal
+ * offset size for the data size, {@code is_large} only for more than
255 fields, and the
+ * reserved bit 7 clear.</li>
+ * <li>Array: element order preserved, element data stored contiguously in
element order, the
+ * end offset equal to the data size, and the header using the minimal
offset size for the
+ * data size, {@code is_large} only for more than 255 elements, and
the reserved bits 5 to
+ * 7 clear.</li>
+ * <li>Integer: the smallest of INT1, INT2, INT4 and INT8 that holds the
value.</li>
+ * <li>Decimal: trailing zeros stripped and a negative scale raised to 0.
An integral decimal
+ * that fits in a signed 64-bit integer is stored as an integer
instead. Otherwise the
+ * smallest of DECIMAL4 (precision and scale at most 9), DECIMAL8 (at
most 18) and
+ * DECIMAL16 that holds the value.</li>
+ * <li>Float and double: -0.0 is stored as 0.0 and NaN as the canonical
NaN bit pattern of
+ * {@link Float#floatToIntBits} and {@link Double#doubleToLongBits}.
The type is kept, so
+ * a float and a double are never identical.</li>
+ * <li>String: at most {@link VariantUtil#MAX_SHORT_STR_SIZE} bytes uses
the short-string
+ * encoding, anything longer the long-string encoding. Bytes are
preserved as is, including
+ * invalid UTF-8.</li>
+ * <li>Every other type (null, boolean, date, timestamps, binary, uuid,
...) has a single
+ * encoding in the spec and is copied unchanged.</li>
+ * </ul>
+ * {@code isCanonical} checks exactly these rules and {@code doCanonicalize}
produces them; the
+ * two must be changed together when a rule changes or a type with
alternative encodings is
+ * added to the spec.
+ */
+ public static Variant canonicalize(Variant v) {
+ if (isCanonical(v)) {
+ return v;
+ }
+ return doCanonicalize(v);
+ }
+
+ // Check whether `v` already satisfies every rule listed in `canonicalize`.
Return false at the
+ // first non-canonical encoding. Malformed bytes that the check can reject
without decoding them
+ // return false, and the rebuild then decides whether they fail. Malformed
bytes that the check
+ // has to decode throw instead, for example `MALFORMED_VARIANT` for an
unreadable metadata table,
+ // a container whose data runs past the value, or a decimal whose precision
or scale is out of
Review Comment:
**Nit (P3):** The `isCanonical` comment lists "a container whose data runs
past the value" among the inputs that make the check throw `MALFORMED_VARIANT`,
but the check does not throw for that case. `valueSize` takes a container's
size from its header and final offset without reading the data, so for an array
or object whose declared data runs past the value, `isCanonical` returns false
(line 310 for the root, line 327 when nested). It throws for a container only
when the size field or offset list itself can't be read. The rebuild never
reads the final offset, so `canonicalize` then succeeds whenever the referenced
children are inside the buffer. For example, `[3, 1, 0, 100, 12, 1]`, an array
whose final offset declares 100 data bytes in a 6-byte value, canonicalizes to
`[3, 1, 0, 2, 12, 1]`, although `is_valid_variant` rejects it. The suite's
malformed `[3, 1, 5, 6, 0]` case does fail with `MALFORMED_VARIANT`, but in the
rebuild, because its element offset points past the value.
The next sentence, "`canonicalize` fails with the same error either way",
also holds only for inputs with a single defect. With `dictionary("a")` and the
object `[2, 2, 0, 5, 0, 1, 2, 68, 0]` (field 0 holds unknown type 17, field 1
has key ID 5), `canonicalize` throws `UNKNOWN_PRIMITIVE_TYPE_IN_VARIANT` from
the check, while `doCanonicalize` on the same bytes throws `MALFORMED_VARIANT`
from key collection.
The lenient rebuild matches how the other rebuild paths read only what the
offsets reach, so it is the comment that needs to change. Replace that example
with a container whose size field or offset list is truncated, and say that a
container whose declared data runs past the value makes the check return false
and leaves the outcome to the rebuild. Replace "the same error either way" with
"when the check throws, `canonicalize` fails with that error", and pin the
over-long container case in a test.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59669#discussion_r4481813549).
**Verification:**
- **Inspection:** Each malformed-input example in the isCanonical comment
matches the branch that handles it: examples described as throwing reach a read
or decode that raises the named error, and examples described as returning
false are rejected from bytes the check has already read.
- **Regression:** For arrays and objects, at the root and nested in another
container, whose declared data size runs past the end of the value while their
referenced children lie inside the buffer, isCanonical returns false without
throwing and canonicalize returns the canonical rebuilt value that the
corrected comment describes. The tests fail if the check starts throwing or
canonicalize starts rejecting or reshaping these inputs.
- **Behavior:** The existing malformed-input cases still fail with
MALFORMED_VARIANT or UNKNOWN_PRIMITIVE_TYPE_IN_VARIANT exactly as before, and
canonical and non-canonical valid inputs produce unchanged bytes.
--
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]
