> As a side note, the `rest-catalog-open-api.yaml` file [2] in the Iceberg repo > contains the latest version of the spec.
I think more clarity on this would be helpful. Is it really the case that the Open API spec contains the latest version of the spec? For example, I'm noticing a discrepancy between sequence-number in the Table Spec and in the Open API Spec... In the table spec, it's required for V2, but it's optional in the REST API Spec: https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L2319-L2335 On 2024/10/17 16:58:17 Kevin Liu wrote: > > Based on the example metadata, that looks like it is not to spec, so it's > reasonable that python would reject it. If the java implementation is > allowing for that, it's likely that we're being too relaxed (possibly a > holdover from v1 parsing). > I believe the Java implementation is relaxing the constraint. I'll create a > PR with test cases and the necessary changes. > > > Do you know what produced the metadata? > It was created by Snowflake [1]. After verifying this, I'll look into > raising the issue with them. > > As a side note, the `rest-catalog-open-api.yaml` file [2] in the Iceberg > repo contains the latest version of the spec. As we're continuing to evolve > to spec for V3, would it be helpful to create a frozen version representing > both the V1 and V2 specs for reference, possibly as a separate file? > > Best, > Kevin Liu > > [1] > https://github.com/apache/iceberg-python/issues/1106#issuecomment-2312108455 > [2] > https://github.com/apache/iceberg/blob/8e2eb9ac2e33ce4bac8956d4e2f099444d03c0e3/open-api/rest-catalog-open-api.yaml > > On Thu, Oct 17, 2024 at 9:20 AM Daniel Weeks <dwe...@apache.org> wrote: > > > Sung, > > > > I was thinking of v1, so you're right that manifest-list and summary are > > required as of v2. The REST Spec seems to follow the v2 definition, so I > > think we're somewhat implicitly requiring those fields via REST. > > > > Kevin, > > > > Based on the example metadata, that looks like it is not to spec, so it's > > reasonable that python would reject it. If the java implementation is > > allowing for that, it's likely that we're being too relaxed (possibly a > > holdover from v1 parsing). > > > > Do you know what produced the metadata? > > > > -Dan > > > > On Thu, Oct 17, 2024 at 9:02 AM Kevin Liu <kevin.jq....@gmail.com> wrote: > > > >> Thanks for the additional context. > >> > >> My understanding is that if a Snapshot has a `summary` field, it must > >> also have a corresponding `operation` key in the summary map. Is that > >> correct? Based on the `SnapshotParser`, this is not enforced [1]. > >> > >> The underlying issue in #1106 [2] is the missing `operation` field when > >> the `summary` field is present. > >> For example, > >> ``` > >> "summary" : { > >> "manifests-created" : "8", > >> "total-records" : "26508666891", > >> "added-files-size" : "3927895626752", > >> "manifests-kept" : "0", > >> "total-files-size" : "3927895626752", > >> "added-records" : "26508666891", > >> "added-data-files" : "231513", > >> "manifests-replaced" : "0", > >> "total-data-files" : "231513" > >> } > >> ``` > >> > >> It could be the case that this particular `metadata.json` was generated > >> not according to the spec. > >> > >> Best, > >> Kevin Liu > >> > >> > >> [1] > >> https://github.com/apache/iceberg/blob/17f1c4d2205b59c2bd877d4d31bbbef9e90979c5/core/src/main/java/org/apache/iceberg/SnapshotParser.java#L124-L142 > >> [2] https://github.com/apache/iceberg-python/issues/1106 > >> > >> > >> On Thu, Oct 17, 2024 at 8:47 AM Sung Yun <sun...@apache.org> wrote: > >> > >>> Thank you for the clarification Daniel, and thank you Kevin for raising > >>> this issue! > >>> > >>> Does that mean that we are creating component schemas that are the > >>> superset of the V1 and V2 schemas? And if so, should we remove summary and > >>> manifest-list from the required properties, and add manifests optional > >>> property to the Snapshot schema to support both V1 and V2 Summary specs? > >>> https://iceberg.apache.org/spec/#snapshots > >>> > >>> Or would creating separate component schemas for V1/V2 be a cleaner way > >>> to align the REST spec with the table spec? > >>> > >>> Sung > >>> > >>> On 2024/10/17 15:19:23 Daniel Weeks wrote: > >>> > I'm not convinced this is incorrect behavior (table spec or > >>> > implementation), but it does lend to some confusion. The 'summary' > >>> field > >>> > is optional, which means that if a summary is not provided, you do not > >>> have > >>> > an associated 'operation' field. The 'operation' field is only > >>> required in > >>> > the context of the summary, so it's actually possible for the > >>> > implementation (i.e. the tests you reference) to not have an operation. > >>> > > >>> > I think what is wrong here is that the REST spec marked the summary as > >>> > required > >>> > < > >>> https://github.com/apache/iceberg/blob/8e2eb9ac2e33ce4bac8956d4e2f099444d03c0e3/open-api/rest-catalog-open-api.yaml#L2040 > >>> >, > >>> > which is inconsistent with the table spec. > >>> > > >>> > On Wed, Oct 16, 2024 at 3:52 PM Anton Okolnychyi < > >>> aokolnyc...@gmail.com> > >>> > wrote: > >>> > > >>> > > Based on [1], we never persisted the operation in the summary map. > >>> > > Instead, we persisted it as a top-level field in Java, which is > >>> actually > >>> > > NOT what the spec says. Does anyone remember cases when the > >>> operation was > >>> > > unknown? I personally don't. > >>> > > > >>> > > [1] - > >>> > > > >>> https://github.com/apache/iceberg/blob/17f1c4d2205b59c2bd877d4d31bbbef9e90979c5/core/src/main/java/org/apache/iceberg/SnapshotParser.java#L63 > >>> > > > >>> > > > >>> > > ср, 16 жовт. 2024 р. о 12:42 Kevin Liu <kevin.jq....@gmail.com> > >>> пише: > >>> > > > >>> > >> Hey folks, > >>> > >> > >>> > >> I’ve noticed a discrepancy between the Iceberg specification and > >>> the Java > >>> > >> implementation regarding the `operation` key in the `Snapshot` > >>> `summary` > >>> > >> field. > >>> > >> > >>> > >> The `Snapshot` object's `summary` dictionary includes a *required* > >>> key > >>> > >> named `operation`, as outlined in the spec describing Table > >>> Metadata and > >>> > >> Snapshots [1] and the generated OpenAPI YAML [2]. However, in the > >>> Java > >>> > >> implementation [3], `operation` is treated as optional. In > >>> contrast, it > >>> > >> remains a required field in the Python implementation [4]. > >>> > >> I also found that Java tests for `SnapshotParser` assert that the > >>> > >> `operation` field is null. [5] > >>> > >> > >>> > >> Due to this discrepancy, a user reported [6] that the > >>> `metadata.json` > >>> > >> file generated for an Iceberg table could not be read by PyIceberg, > >>> though > >>> > >> it is readable using the Iceberg Java library. > >>> > >> > >>> > >> How should we proceed from here? Should the Java library enforce > >>> this > >>> > >> requirement? Additionally, how should we handle existing > >>> `metadata.json` > >>> > >> files that were generated without this field? > >>> > >> > >>> > >> Best, > >>> > >> Kevin Liu > >>> > >> > >>> > >> [1] https://iceberg.apache.org/spec/#table-metadata-and-snapshots > >>> > >> [2] > >>> > >> > >>> https://github.com/apache/iceberg/blob/8e2eb9ac2e33ce4bac8956d4e2f099444d03c0e3/open-api/rest-catalog-open-api.yaml#L2057-L2060 > >>> > >> [3] > >>> > >> > >>> https://github.com/apache/iceberg/blob/64b36999d7ff716ae2534fb0972fcc10d22a64c2/core/src/main/java/org/apache/iceberg/SnapshotParser.java#L124 > >>> > >> [4] > >>> > >> > >>> https://github.com/apache/iceberg-python/blob/7cf0c225c3cdb32ac5e390de06b7b0e4fe7de92e/pyiceberg/table/snapshots.py#L182 > >>> > >> [5] > >>> > >> > >>> https://github.com/apache/iceberg/blob/22a6b19c2e226eacc0aa78c1f2ffbdbb168b13be/core/src/test/java/org/apache/iceberg/TestSnapshotJson.java#L52 > >>> > >> [6] https://github.com/apache/iceberg-python/issues/1106 > >>> > >> > >>> > >> > >>> > > >>> > >> >
