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.
|
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.
3502b32 to
594df45
Compare
…ed-forbid schema, self-test, check-license quoting
b2b42ff to
9e3e3f3
Compare
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.