laskoviymishka commented on code in PR #3: URL: https://github.com/apache/iceberg-verification/pull/3#discussion_r4135189191
########## table-spec/schema/README.md: ########## @@ -0,0 +1,142 @@ +<!-- + ~ 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. + --> + +# Schema decoding + +Reading a schema JSON should produce the same schema in every implementation. +This surface covers the schema document: the field tree and its id spaces, +column order, required flags, `doc`, `schema-id` and `identifier-field-ids`. +Type-string grammar, including the parsing and rejection of parameterized types +such as `decimal(P, S)` and `fixed[L]`, is covered by the types surface under +`table-spec/types/`. Review Comment: This is the small piece I'd keep in this PR. `table-spec/types/` isn't on `main` or this branch yet — it's only tracked in #9 — so pointing at it reads as if malformed-`decimal`/`fixed` rejection is already handled somewhere, when it isn't handled anywhere. Nothing rejects `decimal(abc)` or `fixed[-1]` today. I'd drop the `is covered by the types surface under table-spec/types/` clause entirely and just say type-string grammar (`decimal(P, S)` / `fixed[L]` parsing and rejection) is out of scope for this surface — full stop, without asserting where it's covered. Not pointing anywhere is cleaner than pointing at a path that doesn't exist, and it's what makes deferring the reject cases honest. ########## table-spec/schema/README.md: ########## @@ -0,0 +1,142 @@ +<!-- + ~ 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. + --> + +# Schema decoding + +Reading a schema JSON should produce the same schema in every implementation. +This surface covers the schema document: the field tree and its id spaces, +column order, required flags, `doc`, `schema-id` and `identifier-field-ids`. +Type-string grammar, including the parsing and rejection of parameterized types +such as `decimal(P, S)` and `fixed[L]`, is covered by the types surface under +`table-spec/types/`. + +## Assertion + +``` +project(parse(input)) == decoded +``` + +Bytes are not compared. The spec fixes no key order in a schema JSON, and does +not say whether an absent optional field is written out, so two byte-different +schema JSONs can be the same schema. `decoded` is the comparable form. + +## Scope + +This surface reads the schema JSON object and nothing around it. Writing a schema +back out is out of scope for this surface. Whether a type is legal at a given +format version cannot be decided here, because a schema JSON carries no version. +Type to format-version conformance is out of scope and is best verified at a +higher level. + +## Inputs + +`core/` holds the shape cases: nesting, every id space, column order, `doc`, +`schema-id`, `identifier-field-ids`, and every v1 and v2 primitive type. Every +input in `core/` is valid writer output for format versions 1, 2 and 3. Review Comment: Small one while we're here: this blanket claim collides with the `reject-*` cases right below it — about half of `core/`'s inputs are `valid: false` because they're deliberately malformed. Scoping it to "Every `valid: true` input in `core/` is valid writer output for versions 1, 2 and 3; `valid: false` inputs are deliberately malformed" fixes it. Fine to fold into the follow-up if you'd rather not touch it now. ########## table-spec/schema/unknown/cases.json: ########## @@ -0,0 +1,56 @@ +{ + "cases": [ + { + "id": "unknown-optional", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "id", + "required": true, + "type": "long" + }, + { + "id": 2, + "name": "future", + "required": false, + "type": "unknown" + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "id", "parent": null, "required": true, "type": "long", "doc": null }, + { "id": 2, "name": "future", "parent": null, "required": false, "type": "unknown", "doc": null } + ] + } + }, + { + "id": "reject-unknown-required", Review Comment: `unknown` being optional is position-independent, but this only exercises a top-level field — an impl checking required-ness only at the root would pass while accepting `element-required: true` on an `unknown` list element or a required `unknown` map value. A nested twin (`reject-unknown-required-in-list` / `-in-map-value`) would close it, same way the identifier cases got theirs. Good candidate to bundle into the structural-coverage follow-up. ########## table-spec/schema/core/cases.json: ########## @@ -0,0 +1,836 @@ +{ + "cases": [ + { + "id": "flat-required-and-optional", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "id", + "required": true, + "type": "long" + }, + { + "id": 2, + "name": "data", + "required": false, + "type": "string" + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "id", "parent": null, "required": true, "type": "long", "doc": null }, + { "id": 2, "name": "data", "parent": null, "required": false, "type": "string", "doc": null } + ] + } + }, + { + "id": "fields-not-in-id-order", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 3, + "name": "c", + "required": true, + "type": "int" + }, + { + "id": 1, + "name": "a", + "required": true, + "type": "int" + }, + { + "id": 2, + "name": "b", + "required": false, + "type": "int" + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 3, "name": "c", "parent": null, "required": true, "type": "int", "doc": null }, + { "id": 1, "name": "a", "parent": null, "required": true, "type": "int", "doc": null }, + { "id": 2, "name": "b", "parent": null, "required": false, "type": "int", "doc": null } + ] + } + }, + { + "id": "nested-struct", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "id", + "required": true, + "type": "long" + }, + { + "id": 2, + "name": "location", + "required": false, + "type": { + "type": "struct", + "fields": [ + { + "id": 3, + "name": "lat", + "required": true, + "type": "double" + }, + { + "id": 4, + "name": "lon", + "required": true, + "type": "double" + } + ] + } + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "id", "parent": null, "required": true, "type": "long", "doc": null }, + { "id": 2, "name": "location", "parent": null, "required": false, "type": "struct", "doc": null }, + { "id": 3, "name": "lat", "parent": 2, "required": true, "type": "double", "doc": null }, + { "id": 4, "name": "lon", "parent": 2, "required": true, "type": "double", "doc": null } + ] + } + }, + { + "id": "list-of-primitive", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "id", + "required": true, + "type": "long" + }, + { + "id": 2, + "name": "tags", + "required": true, + "type": { + "type": "list", + "element-id": 3, + "element-required": false, + "element": "string" + } + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "id", "parent": null, "required": true, "type": "long", "doc": null }, + { "id": 2, "name": "tags", "parent": null, "required": true, "type": "list", "doc": null }, + { "id": 3, "name": "element", "parent": 2, "required": false, "type": "string", "doc": null } + ] + } + }, + { + "id": "map-of-primitive", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "id", + "required": true, + "type": "long" + }, + { + "id": 2, + "name": "props", + "required": true, + "type": { + "type": "map", + "key-id": 3, + "key": "string", + "value-id": 4, + "value-required": false, + "value": "int" + } + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "id", "parent": null, "required": true, "type": "long", "doc": null }, + { "id": 2, "name": "props", "parent": null, "required": true, "type": "map", "doc": null }, + { "id": 3, "name": "key", "parent": 2, "required": true, "type": "string", "doc": null }, + { "id": 4, "name": "value", "parent": 2, "required": false, "type": "int", "doc": null } + ] + } + }, + { + "id": "list-of-struct", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "id", + "required": true, + "type": "long" + }, + { + "id": 2, + "name": "points", + "required": true, + "type": { + "type": "list", + "element-id": 3, + "element-required": true, + "element": { + "type": "struct", + "fields": [ + { + "id": 4, + "name": "x", + "required": true, + "type": "int" + }, + { + "id": 5, + "name": "y", + "required": false, + "type": "int" + } + ] + } + } + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "id", "parent": null, "required": true, "type": "long", "doc": null }, + { "id": 2, "name": "points", "parent": null, "required": true, "type": "list", "doc": null }, + { "id": 3, "name": "element", "parent": 2, "required": true, "type": "struct", "doc": null }, + { "id": 4, "name": "x", "parent": 3, "required": true, "type": "int", "doc": null }, + { "id": 5, "name": "y", "parent": 3, "required": false, "type": "int", "doc": null } + ] + } + }, + { + "id": "map-with-struct-value", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "id", + "required": true, + "type": "long" + }, + { + "id": 2, + "name": "by_name", + "required": false, + "type": { + "type": "map", + "key-id": 3, + "key": "string", + "value-id": 4, + "value-required": false, + "value": { + "type": "struct", + "fields": [ + { + "id": 5, + "name": "count", + "required": true, + "type": "long" + } + ] + } + } + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "id", "parent": null, "required": true, "type": "long", "doc": null }, + { "id": 2, "name": "by_name", "parent": null, "required": false, "type": "map", "doc": null }, + { "id": 3, "name": "key", "parent": 2, "required": true, "type": "string", "doc": null }, + { "id": 4, "name": "value", "parent": 2, "required": false, "type": "struct", "doc": null }, + { "id": 5, "name": "count", "parent": 4, "required": true, "type": "long", "doc": null } + ] + } + }, + { + "id": "schema-id-and-identifier-fields", + "valid": true, + "input": { + "type": "struct", + "schema-id": 7, + "identifier-field-ids": [ + 1, + 2 + ], + "fields": [ + { + "id": 1, + "name": "tenant", + "required": true, + "type": "string" + }, + { + "id": 2, + "name": "key", + "required": true, + "type": "long" + }, + { + "id": 3, + "name": "payload", + "required": false, + "type": "string" + } + ] + }, + "decoded": { + "schema-id": 7, + "identifier-field-ids": [1, 2], + "fields": [ + { "id": 1, "name": "tenant", "parent": null, "required": true, "type": "string", "doc": null }, + { "id": 2, "name": "key", "parent": null, "required": true, "type": "long", "doc": null }, + { "id": 3, "name": "payload", "parent": null, "required": false, "type": "string", "doc": null } + ] + } + }, + { + "id": "field-doc", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "id", + "required": true, + "type": "long", + "doc": "primary key" + }, + { + "id": 2, + "name": "data", + "required": false, + "type": "string" + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "id", "parent": null, "required": true, "type": "long", "doc": "primary key" }, + { "id": 2, "name": "data", "parent": null, "required": false, "type": "string", "doc": null } + ] + } + }, + { + "id": "all-primitive-types", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "f_boolean", + "required": false, + "type": "boolean" + }, + { + "id": 2, + "name": "f_int", + "required": false, + "type": "int" + }, + { + "id": 3, + "name": "f_long", + "required": false, + "type": "long" + }, + { + "id": 4, + "name": "f_float", + "required": false, + "type": "float" + }, + { + "id": 5, + "name": "f_double", + "required": false, + "type": "double" + }, + { + "id": 6, + "name": "f_date", + "required": false, + "type": "date" + }, + { + "id": 7, + "name": "f_time", + "required": false, + "type": "time" + }, + { + "id": 8, + "name": "f_timestamp", + "required": false, + "type": "timestamp" + }, + { + "id": 9, + "name": "f_timestamptz", + "required": false, + "type": "timestamptz" + }, + { + "id": 10, + "name": "f_string", + "required": false, + "type": "string" + }, + { + "id": 11, + "name": "f_uuid", + "required": false, + "type": "uuid" + }, + { + "id": 12, + "name": "f_fixed", + "required": false, + "type": "fixed[16]" + }, + { + "id": 13, + "name": "f_binary", + "required": false, + "type": "binary" + }, + { + "id": 14, + "name": "f_decimal", + "required": false, + "type": "decimal(9, 2)" + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "f_boolean", "parent": null, "required": false, "type": "boolean", "doc": null }, + { "id": 2, "name": "f_int", "parent": null, "required": false, "type": "int", "doc": null }, + { "id": 3, "name": "f_long", "parent": null, "required": false, "type": "long", "doc": null }, + { "id": 4, "name": "f_float", "parent": null, "required": false, "type": "float", "doc": null }, + { "id": 5, "name": "f_double", "parent": null, "required": false, "type": "double", "doc": null }, + { "id": 6, "name": "f_date", "parent": null, "required": false, "type": "date", "doc": null }, + { "id": 7, "name": "f_time", "parent": null, "required": false, "type": "time", "doc": null }, + { "id": 8, "name": "f_timestamp", "parent": null, "required": false, "type": "timestamp", "doc": null }, + { "id": 9, "name": "f_timestamptz", "parent": null, "required": false, "type": "timestamptz", "doc": null }, + { "id": 10, "name": "f_string", "parent": null, "required": false, "type": "string", "doc": null }, + { "id": 11, "name": "f_uuid", "parent": null, "required": false, "type": "uuid", "doc": null }, + { "id": 12, "name": "f_fixed", "parent": null, "required": false, "type": { "type": "fixed", "length": 16 }, "doc": null }, + { "id": 13, "name": "f_binary", "parent": null, "required": false, "type": "binary", "doc": null }, + { "id": 14, "name": "f_decimal", "parent": null, "required": false, "type": { "type": "decimal", "precision": 9, "scale": 2 }, "doc": null } + ] + } + }, + { + "id": "decimal-max-precision", + "valid": true, + "input": { + "type": "struct", + "schema-id": 0, + "fields": [ + { + "id": 1, + "name": "id", + "required": true, + "type": "long" + }, + { + "id": 2, + "name": "amount", + "required": false, + "type": "decimal(38, 10)" + } + ] + }, + "decoded": { + "schema-id": 0, + "identifier-field-ids": [], + "fields": [ + { "id": 1, "name": "id", "parent": null, "required": true, "type": "long", "doc": null }, + { "id": 2, "name": "amount", "parent": null, "required": false, "type": { "type": "decimal", "precision": 38, "scale": 10 }, "doc": null } + ] + } + }, + { + "id": "reject-identifier-field-does-not-exist", + "valid": false, Review Comment: All ten `valid: false` cases here are identifier-field-ids violations, so five of the six things the README lists as in-scope — duplicate field id, id at/above the `Integer.MAX_VALUE - 200` cap, non-struct root, missing required key/name/type, malformed `schema-id` — have no rejection case yet. Since this is the shape later surfaces copy, I'd want that coverage to exist eventually. Not blocking this PR though — fine to land the positive cases now and add the structural rejects under a tracked issue. The only condition I'd put on the deferral is the README tweak below, so the doc doesn't claim coverage we're punting on. Happy to file the follow-up if you'd like. -- 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]
