[Variant] Overly aggressive inference of UUID values

[scovich]

Describe the bug

When converting strongly-typed arrow arrays to binary variant, typed_value_to_variant has the following code, which unconditionally treats all 16-byte values as UUID:

        DataType::FixedSizeBinary(binary_len) => {
            let array = typed_value.as_fixed_size_binary();
            // Try to treat 16 byte FixedSizeBinary as UUID
            let value = array.value(index);
            if *binary_len == 16 {
                if let Ok(uuid) = Uuid::from_slice(value) {
                    return Variant::from(uuid);
                }
            }   
            let value = array.value(index);
            Variant::from(value)
        }

To Reproduce

Create a FixedSizeBinaryArray with length 16 and convert to variant.

Expected behavior

UUIDs should only be produced if the UUID extension type is present.

[scovich]

Update: The variant shredding spec apparently (implicitly?) requires that typed_value columns must be valid variant types.

For fixed-size binary, specifically, see case 137 of parquet-testing/shredded_variant/cases.json that expects the read to fail.

So in fact, the match arm should be updated to only check for the 16-byte len that signals a UUID:

DataType::FixedSizeBinary(16) => {

[scovich]

Hmm @alamb I think my "fix" in #8438 was incomplete -- the variant shredding spec requires a shredded UUID column to have parquet logical type UUID, which I believe would translate to the arrow canonical UUID extension? So the read path here would need to verify the annotation is present before accepting the column, and an eventual shredding write path will have to add the extension type to produce correct parquet output.

[alamb]

let's leave this open then and we can look into addressing it later

which I believe would translate to the arrow canonical UUID extension?

The arrow variant extension type docs don't seem to mandate (or prevent) using a canonical arrow extension type

https://github.com/apache/arrow/blob/00439d104804fca889016d054c03ff6ba9d5560f/docs/source/format/CanonicalExtensions.rst?plain=1#L277-L286

[scovich]

The arrow variant extension type docs don't seem to mandate (or prevent) using a canonical arrow extension type

That's arguably a bug/oversight.

The variant shredding spec mandates that

Shredded values must use the following Parquet types:

Variant Type	Parquet Physical Type	Parquet Logical Type
	...
uuid	FIXED_LEN_BYTE_ARRAY[len=16]	UUID

We have three shredding integration test cases that work with UUID values: 37, 81, 123. Of those, case 37 is the most relevant because it actually shreds as UUID (rather than storing a UUID value in an unshredded or wrong-shredded column):

{
  "case_number" : 37,
  "test" : "testShreddedVariantPrimitives",
  "parquet_file" : "case-037.parquet",
  "variant_file" : "case-037_row-0.variant.bin",
  "variant" : "Variant(metadata=VariantMetadata(dict={}), value=Variant(type=UUID, value=f24f9b64-81fa-49d1-b74e-8c09a6e31c56))"
}, {
  "case_number" : 81,
  "test" : "testUnshreddedVariants",
  "parquet_file" : "case-081.parquet",
  "variant_file" : "case-081_row-0.variant.bin",
  "variant" : "Variant(metadata=VariantMetadata(dict={}), value=Variant(type=UUID, value=f24f9b64-81fa-49d1-b74e-8c09a6e31c56))"
}, {
  "case_number" : 123,
  "test" : "testUnshreddedVariantsWithShreddedSchema",
  "parquet_file" : "case-123.parquet",
  "variant_file" : "case-123_row-0.variant.bin",
  "variant" : "Variant(metadata=VariantMetadata(dict={}), value=Variant(type=UUID, value=f24f9b64-81fa-49d1-b74e-8c09a6e31c56))"
}

The corresponding parquet files have the following schemas:

Case 37 (shredded as UUID):

message table {
  required int32 id = 1;
  optional group var (VARIANT(1)) = 2 {
    required binary metadata;
    optional binary value;
    optional fixed_len_byte_array(16) typed_value (UUID);
  }
}

Note that UUID value lives in the typed_value column, according to the row group stats:

Row group 0:  count: 1  139.00 B records  start: 4  total(compressed): 139 B total(uncompressed):139 B
--------------------------------------------------------------------------------
                 type      encodings count     avg size   nulls   min / max
id               INT32     _   _     1         27.00 B    0       "1" / "1"
var.metadata     BINARY    _   _     1         36.00 B    0       "0x010000" / "0x010000"
var.value        BINARY    _   _     1         30.00 B    1
var.typed_value  FIXED[16] _   _     1         46.00 B    0       "f24f9b64-81fa-49d1-b74e-8..." / "f24f9b64-81fa-49d1-b74e-8..."

case 81 (unshredded)

message table {
  required int32 id = 1;
  required group var (VARIANT(1)) = 2 {
    required binary metadata;
    required binary value;
  }
}

case 123 (shredded as string)

message table {
  required int32 id = 1;
  optional group var (VARIANT(1)) = 2 {
    required binary metadata;
    optional binary value;
    optional binary typed_value (STRING);
  }
}

Here, the UUID value is stored in value because it's not a string:

Row group 0:  count: 1  144.00 B records  start: 4  total(compressed): 144 B total(uncompressed):144 B
--------------------------------------------------------------------------------
                 type      encodings count     avg size   nulls   min / max
id               INT32     _   _     1         27.00 B    0       "1" / "1"
var.metadata     BINARY    _   _     1         36.00 B    0       "0x010000" / "0x010000"
var.value        BINARY    _   _     1         51.00 B    0       "0x50F24F9B6481FA49D1B74E8..." / "0x50F24F9B6481FA49D1B74E8..."
var.typed_value  BINARY    _   _     1         30.00 B    1

[scovich]

I double checked, and var.typed_value in case 37 does come back annotated with the arrow.uuid extension.
So it's "merely" a matter of plumbing it through and paying attention to it.

(annoying bit being, extension types are attached to the Field, not the Array and its DataType, so it's actually not trivial to plumb it through)

[alamb]

(annoying bit being, extension types are attached to the Field, not the Array and its DataType, so it's actually not trivial to plumb it through)

Yes, I agree this is very unfriendly from a user perspective.