sungwy commented on code in PR #2: URL: https://github.com/apache/iceberg-verification/pull/2#discussion_r3884394388
########## CONTRIBUTING.md: ########## @@ -0,0 +1,215 @@ +<!-- + ~ 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. + --> + +# Contributing + +First, thank you for contributing to iceberg-verification! The goal of this +document is to provide the guidelines you need to maintain a high quality +verification framework for all Iceberg implementations. + +[Iceberg](https://iceberg.apache.org/) is first and foremost a Spec, an +agreement between different implementations on the structure and meaning of the +artifacts that make up a table. And as such, it is crucial that the way each +implementation interprets the spec, and the values and artifacts it stores and +transfers as a result, stay consistent across implementations. Each +implementation's own test suite cannot catch a divergence here, because it +checks that implementation against its own reading of the spec. + +This framework aims to become a central place where the materialized value +expectations of the spec are discussed, so that their representations and +expectations can be used to verify implementations across languages and +platforms. + +One thing to be clear about up front: we surface ambiguity here, we do not +settle it here. When a case has no clear answer in the spec, take it to +[[email protected]](https://lists.apache.org/[email protected]) +first, and commit the fixture once the community has agreed on the answer. + +## Does your change belong here? + +This repository holds artifacts and the expected values the spec fixes for +them. It holds no implementation code, and it verifies no behavior. + +If you have found a bug in one implementation, that belongs in that +implementation's repository. Open a fixture pull request here when the spec +fixes an expected value and an implementation disagrees with it. + +Engine query results, live catalog protocol behavior, and physical encoding +choices such as compression codec and file format writer version are out of +scope. Any spec-valid encoding is valid. + +## Choosing a test surface + +Given the above goal, it is important that contributors think deeply about what +we are trying to verify when we introduce a new test surface. + +The repository consists of many test surfaces, often sub-grouped by versioned +directories (`table-spec/v1`, `table-spec/v2`). Often, test surfaces map to +specs. Each surface directory holds its input artifacts, the expected values +co-located with them, and a `README.md` defining the assertion a consumer +implements. + +Specs can have dependencies on other Iceberg specs. For example, Table Spec has +a dependency on Iceberg Type Spec. In such cases, we recommend creating test +surfaces for both layers at the root level, and focusing on the coverage that +pertains to each level. + +If there are edge case value concerns with the type spec, or we want to verify +the field structure of a type spec (e.g. Geography Type), we want to include +that coverage in the surface for the Iceberg Type Spec. Geography Type support +in a Table V3 spec can be added as a separate test at the Table Spec test +surface, but the scope of testing will be different. In the Type Spec surface, +we test edge cases in the type representation. In the Table Spec test, we check +that V3 supports a representation of Geography Type. + +Notice that this is not all that different from how we reason through regular +test suites. Keep this model in mind when you work on introducing a new test +surface. + +## What should my test fixtures and expectations look like? + +There is no single answer here, and that is deliberate. Each combination of +input fixture and expectation differs by the surface under test, because what +the spec fixes differs by surface. The spec fixes the canonical form of a type +string. It pins no such form for `metadata.json`, so two byte-different files +can represent the same table metadata, and only the decoded values are +comparable. + +It is the contributor's responsibility to understand the spec, work out the +specification that needs to hold true, and design the input and the assertions +that define correctness of the materialized values for that spec. Before you +write a fixture, you should be able to answer: + +1. Which clause of which spec fixes the correct answer here? Link it. +2. What is the input, and what is the smallest complete statement of the + expected result? +3. What must an implementation do to check it, in one sentence? That sentence + belongs in the surface `README.md`, and it is the contract consumers + implement. +4. Which boundary cases does the spec decide, and which does it leave open? + +That said, two things hold no matter which surface you are working on. + +### Bytes or decoded values? + +Where the spec fixes a canonical form, as it does for type strings, we can +assert that re-serializing the parsed input reproduces that form exactly. +Everywhere else, we compare the decoded logical values. + +It is worth thinking carefully about this, because the stricter option may not +be the most suitable assertion. If we compare a `metadata.json` or an Avro +manifest against an expected form byte for byte, we end up freezing one +implementation's key ordering, and its choices about optional fields and +codecs, as if the spec had required them. Other implementations would then fail +our fixture for reasons the spec does not care about, and that is not what we +want to achieve with the verification framework. + +When we compare decoded values, we still need to be careful about what we +normalize away. Where the spec fixes a logical value but allows more than one +encoding, we normalize to the value. For example, a `day` transform result is a +`date`, and the spec requires readers to accept a plain `int` as the same date, +so the two should compare equal +([iceberg#16414](https://github.com/apache/iceberg/issues/16414)). Where the +spec pins a physical type, we keep it. `equality_ids` elements are `int`, not +`long`, so a decode that reads both as `1` would miss the defect +([iceberg-go#880](https://github.com/apache/iceberg-go/pull/880)). + +So the only things we assert are the ones the spec pins. Where the spec allows +more than one encoding of the same logical value, we normalize and there is no +difference left to fail on. Where the spec has not decided the answer at all, +see the next section. + +### Cases the spec has not settled + +Often the most interesting case is one where the spec does not yet give a clear +answer, and implementations have landed in different places. For example, the +spec caps decimal precision at 38, but whether a reader must reject +`decimal(40, 2)` is still being worked out in +[iceberg#16798](https://github.com/apache/iceberg/pull/16798). + +We do not want to leave these out, and we do not want to invent an answer for +them either. Instead we commit the case as an open case. The input is pinned, +no expected value is committed, and the case carries a link to where the +question is being decided. Capturing it this way is what makes the boundary +concrete, so that implementations can show where they currently land and the +discussion has something specific to point at. + +How the open state is written is up to the surface, but every surface must make +it possible for a consumer to tell an open case from an assertable one without +reading the fixture by hand. + +An open case never fails a consumer. Running one and recording what your +implementation does is optional, and it is the most useful thing you can bring +back to the discussion. + +Once the community settles the question, the case gains its expected value and +stops being open. Call that out in the pull request, because a consumer bumping +its pin may start failing a case that used to pass. + +### Keep expected values unambiguous across implementations + +Your fixture will be read by Go, Python, Rust, Java, C++ consumers and other +Iceberg implementations that may not be managed by the Apache Iceberg +community, so the expected value itself must not be ambiguous to read. Say in +your surface `README.md` how your expected files are written. + +Before inventing a convention, look at how existing surfaces handle the same Review Comment: I think this is a good suggestion. I've added a subsection for guidelines when using JSON as the expected value format ########## CONTRIBUTING.md: ########## @@ -0,0 +1,215 @@ +<!-- + ~ 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. + --> + +# Contributing + +First, thank you for contributing to iceberg-verification! The goal of this +document is to provide the guidelines you need to maintain a high quality +verification framework for all Iceberg implementations. + +[Iceberg](https://iceberg.apache.org/) is first and foremost a Spec, an +agreement between different implementations on the structure and meaning of the +artifacts that make up a table. And as such, it is crucial that the way each +implementation interprets the spec, and the values and artifacts it stores and +transfers as a result, stay consistent across implementations. Each +implementation's own test suite cannot catch a divergence here, because it +checks that implementation against its own reading of the spec. + +This framework aims to become a central place where the materialized value +expectations of the spec are discussed, so that their representations and +expectations can be used to verify implementations across languages and +platforms. + +One thing to be clear about up front: we surface ambiguity here, we do not +settle it here. When a case has no clear answer in the spec, take it to +[[email protected]](https://lists.apache.org/[email protected]) +first, and commit the fixture once the community has agreed on the answer. + +## Does your change belong here? + +This repository holds artifacts and the expected values the spec fixes for +them. It holds no implementation code, and it verifies no behavior. + +If you have found a bug in one implementation, that belongs in that +implementation's repository. Open a fixture pull request here when the spec +fixes an expected value and an implementation disagrees with it. + +Engine query results, live catalog protocol behavior, and physical encoding +choices such as compression codec and file format writer version are out of +scope. Any spec-valid encoding is valid. + +## Choosing a test surface + +Given the above goal, it is important that contributors think deeply about what +we are trying to verify when we introduce a new test surface. + +The repository consists of many test surfaces, often sub-grouped by versioned +directories (`table-spec/v1`, `table-spec/v2`). Often, test surfaces map to Review Comment: Yes, that'll be nice :) ########## CONTRIBUTING.md: ########## @@ -0,0 +1,215 @@ +<!-- + ~ 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. + --> + +# Contributing + +First, thank you for contributing to iceberg-verification! The goal of this +document is to provide the guidelines you need to maintain a high quality Review Comment: Yeah I thought it would be good to be forward looking, given that the community wanted to pull other forms of verification into the scope. But I agree that this is first and foremost a fixture repository and the suggested forms of verification (producer verification, reader verification) are tools that sit alongside it. I've made some edits, feel free to take another pass -- 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]
