Skip to content

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

Merged
alamb merged 1 commit into
apache:masterfrom
sdf-jkl:relabel-missing-value-cases
Jul 22, 2026
Merged

Mark shredded_variant cases 41, 131, 132, 138 as INVALID (missing 'value' column)#117
alamb merged 1 commit into
apache:masterfrom
sdf-jkl:relabel-missing-value-cases

Conversation

@sdf-jkl

@sdf-jklsdf-jkl commented Jul 10, 2026•
edited by alamb
Loading

Copy link
Copy Markdown
Member

Rationale

These four cases omit the required value column from a variant group but are labeled as valid cases with expected outputs. Per the discussion in apache/parquet-format#591, the spec's requirement that the group contain a value field is intentional and will not be relaxed, so these files are not spec-compliant. The mislabeling has already propagated into implementations that run this corpus in CI (see apache/arrow-rs#10306).

casetestomission
41testArrayMissingValueColumntop-level value (typed_value is a LIST)
131testMissingValueColumntop-level value (typed_value is INT32)
132testShreddedObjectMissingFieldValueColumnvalue inside the shredded object field groups
138testShreddedObjectMissingValueColumntop-level value (typed_value is an object)

Changes

Follows the existing convention used by cases 43, 84 and 125:

  • rename case-NNN.parquet / case-NNN_row-0.variant.bin to case-NNN-INVALID.parquet / case-NNN-INVALID_row-0.variant.bin
  • add a notes entry in cases.json stating the file is not valid according to the spec (with the reason) and that implementations can choose to error, or read the shredded value
  • keep the expected variant outputs for implementations that choose to read them

No file contents were changed, only renames and cases.json metadata.

🤖 Generated with Claude Code

@sdf-jkl@claude
Mark shredded_variant cases 41, 131, 132, 138 as INVALID (missing 'va…
6fe23ed
…lue' column)
These cases omit the required 'value' column from a variant group
(cases 41, 131, 138 at the top level; case 132 in the shredded object
field groups) but were labeled as valid cases. Per the discussion in
apache/parquet-format#591, the spec requires the 'value' field to
always be present, so these files are not spec-compliant.
Relabel them following the existing convention used by cases 43, 84
and 125: '-INVALID' filenames plus a 'notes' entry stating that
implementations can choose to error or read the shredded value. The
expected variant outputs are kept for implementations that choose to
read them.
Fixesapache#116
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sdf-jkl

sdf-jkl commented Jul 10, 2026

Copy link
Copy Markdown
MemberAuthor

@alamb

@wgtmac

wgtmac commented Jul 11, 2026

Copy link
Copy Markdown
Member

Does this break existing CI of dependent projects if merged?

@sdf-jklsdf-jkl mentioned this pull request Jul 13, 2026
Closed
@sdf-jkl

sdf-jkl commented Jul 13, 2026

Copy link
Copy Markdown
MemberAuthor

No. I checked the implementations I could find that use this corpus, and they either consume it as a git submodule (arrow-rs, arrow-go, arrow-dotnet, arrow-cpp, datafusion) or pin it to an explicit revision. iceberg-go pins a REF in gen_fixtures.sh, elasticsearch fetches from a pinned commit hash and excludes shredded_variant/ anyway, and parquet-go vendors a subset into its own testdata. For all of those, merging is a no-op until they choose to bump.

And when they bump, they are fine. -INVALID is an existing convention (cases 43, 84 and 125 already use it) that consumers key off generically rather than by case number. arrow-go skips any case whose parquet_file contains -INVALID and prints the notes (variant_test.go#L229), and arrow-dotnet does the same in its conformance suite. Both read the filenames from cases.json, which this PR updates in lockstep. No cases are added, removed or reordered, and no file contents change, so positional indexing into the array (as arrow-rs does) still works, and implementations that choose to leniently read the shredded value still can.

The one project I found that this does affect is hardwood. It shallow-clones parquet-testing master at HEAD (unpinned), enumerates shredded_variant/*.parquet by directory walk rather than from cases.json, and keys its skip-lists on exact filenames, so cases 41, 131 and 138 would stop being skipped. Small fix on their side, and I have opened an issue there: hardwood-hq/hardwood#783.

I cannot claim to have found every consumer. parquet-testing has no dependency graph, so this is not exhaustively enumerable. If any implementer pins to master, please shout.

@alambalamb mentioned this pull request Jul 15, 2026
Closed
alamb
alamb approved these changes Jul 15, 2026
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sdf-jkl -- this makes sense to me to properly label the invalid cases as INVALID in the testing repo

cc @rdblue

@alamb

alamb commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

I plan to merge this PR in the next day or two unless anyone would like more time to offer comments

rdblue
rdblue approved these changes Jul 21, 2026
View reviewed changes

@rdbluerdblue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me. I think implementations should choose to read, but that is for resilience, not adherence to the spec.

@alamb
alamb merged commit cf1eed4 into apache:masterJul 22, 2026
@alamb

alamb commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Thank you everyone for the help

@gunnarmorlinggunnarmorling mentioned this pull request Jul 22, 2026
Merged
gunnarmorling added a commit to hardwood-hq/hardwood that referenced this pull request Jul 22, 2026
@gunnarmorling
#783Key shredded_variant invalid-skip off the -INVALID suffix
fc18b31
apache/parquet-testing#117 renamed the shredded_variant fixtures whose
variant group omits the spec-required `value` column to carry an `-INVALID`
suffix. Our skip-lists keyed on exact pre-rename filenames, so a directory
walk still found the renamed files while the skip no longer matched them:
once the corpus is re-cloned at HEAD, cases 41/131/138 start being exercised
and turn CI red (Hardwood fail-fasts on the malformed group; parquet-java
NPEs), and case 132 begins to be compared.
Skip these deliberately by the stable `-INVALID` suffix upstream keeps in
lockstep with cases.json, rather than by name, so future relabels don't
reopen this gap. The suffix now covers case 132 as well: reader behaviour on
spec-invalid fixtures is implementation-defined, so pinning it to a byte- or
oracle-level match asserts more than the spec guarantees.
The one remaining name-keyed group is the fixtures cases.json marks as error
cases (an `error_message` and no variant payload). cases.json exposes nothing
the `-INVALID` suffix can key on for those, so they stay explicit — but as a
single SHREDDED_VARIANT_ERROR_CASES constant shared by both the comparison and
reassembly tests rather than duplicated in each.
gunnarmorling added a commit to hardwood-hq/hardwood that referenced this pull request Jul 22, 2026
@gunnarmorling
#783Key shredded_variant invalid-skip off the -INVALID suffix
b6a542c
apache/parquet-testing#117 renamed the shredded_variant fixtures whose
variant group omits the spec-required `value` column to carry an `-INVALID`
suffix. Our skip-lists keyed on exact pre-rename filenames, so a directory
walk still found the renamed files while the skip no longer matched them:
once the corpus is re-cloned at HEAD, cases 41/131/138 start being exercised
and turn CI red (Hardwood fail-fasts on the malformed group; parquet-java
NPEs), and case 132 begins to be compared.
Skip these deliberately by the stable `-INVALID` suffix upstream keeps in
lockstep with cases.json, rather than by name, so future relabels don't
reopen this gap. The suffix now covers case 132 as well: reader behaviour on
spec-invalid fixtures is implementation-defined, so pinning it to a byte- or
oracle-level match asserts more than the spec guarantees.
The one remaining name-keyed group is the fixtures cases.json marks as error
cases (an `error_message` and no variant payload). cases.json exposes nothing
the `-INVALID` suffix can key on for those, so they stay explicit — but as a
single SHREDDED_VARIANT_ERROR_CASES constant shared by both the comparison and
reassembly tests rather than duplicated in each.
gunnarmorling added a commit to hardwood-hq/hardwood that referenced this pull request Jul 22, 2026
@gunnarmorling
#783Key shredded_variant invalid-skip off the -INVALID suffix
6586e78
apache/parquet-testing#117 renamed the shredded_variant fixtures whose
variant group omits the spec-required `value` column to carry an `-INVALID`
suffix. Our skip-lists keyed on exact pre-rename filenames, so a directory
walk still found the renamed files while the skip no longer matched them:
once the corpus is re-cloned at HEAD, cases 41/131/138 start being exercised
and turn CI red (Hardwood fail-fasts on the malformed group; parquet-java
NPEs), and case 132 begins to be compared.
Skip these deliberately by the stable `-INVALID` suffix upstream keeps in
lockstep with cases.json, rather than by name, so future relabels don't
reopen this gap. The suffix now covers case 132 as well: reader behaviour on
spec-invalid fixtures is implementation-defined, so pinning it to a byte- or
oracle-level match asserts more than the spec guarantees.
The one remaining name-keyed group is the fixtures cases.json marks as error
cases (an `error_message` and no variant payload). cases.json exposes nothing
the `-INVALID` suffix can key on for those, so they stay explicit — but as a
single SHREDDED_VARIANT_ERROR_CASES constant shared by both the comparison and
reassembly tests rather than duplicated in each.
dhoard pushed a commit to dhoard/hardwood that referenced this pull request Aug 11, 2026
@gunnarmorling@dhoard
hardwood-hq#783Key shredded_variant invalid-skip off the -INVALID su…
8e927ce
…ffix
apache/parquet-testing#117 renamed the shredded_variant fixtures whose
variant group omits the spec-required `value` column to carry an `-INVALID`
suffix. Our skip-lists keyed on exact pre-rename filenames, so a directory
walk still found the renamed files while the skip no longer matched them:
once the corpus is re-cloned at HEAD, cases 41/131/138 start being exercised
and turn CI red (Hardwood fail-fasts on the malformed group; parquet-java
NPEs), and case 132 begins to be compared.
Skip these deliberately by the stable `-INVALID` suffix upstream keeps in
lockstep with cases.json, rather than by name, so future relabels don't
reopen this gap. The suffix now covers case 132 as well: reader behaviour on
spec-invalid fixtures is implementation-defined, so pinning it to a byte- or
oracle-level match asserts more than the spec guarantees.
The one remaining name-keyed group is the fixtures cases.json marks as error
cases (an `error_message` and no variant payload). cases.json exposes nothing
the `-INVALID` suffix can key on for those, so they stay explicit — but as a
single SHREDDED_VARIANT_ERROR_CASES constant shared by both the comparison and
reassembly tests rather than duplicated in each.
@sdf-jklsdf-jkl mentioned this pull request Sep 2, 2026
Merged
Jefffrey pushed a commit to apache/arrow-rs that referenced this pull request Sep 2, 2026
@sdf-jkl@cetra3
Add new parquet tests based on updated variant/json files (#10785)
2a1ba41
# Which issue does this PR close?
- Follow-up to #10786, which updated the `parquet-testing` revision.
- No tracking issue yet; one can be added if needed.
# Rationale for this change
The updated `parquet-testing` revision included:
-
[apache/parquet-testing#113](apache/parquet-testing#113):
malformed and edge-case Variant files.
-
[apache/parquet-testing#117](apache/parquet-testing#117):
mark four shredded Variant cases as invalid because they omit required
`value` columns.
-
[apache/parquet-testing#118](apache/parquet-testing#118):
JSON and BSON logical-type files.
-
[apache/parquet-testing#119](apache/parquet-testing#119):
an extended ALP fixture.
#10786 made these fixtures available to Arrow Rust, but did not exercise
the JSON, BSON, or malformed Variant files.
The same revision also added an extended ALP fixture. That fixture is
covered by the ALP encoder/decoder work in #9372, where the required
decoding support exists.
# What changes are included in this PR?
- Read the JSON logical-type fixture and verify all decoded string
values.
- Read the BSON logical-type fixture and verify all decoded binary
values.
- Validate all 14 files under `bad_data/variants`, including the one
valid duplicate-offset case and 13 malformed cases.
The four shredded Variant cases renamed as invalid by the same
`parquet-testing` update are already exercised by the existing Variant
integration harness.
# Are these changes tested?
Yes. The Parquet and Variant integration tests pass locally, along with
formatting and clippy checks. The PR's CI checks are also green.
# Are there any user-facing changes?
No. This PR only adds integration-test coverage for existing
`parquet-testing` fixtures.
# AI usage
This PR was prepared with OpenAI Codex and reviewed by a human. The
integration tests, formatting, and clippy checks described above were
run against the final branch, and the GitHub CI checks passed.
---------
Co-authored-by: cetra3 <cetra3@hotmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rdbluerdbluerdblue approved these changes
@alambalambalamb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

shredded_variant: cases 41, 131, 132, 138 omit the required 'value' column but are labeled as valid cases

4 participants

@sdf-jkl@wgtmac@alamb@rdblue