laskoviymishka commented on code in PR #9:
URL:
https://github.com/apache/iceberg-verification/pull/9#discussion_r4127584748
##########
table-spec/types/geospatial/cases.json:
##########
@@ -0,0 +1,106 @@
+{
+ "cases": [
+ {
+ "id": "geometry-crs84",
+ "valid": true,
+ "normative_level": "should",
+ "input": "geometry(OGC:CRS84)",
+ "decoded": {"type": "geometry", "crs": "OGC:CRS84"},
+ "canonical": "geometry(OGC:CRS84)",
+ "clause": "geometry(C) with explicit CRS; Appendix C's canonical is
fully parameterized, but when C is the default OGC:CRS84 whether a writer may
elide it is unsettled, so the write direction is advisory",
+ "spec_ref": "format/spec.md#appendix-c-json-serialization"
+ },
+ {
+ "id": "geometry-srid",
+ "valid": true,
+ "input": "geometry(srid:4326)",
+ "decoded": {"type": "geometry", "crs": "srid:4326"},
+ "canonical": "geometry(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,
+ "normative_level": "should",
+ "input": "geometry",
+ "decoded": {"type": "geometry", "crs": "OGC:CRS84"},
+ "canonical": "geometry(OGC:CRS84)",
+ "clause": "geometry(C): if C is not specified, C is OGC:CRS84; Appendix
C's canonical is fully parameterized, but whether a writer may elide the
default is unsettled, so the write direction is advisory",
+ "spec_ref": "format/spec.md#appendix-c-json-serialization"
+ },
+ {
+ "id": "geography-crs84-spherical",
+ "valid": true,
+ "normative_level": "should",
+ "input": "geography(OGC:CRS84, spherical)",
+ "decoded": {"type": "geography", "crs": "OGC:CRS84", "algorithm":
"spherical"},
+ "canonical": "geography(OGC:CRS84, spherical)",
+ "clause": "geography(C, A) with explicit CRS and algorithm; Appendix C's
canonical is fully parameterized, but when C/A are the defaults
OGC:CRS84/spherical whether a writer may elide them is unsettled, so the write
direction is advisory",
+ "spec_ref": "format/spec.md#appendix-c-json-serialization"
+ },
+ {
+ "id": "geography-default",
+ "valid": true,
+ "normative_level": "should",
+ "input": "geography",
+ "decoded": {"type": "geography", "crs": "OGC:CRS84", "algorithm":
"spherical"},
+ "canonical": "geography(OGC:CRS84, spherical)",
+ "clause": "geography(C, A): if not specified, C is OGC:CRS84 and A is
spherical; Appendix C's canonical is fully parameterized, but whether a writer
may elide the default is unsettled, so the write direction is advisory",
+ "spec_ref": "format/spec.md#appendix-c-json-serialization"
+ },
+ {
+ "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"
Review Comment:
The clause is honest that the 1-arg `geography(C)` defaulting is Java
reference behavior, but `spec_ref` still points at Appendix C — which only
documents the 2-arg `geography(<C>, <A>)` form, not this one. Anyone auditing
citations (like I did last round) reads this as spec-backed when it isn't. I'd
point `spec_ref` at the Provenance section, or add a "cross-checked against
Java, not spec grammar" caveat in the clause.
##########
table-spec/types/geospatial/cases.json:
##########
@@ -0,0 +1,106 @@
+{
+ "cases": [
+ {
+ "id": "geometry-crs84",
+ "valid": true,
Review Comment:
The `should` demotion reached too far here. `geometry-crs84` and
`geography-crs84-spherical` have fully explicit inputs — nothing is elided — so
there's no "may a writer drop the default" question to hedge. That's not a
design call, it's just wrong: they're structurally identical to
`geometry-srid`, which correctly stays plain `must` (confirmed against Java and
PyIceberg — the parenthesized form parses deterministically, default-ness
doesn't enter into it). I'd revert both to `must`, dropping `normative_level`.
This is the one thing I'd hold the merge on, and it's a ~2-line change.
Separately, and not blocking: the two bare-input cases
(`geometry-default-crs`, `geography-default`) still fold the unhedged read MUST
(bare `geometry` → `crs: OGC:CRS84`, spec.md:284-285) into the same tier as the
contested write SHOULD (whether a writer may elide the default,
apache/iceberg-python#3530), so a parser that skips the default only gets
`advisory_fail`. The clean fix is to split each into a `must` decoded-only case
and a `should` canonical-only case. Since no runner consumes these fixtures
yet, I'm happy to spin this out to a follow-up rather than hold on it — let's
just track it in a follow-up issue so it doesn't get lost.
##########
dev/schema/cases.base.schema.json:
##########
@@ -0,0 +1,49 @@
+{
+ "$schema": "https://json-schema.org/draft/2020-12/schema";,
+ "$id": "https://iceberg.apache.org/verification/cases.base.schema.json";,
+ "title": "Conformance cases (base)",
+ "description": "Structure shared by every table-spec surface's cases.json.",
+ "type": "object",
+ "required": ["cases"],
+ "additionalProperties": false,
+ "properties": {
+ "cases": {
+ "type": "array",
+ "items": { "$ref": "#/$defs/case" }
+ }
+ },
+ "$defs": {
+ "case": {
+ "type": "object",
+ "required": ["id", "valid", "input"],
+ "additionalProperties": false,
+ "properties": {
+ "id": { "type": "string", "minLength": 1 },
+ "valid": { "type": "boolean" },
+ "input": true,
+ "decoded": true,
Review Comment:
`"decoded": true` validates any value — `"decoded": "banana"`, `42`, `{}`
all pass today — so nothing pins the per-type shape that `parse(input) ==
decoded` actually asserts, and a future fixture with a typo'd `precison` or a
`fixed` carrying `len` instead of `length` would ship green. The eventual fix
is `if/then` (or `oneOf`) keyed on `decoded.type` with the required keys per
type from Appendix C (spec.md:1685-1693): `decimal`→`precision`/`scale`,
`fixed`→`length`, `list`→`element-id`/`element-required`/`element`,
`map`→`key-id`/`key`/`value-id`/`value-required`/`value`, `geometry`→`crs`,
`geography`→`crs`/`algorithm`, with `additionalProperties: false` per shape.
I'm fine deferring that — but only if we make the gap explicit rather than
silent. A one-line note in the README (and/or a `$comment` here) saying the
`decoded` shape isn't schema-enforced yet, pointing at a follow-up issue, keeps
the next fixture author from assuming the schema has their back. With that note
in, this isn't a blocker.
##########
dev/check-license:
##########
@@ -0,0 +1,78 @@
+#!/usr/bin/env bash
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+acquire_rat_jar () {
+
+
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
+
+ JAR="$rat_jar"
+
+ # Download rat launch jar if it hasn't been downloaded yet
+ if [ ! -f "$JAR" ]; then
+ # Download
+ printf "Attempting to fetch rat\n"
+ JAR_DL="${JAR}.part"
+ 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"
+ else
+ printf "You do not have curl or wget installed, please install rat
manually.\n"
+ exit 1
Review Comment:
Both `exit 1` calls in `acquire_rat_jar` (this one and the unzip-failure
path at :44) terminate the whole shell, so the caller's `|| { echo "Download
failed. Obtain the rat jar manually..."; exit 1; }` at :62 can never run — the
more helpful message with the exact jar path is dead code. I'd make these
`return 1` so the fallback is reachable.
--
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]
