laskoviymishka commented on code in PR #9:
URL:
https://github.com/apache/iceberg-verification/pull/9#discussion_r4063641530
##########
table-spec/types/geospatial/cases.json:
##########
@@ -0,0 +1,92 @@
+{
+ "cases": [
+ {
+ "id": "geometry-crs84",
+ "valid": true,
+ "input": "geometry(OGC:CRS84)",
+ "decoded": {"type": "geometry", "crs": "OGC:CRS84"},
+ "canonical": "geometry(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"},
+ "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,
+ "input": "geometry",
+ "decoded": {"type": "geometry", "crs": "OGC:CRS84"},
+ "canonical": "geometry(OGC:CRS84)",
Review 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.
##########
table-spec/types/geospatial/cases.json:
##########
@@ -0,0 +1,92 @@
+{
+ "cases": [
+ {
+ "id": "geometry-crs84",
+ "valid": true,
+ "input": "geometry(OGC:CRS84)",
+ "decoded": {"type": "geometry", "crs": "OGC:CRS84"},
+ "canonical": "geometry(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"},
+ "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,
+ "input": "geometry",
+ "decoded": {"type": "geometry", "crs": "OGC:CRS84"},
+ "canonical": "geometry(OGC:CRS84)",
+ "clause": "geometry(C): if C is not specified, C is OGC:CRS84; the
canonical serialized form is fully parameterized (Appendix C)",
+ "spec_ref": "format/spec.md#appendix-c-json-serialization"
+ },
+ {
+ "id": "geography-crs84-spherical",
+ "valid": true,
+ "input": "geography(OGC:CRS84, spherical)",
Review 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:
```json
{
"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?
##########
table-spec/types/README.md:
##########
@@ -0,0 +1,116 @@
+<!--
+ ~ 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.
+ -->
+
+# Type decoding
+
+Parsing a type string produces the same type in every implementation. This
+surface pins each type the spec defines and the parse rules that attach to it.
+
+## Assertion
+
+```
+parse(input) == decoded
+```
+
+`input` is a type string, or a JSON object for a nested type; `decoded` is the
+language-neutral shape below. Bytes are not compared - each implementation maps
+its own type object to `decoded`, so the comparison does not depend on one
+language's representation.
+
+- `valid: true` - the parser succeeds and the decoded type equals `decoded`. A
+ type an implementation does not model is UNSUPPORTED, not a failure. If the
case
+ also carries `canonical`, re-serializing the parsed type must equal it byte
for
+ byte (the write direction).
+- `valid: false` - the parser must reject `input`. A rejection passes; a
+ 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
Review 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.
##########
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,
+ "canonical": { "type": "string" },
+ "clause": { "type": "string" },
+ "spec_ref": { "type": "string" },
+ "normative_level": { "enum": ["must", "should"], "description": "must
(default) or should. A should case is advisory (the spec only recommends the
behavior); a runner must report a failed should distinctly from a pass, so the
advisory tier stays visible rather than reading as inert." }
+ },
+ "allOf": [
+ {
+ "$comment": "a valid case must carry decoded",
+ "if": { "properties": { "valid": { "const": true } }, "required":
["valid"] },
+ "then": { "required": ["decoded"] }
+ },
+ {
+ "$comment": "an invalid case must not carry decoded",
+ "if": { "properties": { "valid": { "const": false } }, "required":
["valid"] },
+ "then": { "not": { "required": ["decoded"] } }
Review 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.
##########
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"
Review 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`.
##########
.github/workflows/validate-fixtures.yml:
##########
@@ -0,0 +1,58 @@
+# 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.
+
+name: "Validate Fixtures"
+
+on:
+ push:
+ branches:
+ - main
+ pull_request:
+
+concurrency:
+ group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{
github.workflow }}
+ cancel-in-progress: ${{ github.event_name == 'pull_request' }}
+
+permissions:
+ contents: read
+
+jobs:
+ validate:
+ name: Validate fixture cases
+ runs-on: ubuntu-24.04
+ steps:
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+ with:
+ persist-credentials: false
+ - name: Install Python
+ uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 #
v6.3.0
+ with:
+ python-version: '3.12'
+ - name: Install validator deps
+ run: python3 -m pip install -r dev/requirements.txt
+ - name: Validate cases.json
+ run: python3 dev/validate-fixtures.py
+ - name: Validator rejects a malformed fixture (self-test)
+ run: |
Review 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.
--
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]
