moomindani commented on code in PR #9:
URL:
https://github.com/apache/iceberg-verification/pull/9#discussion_r4042831026
##########
table-spec/types/geospatial/cases.json:
##########
@@ -0,0 +1,83 @@
+{
+ "cases": [
+ {
+ "id": "geometry-crs84",
+ "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>)\"",
+ "spec_ref": "format/spec.md#appendix-c-json-serialization"
+ },
+ {
+ "id": "geometry-srid",
+ "valid": true,
+ "input": "geometry(srid:4326)",
+ "decoded": {"type": "geometry", "crs": "srid:4326"},
+ "clause": "geometry(C) example from Appendix C is the unquoted
\"geometry(srid:4326)\"",
+ "spec_ref": "format/spec.md#appendix-c-json-serialization"
+ },
+ {
+ "id": "geometry-default-crs",
+ "valid": true,
+ "input": "geometry",
+ "decoded": {"type": "geometry", "crs": "OGC:CRS84"},
Review Comment:
Thanks — for what it's worth, here is what the two implementations actually
emit for the default CRS today, since this now has a `canonical` pinned to it.
- **Java always writes the parameters.** `crs` can never be null
(`Types.java:596`, `crs == null ? DEFAULT_CRS : crs`) and `toString()` formats
it (`Types.java:631`), so a geometry built without a CRS re-serializes as
`geometry(OGC:CRS84)` — exactly what this PR pins.
- **PyIceberg always elides the default.** `str(GeometryType())` is
`geometry`, and `str(GeometryType("OGC:CRS84"))` is *also* `geometry`, so an
explicitly supplied default is normalized away. A non-default CRS is emitted
(with the quotes we already discussed).
I read the canonical-forms sentence at `format/spec.md:1693` as supporting
your choice: the table's form is `"geometry(<C>)"`, so pinning
`geometry(OGC:CRS84)` is the defensible reading, and `geometry-default-crs`,
`geography-default` and (once its read side is fixed) `geometry-crs84` failing
PyIceberg is the corpus doing its job rather than a fixture problem.
The one thing the spec never says either way is whether *eliding* a default
when writing is a violation or a permitted alternative — the sentence declares
what the canonical form is, not that a writer must not elide. @szehon-ho if you
read it as a violation, I'll take it to the PyIceberg side as a follow-up to
apache/iceberg-python#3530, which is already dealing with the quoting half of
the same divergence.
##########
table-spec/types/primitive/cases.json:
##########
@@ -0,0 +1,33 @@
+{
+ "cases": [
+ { "id": "boolean", "valid": true, "input": "boolean", "decoded": { "type":
"boolean" }, "canonical": "boolean", "clause": "Primitive Types: boolean;
Appendix C canonical string", "spec_ref":
"format/spec.md#appendix-c-json-serialization" },
+ { "id": "int", "valid": true, "input": "int", "decoded": { "type": "int"
}, "canonical": "int", "clause": "Primitive Types: int; Appendix C canonical
string", "spec_ref": "format/spec.md#appendix-c-json-serialization" },
+ { "id": "long", "valid": true, "input": "long", "decoded": { "type":
"long" }, "canonical": "long", "clause": "Primitive Types: long; Appendix C
canonical string", "spec_ref": "format/spec.md#appendix-c-json-serialization" },
+ { "id": "float", "valid": true, "input": "float", "decoded": { "type":
"float" }, "canonical": "float", "clause": "Primitive Types: float; Appendix C
canonical string", "spec_ref": "format/spec.md#appendix-c-json-serialization" },
+ { "id": "double", "valid": true, "input": "double", "decoded": { "type":
"double" }, "canonical": "double", "clause": "Primitive Types: double; Appendix
C canonical string", "spec_ref": "format/spec.md#appendix-c-json-serialization"
},
+ { "id": "date", "valid": true, "input": "date", "decoded": { "type":
"date" }, "canonical": "date", "clause": "Primitive Types: date; Appendix C
canonical string", "spec_ref": "format/spec.md#appendix-c-json-serialization" },
+ { "id": "time", "valid": true, "input": "time", "decoded": { "type":
"time" }, "canonical": "time", "clause": "Primitive Types: time; Appendix C
canonical string", "spec_ref": "format/spec.md#appendix-c-json-serialization" },
+ { "id": "timestamp", "valid": true, "input": "timestamp", "decoded": {
"type": "timestamp" }, "canonical": "timestamp", "clause": "Primitive Types:
timestamp; Appendix C canonical string", "spec_ref":
"format/spec.md#appendix-c-json-serialization" },
+ { "id": "timestamptz", "valid": true, "input": "timestamptz", "decoded": {
"type": "timestamptz" }, "canonical": "timestamptz", "clause": "Primitive
Types: timestamptz; Appendix C canonical string", "spec_ref":
"format/spec.md#appendix-c-json-serialization" },
+ { "id": "timestamp_ns", "valid": true, "input": "timestamp_ns", "decoded":
{ "type": "timestamp_ns" }, "canonical": "timestamp_ns", "clause": "Primitive
Types: timestamp_ns added in v3; Appendix C canonical string", "spec_ref":
"format/spec.md#primitive-types" },
+ { "id": "timestamptz_ns", "valid": true, "input": "timestamptz_ns",
"decoded": { "type": "timestamptz_ns" }, "canonical": "timestamptz_ns",
"clause": "Primitive Types: timestamptz_ns added in v3; Appendix C canonical
string", "spec_ref": "format/spec.md#primitive-types" },
+ { "id": "string", "valid": true, "input": "string", "decoded": { "type":
"string" }, "canonical": "string", "clause": "Primitive Types: string; Appendix
C canonical string", "spec_ref": "format/spec.md#appendix-c-json-serialization"
},
+ { "id": "uuid", "valid": true, "input": "uuid", "decoded": { "type":
"uuid" }, "canonical": "uuid", "clause": "Primitive Types: uuid; Appendix C
canonical string", "spec_ref": "format/spec.md#appendix-c-json-serialization" },
+ { "id": "binary", "valid": true, "input": "binary", "decoded": { "type":
"binary" }, "canonical": "binary", "clause": "Primitive Types: binary; Appendix
C canonical string", "spec_ref": "format/spec.md#appendix-c-json-serialization"
},
+ { "id": "unknown", "valid": true, "input": "unknown", "decoded": { "type":
"unknown" }, "canonical": "unknown", "clause": "Primitive Types: unknown added
in v3; Appendix C canonical string", "spec_ref":
"format/spec.md#primitive-types" },
+ { "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" },
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
