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.

[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.

[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.

[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?

[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 }.

[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.

[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" }.

[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.

[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.