moomindani commented on PR #3: URL: https://github.com/apache/iceberg-verification/pull/3#issuecomment-5736886601
Ran this PR's 25 cases against PyIceberg `main` (`0d584073`) from the consumer side. Results first, then one suggestion. **`core/` is sound as written.** All 11 valid cases decode exactly as `decoded` says — nesting, the `element`/`key`/`value` rows, out-of-order fields, `doc`, `all-primitive-types`, `decimal-max-precision` — and all 6 `reject-identifier-*` cases raise. Nothing to report there. **Both explicit-CRS cases fail PyIceberg**, for the same reason as the geospatial cases in #9: `geometry(srid:4326)` and `geography(srid:4326, vincenty)` are rejected because `GEOMETRY_REGEX` / `GEOGRAPHY_REGEX` require a quoted CRS, while Appendix C's examples are unquoted (apache/iceberg-python#3530 is the fix). These are Appendix C's own example strings, so the cases are right and the implementation is wrong — and two independent surfaces now catch the one bug. **`variant/` cannot run against PyIceberg at all**: there is no variant type in `pyiceberg/types.py`, so both cases fail at `Unsupported field type: 'variant'`. That is the per-subdirectory subscription in your README working as intended — PyIceberg opts into everything except `variant/` until it has one. **Suggestion: cover `initial-default` and `write-default`.** No input here carries either, and the `decoded` row has no slot for them, so this surface cannot catch a divergence in default parsing. Two reasons I think it belongs here: - `initial-default` decides what a reader returns for a column a data file does not contain, so a divergence is a wrong query result rather than a metadata nit. - `format/spec.md:335` is a rule with a built-in rejection case: "All columns of `unknown`, `variant`, `geometry`, and `geography` types must default to null. Non-null values for `initial-default` or `write-default` are invalid." Nothing verifies it today. PyIceberg does reject a non-null default on those types, but incidentally rather than by that rule — `unknown` fails with `Cannot deserialize bytes, type unknown not supported` and `geometry` with `Geometry JSON deserialization requires WKT to WKB conversion, which is not yet implemented`. A fixture would pin the rule where implementations currently reach the right answer by accident. The rejection cases fit the current format as is; the valid case would need a slot for the two keys in the `decoded` row, which is a change to this surface's contract rather than something to slip in. Happy to write either or both, here or as a follow-up, if you want them. -- 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]
