laskoviymishka commented on code in PR #16: URL: https://github.com/apache/iceberg-verification/pull/16#discussion_r4243161700
########## runners/java/src/main/java/org/apache/iceberg/conformance/VerifyTypes.java: ########## @@ -0,0 +1,448 @@ +/* + * 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. + */ + +package org.apache.iceberg.conformance; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Locale; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.iceberg.SchemaParser; +import org.apache.iceberg.types.Type; +import org.apache.iceberg.types.Types; + +/** + * Checks Apache Iceberg (Java) against the type-surface fixtures. Parses each case {@code input} + * with {@link Types#fromTypeName(String)} (string types) or {@link SchemaParser#fromJson(String)} + * (nested types) and grades it per the shared contract in {@code runners/README.md}. Exits 0 when + * no case fails, 1 on any fail, 2 on a setup error. + */ +public final class VerifyTypes { + + private static final String[] SURFACE_ROOTS = {"table-spec/types"}; + + private VerifyTypes() {} + + public static void main(String[] args) { + Path start = Paths.get(System.getProperty("user.dir")); + List<String> surfaces = new ArrayList<>(); + for (int i = 0; i < args.length; i++) { + String a = args[i]; + if (a.equals("--surface")) { + if (i + 1 >= args.length) { + System.err.println("Setup error: --surface requires a value"); + System.exit(2); + } + surfaces.add(args[++i]); + } else if (a.startsWith("--surface=")) { + surfaces.add(a.substring("--surface=".length())); + } else { + start = Paths.get(a); + } + } + String env = System.getenv("CONFORMANCE_SURFACES"); + if (env != null) { + for (String s : env.split("[,\\s]+")) { + if (!s.isEmpty()) { + surfaces.add(s); + } + } + } + Path root = findRepoRoot(start.toAbsolutePath()); + if (root == null) { + System.err.println("Could not locate repo root (no table-spec/) above " + start); + System.exit(2); + } + + List<JsonNode> cases; + try { + cases = loadCases(root, surfaces); + } catch (IOException e) { + // A fixture-read failure is a setup error (exit 2), not a conformance FAIL. + System.err.println("Setup error reading fixtures: " + e.getMessage()); + System.exit(2); + return; + } + if (cases.isEmpty()) { + System.err.println("No cases found under " + root); + System.exit(2); + } + + int pass = 0; + int fail = 0; + int advisoryFail = 0; + int skip = 0; + List<String> failIds = new ArrayList<>(); + List<String> advisoryIds = new ArrayList<>(); + List<String> skipIds = new ArrayList<>(); + + for (JsonNode node : cases) { + String id = node.get("id").asText(); + String[] graded = grade(node); + String status = graded[0]; + String detail = graded[1]; + + switch (status) { + case "pass": + pass++; + break; + case "advisory_fail": + advisoryFail++; + advisoryIds.add(id); + break; + case "skip": + skip++; + skipIds.add(id); + break; + default: + fail++; + failIds.add(id); + break; + } + if (detail.isEmpty()) { + System.out.printf(Locale.ROOT, "%-32s %s%n", id, status); + } else { + System.out.printf(Locale.ROOT, "%-32s %s (%s)%n", id, status, detail); + } + } + + System.out.printf( + Locale.ROOT, + "%nTOTALS: %d cases | pass=%d fail=%d advisory_fail=%d skip=%d%n", + cases.size(), + pass, + fail, + advisoryFail, + skip); + if (!failIds.isEmpty()) { + System.out.println("FAIL ids: " + failIds); + } + if (!advisoryIds.isEmpty()) { + System.out.println("ADVISORY_FAIL ids: " + advisoryIds); + } + if (!skipIds.isEmpty()) { + System.out.println("SKIP ids: " + skipIds); + } + if (fail > 0) { + System.exit(1); + } + } + + /** Grades one case, returning {@code {status, detail}}. */ + static String[] grade(JsonNode node) { + String id = node.get("id").asText(); + JsonNode input = node.get("input"); + boolean valid = node.get("valid").asBoolean(); + + Type parsed = null; + RuntimeException parseError = null; + try { + parsed = parseType(input); + } catch (RuntimeException e) { + parseError = e; + } + + String status; + String detail; + + if (!valid) { + if (parseError != null) { + status = "pass"; + detail = "rejected: " + parseError.getMessage(); + } else { + status = failState(node); + detail = "expected reject, parsed as \"" + parsed + "\""; + } + } else if (parseError != null) { + if (!isModeled(input)) { + status = "skip"; + detail = "not modeled: " + parseError.getMessage(); + } else { + status = failState(node); + detail = + "expected accept, parse threw " + + parseError.getClass().getSimpleName() + + ": " + + parseError.getMessage(); + } + } else { + JsonNode decoded = node.get("decoded"); + if (decoded == null) { + System.err.println("Setup error: valid case " + id + " has no decoded shape"); + System.exit(2); + } + String mismatch = decodedMismatch(parsed, decoded); + if (mismatch != null) { + status = failState(node); + detail = + "decoded mismatch: expected " + + decoded + + ", actual " + + describe(parsed) + + " [" + + mismatch + + "]"; + } else if (node.hasNonNull("canonical") + && !parsed.toString().equals(node.get("canonical").asText())) { + // The case pins a canonical spelling: the re-serialized type must reproduce it. + status = failState(node); + detail = + "canonical mismatch: expected \"" + + node.get("canonical").asText() + + "\", actual \"" + + parsed + + "\""; + } else { + status = "pass"; + detail = ""; + } + } + return new String[] {status, detail}; + } + + static String failState(JsonNode node) { + return node.hasNonNull("normative_level") + && "should".equals(node.get("normative_level").asText()) + ? "advisory_fail" + : "fail"; + } + + private static final Set<String> EXACT_KEYWORDS = Review Comment: Worth getting right wherever this lands: `skip` here means "the input string isn't in the runner's allowlist", not "the implementation doesn't model this type". So against the `1.11.0` floor every v3 case (`variant`, `geometry`, `timestamp_ns`) hard-fails instead of skipping, while a type that's missing from the list silently skips and exits 0. The prefix arm also matches `fixedfoo` / `decimalx`. This is really the per-consumer skip list the proposal describes, so I'd make it an explicit, reviewed list that lives with the consumer, plus a guard so an all-skip run can't go green. ########## README.md: ########## @@ -54,6 +54,11 @@ repository to a commit, runs the surfaces it opts into, and bumps that pin deliberately. There is no central conformance gate and no pass/fail matrix across implementations. +## Runners Review Comment: I'd drop this section. Runners hosted here can only test published artifacts, so conformance never gates the Iceberg commit that breaks it. Per the proposal, each implementation pins this repo and runs the fixtures in its own CI, so I'd move the Java runner into apache/iceberg as a regular test. ########## runners/java/src/main/java/org/apache/iceberg/conformance/VerifyTypes.java: ########## @@ -0,0 +1,448 @@ +/* + * 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. + */ + +package org.apache.iceberg.conformance; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Locale; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.iceberg.SchemaParser; +import org.apache.iceberg.types.Type; +import org.apache.iceberg.types.Types; + +/** + * Checks Apache Iceberg (Java) against the type-surface fixtures. Parses each case {@code input} + * with {@link Types#fromTypeName(String)} (string types) or {@link SchemaParser#fromJson(String)} + * (nested types) and grades it per the shared contract in {@code runners/README.md}. Exits 0 when + * no case fails, 1 on any fail, 2 on a setup error. + */ +public final class VerifyTypes { + + private static final String[] SURFACE_ROOTS = {"table-spec/types"}; + + private VerifyTypes() {} + + public static void main(String[] args) { + Path start = Paths.get(System.getProperty("user.dir")); + List<String> surfaces = new ArrayList<>(); + for (int i = 0; i < args.length; i++) { + String a = args[i]; + if (a.equals("--surface")) { + if (i + 1 >= args.length) { + System.err.println("Setup error: --surface requires a value"); + System.exit(2); + } + surfaces.add(args[++i]); + } else if (a.startsWith("--surface=")) { + surfaces.add(a.substring("--surface=".length())); + } else { + start = Paths.get(a); + } + } + String env = System.getenv("CONFORMANCE_SURFACES"); + if (env != null) { + for (String s : env.split("[,\\s]+")) { + if (!s.isEmpty()) { + surfaces.add(s); + } + } + } + Path root = findRepoRoot(start.toAbsolutePath()); + if (root == null) { + System.err.println("Could not locate repo root (no table-spec/) above " + start); + System.exit(2); + } + + List<JsonNode> cases; + try { + cases = loadCases(root, surfaces); + } catch (IOException e) { + // A fixture-read failure is a setup error (exit 2), not a conformance FAIL. + System.err.println("Setup error reading fixtures: " + e.getMessage()); + System.exit(2); + return; + } + if (cases.isEmpty()) { + System.err.println("No cases found under " + root); + System.exit(2); + } + + int pass = 0; + int fail = 0; + int advisoryFail = 0; + int skip = 0; + List<String> failIds = new ArrayList<>(); + List<String> advisoryIds = new ArrayList<>(); + List<String> skipIds = new ArrayList<>(); + + for (JsonNode node : cases) { + String id = node.get("id").asText(); + String[] graded = grade(node); + String status = graded[0]; + String detail = graded[1]; + + switch (status) { + case "pass": + pass++; + break; + case "advisory_fail": + advisoryFail++; + advisoryIds.add(id); + break; + case "skip": + skip++; + skipIds.add(id); + break; + default: + fail++; + failIds.add(id); + break; + } + if (detail.isEmpty()) { + System.out.printf(Locale.ROOT, "%-32s %s%n", id, status); + } else { + System.out.printf(Locale.ROOT, "%-32s %s (%s)%n", id, status, detail); + } + } + + System.out.printf( + Locale.ROOT, + "%nTOTALS: %d cases | pass=%d fail=%d advisory_fail=%d skip=%d%n", + cases.size(), + pass, + fail, + advisoryFail, + skip); + if (!failIds.isEmpty()) { + System.out.println("FAIL ids: " + failIds); + } + if (!advisoryIds.isEmpty()) { + System.out.println("ADVISORY_FAIL ids: " + advisoryIds); + } + if (!skipIds.isEmpty()) { + System.out.println("SKIP ids: " + skipIds); + } + if (fail > 0) { + System.exit(1); + } + } + + /** Grades one case, returning {@code {status, detail}}. */ + static String[] grade(JsonNode node) { + String id = node.get("id").asText(); + JsonNode input = node.get("input"); + boolean valid = node.get("valid").asBoolean(); + + Type parsed = null; + RuntimeException parseError = null; + try { + parsed = parseType(input); + } catch (RuntimeException e) { + parseError = e; + } + + String status; + String detail; + + if (!valid) { + if (parseError != null) { + status = "pass"; + detail = "rejected: " + parseError.getMessage(); + } else { + status = failState(node); + detail = "expected reject, parsed as \"" + parsed + "\""; + } + } else if (parseError != null) { + if (!isModeled(input)) { + status = "skip"; + detail = "not modeled: " + parseError.getMessage(); + } else { + status = failState(node); + detail = + "expected accept, parse threw " + + parseError.getClass().getSimpleName() + + ": " + + parseError.getMessage(); + } + } else { + JsonNode decoded = node.get("decoded"); + if (decoded == null) { + System.err.println("Setup error: valid case " + id + " has no decoded shape"); + System.exit(2); + } + String mismatch = decodedMismatch(parsed, decoded); + if (mismatch != null) { + status = failState(node); + detail = + "decoded mismatch: expected " + + decoded + + ", actual " + + describe(parsed) + + " [" + + mismatch + + "]"; + } else if (node.hasNonNull("canonical") + && !parsed.toString().equals(node.get("canonical").asText())) { Review Comment: The contract says the parsed type is re-serialized and must match `canonical` byte for byte, but this compares `parsed.toString()`, which isn't the wire encoding — for nested types it's `struct<1: a: required int>`, not Appendix-C JSON, so nested canonical cases false-fail. I'd go through `SchemaParser.toJson` so the write path is actually exercised, wherever the grader ends up. -- 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]
