GH-590: Allow VARIANT value to be omitted

[HuaHuaY]

Rationale for this change

Allow VARIANT value to be omitted.

There are some cases in parquet-testinghttps://github.com/apache/parquet-testing/tree/master/shredded_variant which don't have value field.

# Doesn't have top value field.
parquet-testing/shredded_variant on  master ❯ ~/arrow/cpp/out/build/ninja-debug/debug/parquet-dump-schema case-131.parquet required group field_id=-1 table {
required int32 field_id=1 id;
required group field_id=2 var (Variant(1)) {
required binary field_id=-1 metadata;
optional int32 field_id=-1 typed_value;
}
}
# Doesn't have inner value field.
parquet-testing/shredded_variant on  master ❯ ~/arrow/cpp/out/build/ninja-debug/debug/parquet-dump-schema case-132.parquet
required group field_id=-1 table {
required int32 field_id=1 id;
optional group field_id=2 var (Variant(1)) {
required binary field_id=-1 metadata;
optional binary field_id=-1 value;
optional group field_id=-1 typed_value {
required group field_id=-1 a {
optional int32 field_id=-1 typed_value;
}
required group field_id=-1 b {
optional binary field_id=-1 typed_value (String);
}
}
}
}

What changes are included in this PR?

Updated the descriptions in the variant-related Markdown files.

Do these changes have PoC implementations?

No.

[HuaHuaY]

@alamb@emkornfield@zeroshade Please take a look. I found that arrow-rs already allows to omit value array. https://github.com/apache/arrow-rs/blob/7616e10f12894232eacb807993e55427a067d061/parquet-variant-compute/src/variant_array.rs#L858

[wgtmac]

Could you help review this? @aihuaxu@steveloughran

[alamb]

Perhaps @rdblue or @gene-db (the authors of the original spec in #456#460) can help take a look

The example files were generated by spark so I assume they are now out in the wild and we shoudl adjust the spec accordingly

[rdblue]

I don't think this is correct.

The test cases are the ones that we use in Parquet Java and Iceberg to ensure that we can read variant data as long as it can be interpreted correctly. Being liberal about what we accept does not mean that it is a good idea for writers to produce values that way. We don't need to adjust the spec, this is just a reasonable choice for readers so they don't needlessly fail.

[wgtmac]

Thanks @rdblue for the explanation! Then I agree that this spec doesn't need any change. Perhaps we have two follow-ups to avoid future confusion:

• Fix wording in https://github.com/apache/arrow/blob/f10c93c000775c5fb30ca230f540358611091589/docs/source/format/CanonicalExtensions.rst?plain=1#L448 to align with the Parquet spec. cc @zeroshade@alamb
• Clarify the purpose of these test files in https://github.com/apache/parquet-testing/blob/master/shredded_variant/README.md

[zeroshade]

so, we're saying that it SHOULD always write both? and that writers shouldn't omit the optional value field when it is unnecessary?

[wgtmac]

@zeroshade I think so.

[rdblue]

@zeroshade and @wgtmac, the wording in the current spec is clear:

The group must contain a field named metadata and a field named value.

This spec has been adopted by the community and has deployed implementations, including proprietary implementations.

The change proposed here is a compatibility breaking change because it would allow a field that is currently required to be omitted. We should not make changes like this to the spec because it could break implementations that rely on this field. A breaking change like this would need to go into a new compatibility version.

[steveloughran]

if it's not optional, then arrow-rs must stop issuing shredded variant fields without it; a variant without the value should be added to the test file repository as a bad data example.

[rdblue]

If arrow-rs is not producing the value field, then it is out of spec. But that's also what this test case is for, to ensure that we can read things that are out of spec. I don't think we want to add this as a "bad" example, if what you mean is to test that readers fail. It is reasonable to read the data.

[alamb]

I filed a ticket in arrow-rs to track this report

• Variant: report that arrow-rs produces "bad" shredded variant arrow-rs#10306

I think we tried to make the malformed variant examples in the parquet-testing corpus clear (aka what is in https://github.com/apache/parquet-testing/tree/master/shredded_variant) by clearly labeling them as INVALID

e.g. https://github.com/apache/parquet-testing/blob/master/shredded_variant/case-043-INVALID.parquet

[alamb]

Specifcally, if case-132 is actually invalid, perhaps we should rename https://github.com/apache/parquet-testing/blob/master/shredded_variant/case-132_row-0.variant.bin to case-132-INVALID_row-0.variant.bin

[sdf-jkl]

To add some clarity following the arrow-rs report (apache/arrow-rs#10306), here is a survey of how implementations currently handle a variant group that omits the value column:

implementation	reading a value-less group	writing
DuckDB	rejects: "The Variant column must have 'metadata' and 'value' as the first two columns"	always emits value (writer)
Spark	reads (guards every value access)	always emits value (variantShreddingSchema includes it at every level)
parquet-java	reads (containsField("value") guards)	assumes value exists whenever a row needs the binary fallback (getFieldIndex throws otherwise)
Arrow C++	no shredding support yet; its unshredded storage validation requires non-nullable metadata and value	n/a
arrow-go	reads; type validation currently requires only "at least one of value or typed_value"	may omit
arrow-rs	currently reads (validation gap); fix in progress to reject (apache/arrow-rs#10306)	currently may omit value; fix in progress to always emit it (apache/arrow-rs#10306)
cuDF	reads the raw struct without reconstructing values; its tests assert the value child is present even for shredded columns (fixtures are DuckDB-written)	n/a

This supports the spec text as it stands: the implementations that validate variant group structure (DuckDB, Arrow C++) require value, arrow-rs is adopting the same, and every surveyed writer that produces spec-targeted files emits it. Where value-less groups are readable, it is by absence of validation rather than considered support.

On how the divergence propagated: the shredded_variant cases in parquet-testing (generated from the Iceberg test suite in apache/parquet-testing#91) include four cases that omit value (41, 131, 132, 138) labeled as valid cases with expected outputs rather than with the -INVALID convention. arrow-rs and arrow-go both run that corpus in CI, so the lenient interpretation looked conformance-verified to both. I have filed apache/parquet-testing#116 proposing those cases be relabeled per @alamb's suggestion above.

With the spec question settled, the remaining work is enforcement in implementations: arrow-rs's is in progress under apache/arrow-rs#10306, and arrow-go's type validation currently accepts the omission and would need a similar update.

[alamb]

@sdf-jkl made a proposal to mark the cases with missing value columns as INVALID in the parquet-testing repo:

• Mark shredded_variant cases 41, 131, 132, 138 as INVALID (missing 'value' column) parquet-testing#117

[HuaHuaY]

apache/parquet-testing#117 has been merged and I close this pull request.