Adding Fixtures and Assertions for Schemas

[sungwy]

Adds the first fixture surface. Asserts parsed schema values against expected values.

• Added cases that span nested field types and other v1, v2 supported types in core/.
• variant, geometry, geography, unknown and timestamp-ns

Scaffolded by Claude, reviewed by human

[Copilot]

🟡 Changes recommended

The new README contains internal inconsistencies/omissions about subdirectory naming/scope and decimal whitespace tolerance relative to the added fixtures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds a new “schema decoding” table-spec surface consisting of JSON fixtures (cases.json) and accompanying documentation to enable consistent, cross-implementation verification of Iceberg schema JSON decoding and field-id/parent mapping.

Changes:

• Added table-spec/schema/README.md describing the assertion, scope, inputs, case format, and comparison rules for schema decoding.
• Added core schema decoding fixtures covering nesting, id spaces, identifier fields, docs, and v1/v2 primitive types.
• Added fixtures for additional v3-oriented type areas (geospatial, variant, unknown, timestamp-ns).

File summaries

File	Description
table-spec/schema/README.md	Defines the schema decoding surface, assertion, inputs, case format, and comparison rules.
table-spec/schema/core/cases.json	Adds baseline/negative schema decoding cases for structure, identifiers, docs, and primitive types.
table-spec/schema/geospatial/cases.json	Adds schema decoding cases for geometry/geography types and canonicalization defaults.
table-spec/schema/timestamp-ns/cases.json	Adds schema decoding cases for timestamp_ns and timestamptz_ns types.
table-spec/schema/unknown/cases.json	Adds a schema decoding case for the unknown type.
table-spec/schema/variant/cases.json	Adds schema decoding cases for the variant type at top-level and nested positions.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[laskoviymishka]

The decoded-projection approach here is a clean way to make schema decoding comparable across implementations — the flattened field rows with parent pointers, and pushing parameterized types like decimal and geography into decoded objects rather than strings, all read well.

I'd hold this before merging though, since it's the first conformance surface and later fixtures will copy its shape — worth getting the negative-case pattern right before it propagates.

Main concern is that the README names type-string serialization as the primary thing this surface catches, but nearly every valid: false case is an identifier-field violation. There's no rejection case for a malformed decimal or fixed, and no reject-unknown-required. A parser that accepts decimal(abc) or a required unknown field passes the whole surface right now — for an oracle, the negative cases are what give it teeth. The decimal(9,2) vs decimal(38, 10) split is related: it turns whitespace tolerance into an accidental, undocumented conformance property. Copilot flagged the decimal spacing too, and I agree.

Things I'd like to settle in this PR before the follow-ups build on it:

• a reject-unknown-required case (unknown must always be optional)
• at least one malformed-decimal and one malformed-fixed reject case, to match the stated scope
• consistent decimal whitespace, or an explicitly named case if we want to assert tolerance
• the float-identifier and map-nested-identifier twins, to match the existing double/list cases
• rename timestamp-ns/ (or loosen the README rule), and drop the CONTRIBUTING.md link since that file doesn't exist yet

One non-blocking heads-up: PyIceberg currently serializes and parses geography/geometry CRS strings quoted (geometry('srid:4326')), so the geography and geometry cases will fail against it. The fixtures are spec-correct and PyIceberg is the deviation here, but it's worth a note somewhere so implementors can tell a fixture bug from an implementation bug.

Once those land, happy to take another pass.

[laskoviymishka]

The README names type-string serialization (decimal(P, S)) as the primary thing this surface catches, but every valid: false case in core/ is an identifier-field violation — nothing exercises a malformed type string. A parser that happily accepts decimal(abc), decimal(39,2), or fixed[-1] passes the whole surface.

I'd add at least one malformed-decimal and one malformed-fixed reject case so the stated scope actually has teeth.

[sungwy]

good call out, but I'd argue that the type specific assertions of that granularity is better suited to be covered by the Type Conformance tests @nssalian is working on: #9

Let me update the README so that it is better aligned with the scope of this surface.

[laskoviymishka]

This links to CONTRIBUTING.md, but that file doesn't exist yet — the root README still marks it a TODO, so implementors following the reference hit a 404.

I'd drop the link and just say the writer phase is out of scope for this surface.

[sungwy]

we have CONTRIBUTING.md now: https://github.com/apache/iceberg-verification/blob/main/CONTRIBUTING.md

[laskoviymishka]

The README lists schema-id as something core/ covers, but every valid case carries an explicit schema-id and the spec makes it optional (defaults to 0). An implementation that mishandles the absent case never gets caught here.

I'd add one case that omits schema-id and decodes it to 0.

[sungwy]

The spec doesn't fix a default in the optional case.

Appendix C makes schema-id optional in v1 and required in v2, and the 0 is only in the Example column. Java and PyIceberg both fall back to 0, but that's an implementation choice.

If we want to add that case, I think we'd want to clarify the spec first, but I also don't want to waste the community's time in clarifying a V1 spec. I'd like to leave this case out for that reason

[laskoviymishka]

decimal-max-precision uses decimal(38, 10) (with a space) while all-primitive-types uses decimal(9,2) (no space), both valid: true. That quietly makes whitespace tolerance a conformance property the README never mentions — a strict parser fails one of these for a reason nobody documented. Copilot flagged this too.

Either normalize both to the canonical decimal(P, S) form, or keep the split but pull it into a dedicated, named decimal-whitespace case and say so in the README. How strict do we want the surface to be on type-string whitespace — wdyt?

[sungwy]

Agreed this is a gap, but as noted above, I think we'll cover these cases in #9

[laskoviymishka]

We reject a double identifier field here but not a float one, and the spec bars both by the same rule. An impl that only checks for double passes.

I'd add the float twin right alongside this case.

[laskoviymishka]

This rejects an identifier field inside an optional struct, which is right, but there's no positive case for an identifier inside a required nested struct (all ancestors required), which the spec allows. So an implementation that over-rejects every nested identifier passes all of these.

I'd add one positive nested-identifier case to pin down the accept side.

[laskoviymishka]

geography has three forms — bare, geography(C), and geography(C, A) — and we only test the first and third. The single-argument form (explicit CRS, default algorithm) is exactly where a broken parser drops or mis-defaults the algorithm.

I'd add a geography(srid:4326) case decoding to { crs: srid:4326, algorithm: spherical }.

[sungwy]

Agreed this is a gap, but as noted above, I think we'll cover these cases in #9

[laskoviymishka]

Two things here clash with the README. The directory is timestamp-ns (hyphen) but the v3 type is timestamp_ns (underscore), and the README says each subdirectory is "named for" its type — so the rule points at timestamp_ns/. This case also covers two types (timestamp_ns and timestamptz_ns), so "covers one v3 type" isn't quite accurate, and an implementor opting out of one silently loses the other.

I'd either rename to timestamp_ns/ and split the two types, or loosen the README rule to allow a named group. Copilot raised the naming mismatch too.

[sungwy]

Agreed this is a gap, but as noted above, I think we'll cover these cases in #9

[laskoviymishka]

This is the only case in the subdirectory and it's the happy path — there's no valid: false twin for a required unknown field. The spec is explicit that unknown must always be optional, so an implementation that accepts required: true here passes everything in this surface.

I'd add a reject-unknown-required case with { id, name, required: true, type: "unknown" }.

[sungwy]

Adding this

[laskoviymishka]

Both variant cases put it at the top level or inside a struct, but the spec treats variant as first-class — it can be a list element or a map value. A parser that only special-cases variant in struct position passes both of these.

I'd add a list<variant> and/or map<string, variant> case.

[sungwy]

Agreed this is a gap, but as noted above, I think we'll cover these cases in #9

[moomindani]

Ran this PR's 25 cases against PyIceberg main (0d584073) from the consumer side. Results first, then one suggestion.

core/ is sound as written. All 11 valid cases decode exactly as decoded says — nesting, the element/key/value rows, out-of-order fields, doc, all-primitive-types, decimal-max-precision — and all 6 reject-identifier-* cases raise. Nothing to report there.

Both explicit-CRS cases fail PyIceberg, for the same reason as the geospatial cases in #9: geometry(srid:4326) and geography(srid:4326, vincenty) are rejected because GEOMETRY_REGEX / GEOGRAPHY_REGEX require a quoted CRS, while Appendix C's examples are unquoted (apache/iceberg-python#3530 is the fix). These are Appendix C's own example strings, so the cases are right and the implementation is wrong — and two independent surfaces now catch the one bug.

variant/ cannot run against PyIceberg at all: there is no variant type in pyiceberg/types.py, so both cases fail at Unsupported field type: 'variant'. That is the per-subdirectory subscription in your README working as intended — PyIceberg opts into everything except variant/ until it has one.

Suggestion: cover initial-default and write-default. No input here carries either, and the decoded row has no slot for them, so this surface cannot catch a divergence in default parsing. Two reasons I think it belongs here:

• initial-default decides what a reader returns for a column a data file does not contain, so a divergence is a wrong query result rather than a metadata nit.
• format/spec.md:335 is a rule with a built-in rejection case: "All columns of unknown, variant, geometry, and geography types must default to null. Non-null values for initial-default or write-default are invalid." Nothing verifies it today. PyIceberg does reject a non-null default on those types, but incidentally rather than by that rule — unknown fails with Cannot deserialize bytes, type unknown not supported and geometry with Geometry JSON deserialization requires WKT to WKB conversion, which is not yet implemented. A fixture would pin the rule where implementations currently reach the right answer by accident.

The rejection cases fit the current format as is; the valid case would need a slot for the two keys in the decoded row, which is a change to this surface's contract rather than something to slip in. Happy to write either or both, here or as a follow-up, if you want them.

[laskoviymishka]

Everything from the last round is in now: the two identifier rejects, reject-unknown-required, and the decimal whitespace cleanup. Pulling out the geo, variant, and timestamp-ns cases also makes the naming/link comments moot. Thanks for working through all of it.

On the reject coverage: identifiers are covered now, but we still don’t have cases for duplicate field IDs, IDs past the Integer.MAX_VALUE - 200 cap, non-struct roots, or malformed schema IDs. I think those are worth adding, but I wouldn’t block this first surface on them. Fine to track them separately, and the type-grammar rejects can probably go under #9.

One thing I would still fix here: the README says type-string grammar “is covered by” table-spec/types/, but that surface does not exist yet. I’d just say type-string grammar is out of scope here and leave it at that.

Two small nits can also move to the follow-up: the core/ inputs line now conflicts with the reject-* cases below it, and reject-unknown-required only checks a top-level field.

Also worth updating the PR description since variant/geometry/geography/timestamp-ns are gone now.

Drop the table-spec/types/ claim and I’m good with this merging.

[laskoviymishka]

All ten valid: false cases here are identifier-field-ids violations, so five of the six things the README lists as in-scope — duplicate field id, id at/above the Integer.MAX_VALUE - 200 cap, non-struct root, missing required key/name/type, malformed schema-id — have no rejection case yet. Since this is the shape later surfaces copy, I'd want that coverage to exist eventually.

Not blocking this PR though — fine to land the positive cases now and add the structural rejects under a tracked issue. The only condition I'd put on the deferral is the README tweak below, so the doc doesn't claim coverage we're punting on. Happy to file the follow-up if you'd like.

[laskoviymishka]

This is the small piece I'd keep in this PR. table-spec/types/ isn't on main or this branch yet — it's only tracked in #9 — so pointing at it reads as if malformed-decimal/fixed rejection is already handled somewhere, when it isn't handled anywhere. Nothing rejects decimal(abc) or fixed[-1] today.

I'd drop the is covered by the types surface under table-spec/types/ clause entirely and just say type-string grammar (decimal(P, S) / fixed[L] parsing and rejection) is out of scope for this surface — full stop, without asserting where it's covered. Not pointing anywhere is cleaner than pointing at a path that doesn't exist, and it's what makes deferring the reject cases honest.

[laskoviymishka]

Small one while we're here: this blanket claim collides with the reject-* cases right below it — about half of core/'s inputs are valid: false because they're deliberately malformed. Scoping it to "Every valid: true input in core/ is valid writer output for versions 1, 2 and 3; valid: false inputs are deliberately malformed" fixes it. Fine to fold into the follow-up if you'd rather not touch it now.

[laskoviymishka]

unknown being optional is position-independent, but this only exercises a top-level field — an impl checking required-ness only at the root would pass while accepting element-required: true on an unknown list element or a required unknown map value. A nested twin (reject-unknown-required-in-list / -in-map-value) would close it, same way the identifier cases got theirs. Good candidate to bundle into the structural-coverage follow-up.

[sungwy]

Thanks for the review @laskoviymishka table-spec/types/ was made available once #9 was merged, so I'll merge this now as well.