Conversation
|
Reviewed from the consumer side: I ran the cases in this PR against PyIceberg on current The geospatial cases work exactly as intended, and they catch a live defect. Spelling "unquoted" into the clauses was the right call. PyIceberg's The decimal whitespace case stops one step short of the interesting input. On the |
laskoviymishka
left a comment
There was a problem hiding this comment.
Good first cut. The base/types schema split makes sense, expected values come from the spec rather than one implementation, and the JSON Schema validator + RAT in CI give us a useful starting point. Also good that moomindani already ran this against PyIceberg and found a real divergence.
I’d still hold the merge for a few things, mainly because this becomes the contract other implementations will validate against.
The biggest one is geospatial serialization. A few geometry/geography cases describe the canonical serialized form in the clause but don’t have a canonical field. moomindani’s run shows why that matters: PyIceberg writes the quoted CRS form, while Java reads those quotes into the CRS itself, so the two sides diverge (apache/iceberg-python#3530). I’d add canonical to those cases and settle the default-CRS form here.
The decimal whitespace case is a bit different. decimal-9-2-spaced currently uses decimal(9, 2), which all the implementations checked already accept, so it doesn’t really distinguish strict vs lenient parsing. The more useful case is decimal( 9 , 2 ) from apache/iceberg#16798: PyIceberg rejects it and Java accepts it, and both are still conformant. That probably means this needs a small normative distinction, e.g. optional normative_level with default must, so a SHOULD case isn’t encoded as MUST.
Before merge I’d fix:
- add
canonicalto the geospatial cases that already pin the serialized form, and decide the default-CRS representation - change the decimal whitespace fixture to
decimal( 9 , 2 )and add optional normative level - pin the
jsonschemadependency and add one negative self-test so CI proves the validator can fail - set
additionalProperties: falseon the case schema so typos don’t silently pass
The rest is in the inline comments. After these, I’m happy to take another look.
| "valid": true, | ||
| "input": "geometry(OGC:CRS84)", | ||
| "decoded": {"type": "geometry", "crs": "OGC:CRS84"}, | ||
| "clause": "geometry(C) with explicit CRS; the canonical serialized form is unquoted \"geometry(<C>)\"", |
There was a problem hiding this comment.
The clause here says the canonical form is the unquoted geometry(OGC:CRS84), but without a canonical field the write direction never gets tested — an implementation that emits geometry('OGC:CRS84') or GEOMETRY(OGC:CRS84) would pass. Since the clause already pins the spelling, I'd add canonical to each explicit case: "canonical": "geometry(OGC:CRS84)" here, "geography(OGC:CRS84, spherical)" on the geography ones, and so on. Same applies to geometry-srid and the four geography algorithm cases below.
moomindani's consumer-side run makes this concrete: PyIceberg's regex requires a quoted CRS, so it rejects geometry(srid:4326) and writes geometry('srid:4326'), while Java reads the quotes into the CRS — a silent bidirectional divergence this fixture catches once canonical pins the write side (they filed apache/iceberg-python#3530).
| { "id": "fixed-1", "valid": true, "input": "fixed[1]", "decoded": { "type": "fixed", "length": 1 }, "canonical": "fixed[1]", "clause": "Appendix C: fixed canonical string is fixed[<L>]", "spec_ref": "format/spec.md#appendix-c-json-serialization" }, | ||
| { "id": "fixed-16", "valid": true, "input": "fixed[16]", "decoded": { "type": "fixed", "length": 16 }, "canonical": "fixed[16]", "clause": "Appendix C: fixed canonical string is fixed[<L>]", "spec_ref": "format/spec.md#appendix-c-json-serialization" }, | ||
| { "id": "decimal-9-2", "valid": true, "input": "decimal(9,2)", "decoded": { "type": "decimal", "precision": 9, "scale": 2 }, "clause": "Appendix C: both decimal(9,2) and decimal(9, 2) are canonical, so no byte-exact form is pinned", "spec_ref": "format/spec.md#appendix-c-json-serialization" }, | ||
| { "id": "decimal-9-2-spaced", "valid": true, "input": "decimal(9, 2)", "decoded": { "type": "decimal", "precision": 9, "scale": 2 }, "clause": "Appendix C: the spaced decimal(9, 2) form parses to the same decimal", "spec_ref": "format/spec.md#appendix-c-json-serialization" }, |
There was a problem hiding this comment.
moomindani's consumer-side run reframed this one for me: decimal(9, 2) is accepted by every implementation they checked, so as written this case is inert — it doesn't separate a strict parser from a lenient one. The input that does is the broader form apache/iceberg#16798 actually added, "whitespace around parameters and separators": PyIceberg rejects decimal( 9 , 2 ) / decimal(9 ,2) while Java accepts them, and both are conformant.
That's the case I'd want on this surface (the sungwy/iceberg-testing prototype carried it, noting "a reader stricter than that diverges here"). But because both parsers are conformant on it, it can't be a plain valid: true — a MUST-accept would fail PyIceberg for behavior the spec only recommends. So I'd swap in the divergent decimal( 9 , 2 ) input and give it a normative tier: an optional normative_level on the base schema (default "must") with this case marked as the SHOULD, so runners treat it as advisory rather than a hard fail.
I'd leave canonical off the decimal cases as you have it — agreed the pinned-spelling question is spec text, not something to settle in a fixture. wdyt on the normative tier?
There was a problem hiding this comment.
I put in a normative_level for now, but I'd like to hear from others if that's good enough.
There was a problem hiding this comment.
The tier reads right to me, and the wording it encodes is already in the spec: format/spec.md:1693 says readers should accept the optional whitespace, so a reader that rejects decimal( 9 , 2 ) stays conformant and a plain MUST-accept fixture would fail it for behavior that is only recommended. Defaulting to must also means no existing case changes meaning.
One thing worth writing into the schema description while it is fresh: a runner has to report a failed SHOULD distinctly from a pass, otherwise the advisory tier is invisible in practice and contributors will assume the case is inert again.
There was a problem hiding this comment.
On defining the states, I'd suggest four rather than three, based on running this surface and the schema surface in #3 against PyIceberg:
passfail— amustcase that failed. The only state that makes a run nonzero.advisory_fail— ashouldcase that failed. Reported, never blocking.skip— the case was not run: the surface is not subscribed, or the feature is absent.
skip is the one I would not leave implicit. PyIceberg has no variant type at all, so both variant cases on the schema surface cannot run — and a runner that folds "not run" into "passed" reports green for a type the client does not implement, which is the opposite of what this corpus is for. A first-class skip also gives a client's own skip list something to name.
|
Added @szehon-ho here to add some perspective on Geo and how we should represent them. Also, CC: @huan233usc |
7e0a119 to
2bc32a5
Compare
2bc32a5 to
63064f3
Compare
laskoviymishka
left a comment
There was a problem hiding this comment.
Most of round 1 landed: per-surface ids, canonical on the explicit geospatial cases, decimal( 9 , 2 ) at normative_level: should, the bounded jsonschema pin, additionalProperties: false on both layers, the license_check push trigger, and the variant(x) reject plus the self-test. I'd still hold a bit, mostly on cross-implementation portability, since this corpus is what every client validates against.
Three interop threads:
- The default-CRS canonicals match Java but as MUST they flunk PyIceberg on write, which elides the default and emits bare
geometry(apache/iceberg-python#3530). @szehon-ho hasn't ruled on whether eliding is a violation, so I'd gate these on the ruling or drop toshouldrather than fail a client on unsettled policy. geography(OGC:CRS84)(CRS, no algorithm) is accepted by Java and re-serialized asgeography(OGC:CRS84, spherical), but the suite is silent on it. Agreed a MUST over-specifies; ashouldcase flags a stricter or looser parser without over-reaching.- The README's "decimal has two blessed forms, so no canonical" doesn't match Appendix C: the format column
decimal(<P>,<S>)is the canonical form and Java writes the spaceddecimal(9, 2), so the write direction is untested for the most common parameterized type. Quote the text that blesses both, or pincanonical.
Two gate bugs I hit re-reading the validator itself:
- The invalid-case rule in
cases.base.schema.jsondoesn't forbiddecodedon a reject case;not/requiredonly means the key isn't required. Useproperties: {decoded: false}. - The self-test has no
set -e, so a setup failure makes it pass green without testing anything. Addset -eand a file guard, and extend it to unknown-property and duplicate-id.
Smaller: two leftover quoting spots in dev/check-license (wget ${URL}, $java_cmd -jar); the normative_level note says report a failed should "distinctly" without defining the states, so a pass/fail/advisory_fail enum now saves churn once runners land next PR; and the validate-fixtures.py docstring still says ids are "globally unique." The RAT checksum from last round stays fine as-is, consistent with the ASF scripts.
Details inline. Square away the two gate bugs and the interop threads and I'm happy to approve.
| "valid": true, | ||
| "input": "geometry", | ||
| "decoded": {"type": "geometry", "crs": "OGC:CRS84"}, | ||
| "canonical": "geometry(OGC:CRS84)", |
There was a problem hiding this comment.
These default-CRS canonicals (geometry(OGC:CRS84) here, geography(OGC:CRS84, spherical) on geography-default) match Java, which always writes the fully-parameterized form. But as MUST they flunk PyIceberg on the write direction: it elides the default and emits bare geometry (apache/iceberg-python#3530), and @szehon-ho hasn't ruled on whether eliding is actually a violation.
I'd not fail a client on unsettled policy. Gate these on the ruling, or mark normative_level: "should" until the spec side lands.
| { | ||
| "id": "geography-crs84-spherical", | ||
| "valid": true, | ||
| "input": "geography(OGC:CRS84, spherical)", |
There was a problem hiding this comment.
Agreed a MUST would over-specify: the spec doesn't pin a one-parameter geography(<C>) form. But Java accepts it, so I'd not leave the suite silent. The algorithm group in the Types.java:69 regex is optional, so geography(OGC:CRS84) parses with algorithm defaulting to spherical and re-serializes as geography(OGC:CRS84, spherical). An impl that rejects it, or defaults the algorithm differently, diverges from Java at parse time and nothing here catches it.
should is the honest tier — it flags the divergence without a MUST the spec doesn't back:
{
"id": "geography-crs-only",
"valid": true,
"normative_level": "should",
"input": "geography(OGC:CRS84)",
"decoded": {"type": "geography", "crs": "OGC:CRS84", "algorithm": "spherical"},
"canonical": "geography(OGC:CRS84, spherical)",
"clause": "geography(C, A): if A is unspecified but C is given, A defaults to spherical (Java reference behavior; Appendix C defines geography(<C>, <A>) as canonical)",
"spec_ref": "format/spec.md#appendix-c-json-serialization"
}wdyt?
| successful parse fails. | ||
|
|
||
| `canonical` is present only where the spec pins one spelling. `decimal` has two | ||
| blessed forms (`decimal(9,2)` and `decimal(9, 2)`), so its cases have no |
There was a problem hiding this comment.
The "two blessed forms, so no canonical" framing doesn't match Appendix C. The format column is decimal(<P>,<S>) (no space) and spec.md:1695 calls the format-column strings the canonical forms; the two examples are what a reader must accept, not co-equal outputs. Java writes the spaced decimal(9, 2) (Types.java:549), so the write direction is untested for our most common parameterized type: an impl emitting either spelling passes today.
Either quote the Appendix C text that blesses both as co-equal canonical, or pin canonical: "decimal(9, 2)" on decimal-9-2 and decimal-9-2-spaced so we test what implementations actually write.
There was a problem hiding this comment.
The unspaced format column you are citing was fixed a few hours after this review: #18145 merged as b5f363932 on the 21st, and the row now reads
|**`decimal(P, S)`**|`JSON string: "decimal(<P>, <S>)"`|`"decimal(9, 2)"`|
one template and one example, both spaced. So there is no Appendix C text blessing the two spellings as co-equal canonical forms: the spaced form is the canonical one, and the unspaced one is only something a reader must accept. canonical: "decimal(9, 2)" on both decimal-9-2 and decimal-9-2-spaced is the accurate reading now, and the README's "two blessed forms" line goes with it.
For context, that spec change came out of this thread — the decimal row had been left half-applied by #16798, whose sibling #16799 had already spaced the geography row.
| { | ||
| "$comment": "an invalid case must not carry decoded", | ||
| "if": { "properties": { "valid": { "const": false } }, "required": ["valid"] }, | ||
| "then": { "not": { "required": ["decoded"] } } |
There was a problem hiding this comment.
{"not": {"required": ["decoded"]}} doesn't forbid decoded here: it only asserts the key isn't required, which is always true, so a valid: false case carrying a stray decoded validates clean. That's the one invariant this schema most needs to hold.
The forbidding form is "then": {"properties": {"decoded": false}}. Worth a self-test feeding a reject case with decoded present, since CI only covers the other direction today.
| - name: Validate cases.json | ||
| run: python3 dev/validate-fixtures.py | ||
| - name: Validator rejects a malformed fixture (self-test) | ||
| run: | |
There was a problem hiding this comment.
No set -e in this block, so the self-test can pass without testing anything: if mkdir or printf fails, the validator runs on an empty dir, exits nonzero because it finds no cases.json, the if ...; then ... exit 1 branch isn't taken, and the step reports green. Add set -e and a test -f guard before the run.
It also only covers valid + no-decoded. additionalProperties: false and duplicate-id, the whole point of the self-test, still have no CI proof; a typo'd field like spec-ref sails through. An unknown-property case and a duplicate-id case would close that.
| if [ $(command -v curl) ]; then | ||
| curl -L --silent "${URL}" > "$JAR_DL" && mv "$JAR_DL" "$JAR" | ||
| elif [ $(command -v wget) ]; then | ||
| wget --quiet ${URL} -O "$JAR_DL" && mv "$JAR_DL" "$JAR" |
There was a problem hiding this comment.
Two quoting spots from item H didn't land with the rest. wget --quiet ${URL} is still unquoted while the curl branch got "${URL}", and $java_cmd -jar at line 70 breaks when JAVA_HOME has a space (CI masks it, the runners' has none). wget --quiet "${URL}" and "$java_cmd" -jar.
Rationale for this change
Adds the first conformance surface: spec-derived type fixtures under
table-spec/types/(parse(input) == decoded, withvalidmarking accept vs reject), validated by JSON Schema. Runners that exercise the fixtures against each implementation follow in later PRs.Are these changes tested?
Yes.
dev/validate-fixtures.pyvalidates everycases.jsonagainst the schemas and the unique-id rule in CI, anddev/check-licenseruns Apache RAT.Are there any user-facing changes?
No.
AI Disclosure
Developed with Claude (Claude Code); fully reviewed by the author.