Thanks, Ryan! That makes sense. I want to follow up on the original issue. I've made a PR [1] to enforce that the Snapshot `summary` map must have an `operation` key. Please take a look. Thank you @nastra for the comments and reviews.
Best, Kevin Liu [1] https://github.com/apache/iceberg/pull/11354 On Tue, Oct 22, 2024 at 4:06 PM rdb...@gmail.com <rdb...@gmail.com> wrote: > > For example, the `Snapshot` `summary` field is optional in V1 but > required in V2. Therefore, the REST spec definition should mark the > `summary` field as optional to support both versions. > > Yeah, this is technically true. But as I said in my first email, unless > you have tables that are 5 years old, it's unlikely that this is going to > be a problem. A failure here is more likely with newer implementations that > have a bug. So I'd argue there's value in leaving it as required. > > On Mon, Oct 21, 2024 at 9:41 AM Kevin Liu <kevin.jq....@gmail.com> wrote: > >> > No. They were introduced at the same time. >> Great! Since the `summary` field and the `operation` key were introduced >> together, we should enforce the rule that the `summary` field must >> always have an accompanying `operation` key. This has been addressed in >> PR 11354 [1]. >> >> > I am strongly against this. The REST spec should be independent of the >> table versions. >> That makes sense. For the REST spec to support both V1 and V2 tables, it >> should "accept" the least common denominator between the two versions. For >> example, the `Snapshot` `summary` field is optional in V1 but required in >> V2. Therefore, the REST spec definition should mark the `summary` field as >> optional to support both versions. However, the current REST spec leans >> towards the V2 table spec; fields that are optional in V1 and required in >> V2 are marked as required in the spec, such as `TableMetadata.table-uuid` >> [2][3] and `Snapshot.summary` [4][5]. >> >> Would love to get other people's thoughts on this. >> >> Best, >> Kevin Liu >> >> [1] https://github.com/apache/iceberg/pull/11354 >> [2] >> https://github.com/apache/iceberg/blob/8e743a5b5209569f84b6bace36e1106c67e1eab3/open-api/rest-catalog-open-api.yaml#L2414 >> [3] https://iceberg.apache.org/spec/#table-metadata-fields >> [4] >> https://github.com/apache/iceberg/blob/8e743a5b5209569f84b6bace36e1106c67e1eab3/open-api/rest-catalog-open-api.yaml#L2325 >> [5] https://iceberg.apache.org/spec/#snapshots >> >> On Sun, Oct 20, 2024 at 11:24 AM rdb...@gmail.com <rdb...@gmail.com> >> wrote: >> >>> Was it ever valid to have a summary field without the operation key? >>> >>> No. They were introduced at the same time. >>> >>> Would it be helpful to create alternative versions of the REST spec >>> specifically for referencing V1 and V2 tables? >>> >>> I am strongly against this. The REST spec should be independent of the >>> table versions. Any table format version can be passed and the table format >>> should be the canonical reference for what is allowed. We want to avoid >>> cases where there are discrepancies. The table spec is canonical for table >>> metadata, and the REST spec allows passing it. >>> >>> On Sun, Oct 20, 2024 at 11:18 AM Kevin Liu <kevin.jq....@gmail.com> >>> wrote: >>> >>>> Hey folks, >>>> >>>> Thanks, everyone for the discussion, and thanks Ryan for providing the >>>> historical context. >>>> Enforce the `operation` key in Snapshot’s `summary` field >>>> >>>> When serializing the `Snapshot` object from JSON, the Java >>>> implementation does not enforce that the `summary` field must contain an >>>> `operation` key. In the V1 spec, the `summary` field is optional, while in >>>> the V2 spec, it is required. However, in both versions, if a `summary` >>>> field is present, it must include an `operation` key. Any `summary` field >>>> lacking an `operation` key should be considered invalid. >>>> >>>> I’ve addressed this issue in PR 11354 [1] by adding this constraint >>>> when parsing JSON. >>>> >>>> > We initially did not have the snapshot summary or operation. When I >>>> added the summary, the operation was intended to be required in cases where >>>> the summary is present. It should always be there if the summary is and the >>>> summary should always be there unless you wrote the metadata.json file >>>> way back in 2017 or 2018. >>>> >>>> @Ryan, does this constraint also apply to `metadata.json` files from >>>> 2017/2018? Was it ever valid to have a `summary` field without the >>>> `operation` key? >>>> >>>> > Well, the spec says nothing about a top-level `operation` field in >>>> JSON [1]. Yet the Java implementation produces it [2] and removes the >>>> operation from the summary map. This seems inconsistent? >>>> >>>> @Anton, the Java `Snapshot` object includes both the `summary` and >>>> `operation` fields. When serializing to JSON, the `operation` field is >>>> included in the `summary` map [2], rather than as a top-level field. During >>>> deserialization from JSON, the `operation` field is extracted from the >>>> `summary` map [3]. >>>> >>>> I believe this is consistent with the table spec, which defines the >>>> JSON output, not how the `Snapshot` object is implemented in Java. >>>> On REST spec and Table spec >>>> >>>> Thanks, Yufei, for highlighting the difference between the REST spec >>>> and the table spec. I mistakenly used the REST spec >>>> (`rest-catalog-open-api.yaml` [4]) as the source of truth for V2 tables. >>>> >>>> Looking at the REST spec file, it can be challenging to determine how a >>>> REST server should handle V1 versus V2 tables. Even for V2 tables, the >>>> current version of the file combines features from V2, along with >>>> additional changes made in preparation for the upcoming V3 spec. >>>> >>>> Would it be helpful to create alternative versions of the REST spec >>>> specifically for referencing V1 and V2 tables? The goal would be to have a >>>> "frozen" version of the REST spec dedicated to V1 tables and another for V2 >>>> tables while allowing the current REST spec file to evolve as needed. >>>> >>>> Taking a step back, I think we need more documentation on the REST >>>> spec, including support for different table versions and guidance on >>>> upgrading from one version to another. I’d love to hear everyone’s thoughts >>>> on this. >>>> >>>> >>>> Best, >>>> >>>> Kevin Liu >>>> >>>> >>>> [1] https://github.com/apache/iceberg/pull/11354 >>>> >>>> [2] >>>> https://github.com/apache/iceberg/blob/17f1c4d2205b59c2bd877d4d31bbbef9e90979c5/core/src/main/java/org/apache/iceberg/SnapshotParser.java#L63-L66 >>>> >>>> [3] >>>> https://github.com/apache/iceberg/blob/17f1c4d2205b59c2bd877d4d31bbbef9e90979c5/core/src/main/java/org/apache/iceberg/SnapshotParser.java#L124-L137 >>>> >>>> [4] >>>> https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml >>>> >>>> >>>> On Sat, Oct 19, 2024 at 7:48 PM Sung Yun <sun...@apache.org> wrote: >>>> >>>>> Hi Ryan, thank you for your response! >>>>> >>>>> That detailed context is very helpful in allowing me to understanding >>>>> why the REST catalog spec has evolved the way it has, and how the Table >>>>> Spec and the REST Catalog Spec should each be referenced in the >>>>> sub-communities (like in PyIceberg). I'll keep those motivations in mind >>>>> as >>>>> we discuss those Specs in the future. >>>>> >>>>> Also, here's a small PR to specify more explicitly that the operation >>>>> field should be a required field in the summary field: >>>>> https://github.com/apache/iceberg/pull/11355 >>>>> >>>>> Sung >>>>> >>>>> On 2024/10/19 22:14:59 "rdb...@gmail.com" wrote: >>>>> > I can provide some historical context here about how the table spec >>>>> evolved >>>>> > and how the REST spec works with respect to table versions. >>>>> > >>>>> > We initially did not have the snapshot summary or operation. When I >>>>> added >>>>> > the summary, the operation was intended to be required in cases >>>>> where the >>>>> > summary is present. It should always be there if the summary is and >>>>> the >>>>> > summary should always be there unless you wrote the metadata.json >>>>> file way >>>>> > back in 2017 or 2018. It looks like the spec could be more clear >>>>> that the >>>>> > operation is required when summary is present. Anyone want to open a >>>>> PR? >>>>> > >>>>> > Anton, I don't think there is a top-level operation field. The Java >>>>> > Snapshot class tracks the operation as top-level, but it is always >>>>> stored >>>>> > in the summary. I think this is consistent with the spec. >>>>> > >>>>> > For the REST spec, I think that it should be strictly optional to >>>>> support >>>>> > v1 tables written with no summary, but it should always be present. >>>>> I'd >>>>> > probably leave it required since it already is and is catching a >>>>> valuable >>>>> > error case here. >>>>> > >>>>> > When building the REST spec, I took the same approach as the Java >>>>> parser >>>>> > (which is also to parse table metadata coming from REST servers). >>>>> That is, >>>>> > it is compatible with v1 metadata; fields that were not required in >>>>> v1 are >>>>> > optional. This fits with the principle of "be liberal in what you >>>>> accept >>>>> > and strict in what you produce". The REST spec allows passing >>>>> metadata for >>>>> > any table version so that the specs are not tightly coupled. The >>>>> table >>>>> > version is passed when loading and clients should reject table >>>>> versions >>>>> > that are newer than what they can support. The REST protocol just >>>>> needs to >>>>> > facilitate passing the table metadata. >>>>> > >>>>> > Most v2 structures, like the `schemas` list, are introduced as >>>>> optional in >>>>> > v1 and made required in v2. That way, it's possible to add metadata >>>>> to >>>>> > existing format versions and make the upgrade path easier. >>>>> Maintaining the >>>>> > newer structures even though there are different writer versions >>>>> deployed >>>>> > is one of the reasons why the REST spec changes to a change-based >>>>> model. >>>>> > New metadata only needs to be supported by the service maintaining >>>>> the >>>>> > metadata.json files and any writers that want to update it. >>>>> > >>>>> > I see some points about being able to remove old table versions. I >>>>> don't >>>>> > think that the REST protocol itself is the place to do this. The >>>>> protocol >>>>> > is format-agnostic. Implementations are free to reject requests to >>>>> create >>>>> > tables with older versions, or to update the table to a new version. >>>>> > >>>>> > Ryan >>>>> > >>>>> > On Fri, Oct 18, 2024 at 6:42 AM Sung Yun <sun...@apache.org> wrote: >>>>> > >>>>> > > Folks, thank you for your responses! These area really helpful >>>>> insights. >>>>> > > >>>>> > > > I agree that the REST spec should aim to support v1, v2, and >>>>> potentially >>>>> > > the upcoming v3. In practice, it seems like the choice of table >>>>> spec might >>>>> > > ultimately be dictated by the REST catalog implementation. >>>>> > > >>>>> > > > A best practice would be for the server to strive to support all >>>>> Iceberg >>>>> > > versions, but the REST spec itself should remain flexible enough to >>>>> > > accommodate less strict table specs. >>>>> > > >>>>> > > Yufei, yes that makes sense, and I agree that the server should >>>>> strive to >>>>> > > support all format versions, because otherwise the an older client >>>>> > > application, may just not be compatible with a REST Catalog >>>>> running on a >>>>> > > higher version of table spec. I think we have two choices here in >>>>> ensuring >>>>> > > that the REST Catalog server is able to support multiple versions >>>>> of the >>>>> > > Table Spec: >>>>> > > >>>>> > > 1. We could create single components that are common denominators >>>>> of all >>>>> > > existing table specs to accommodate the less table specs. The REST >>>>> Catalog >>>>> > > Spec currently falls short in this approach, and I've put up this >>>>> PR to >>>>> > > show what this change would look like just for the Snapshot >>>>> component: >>>>> > > https://github.com/apache/iceberg/pull/11353 - My take on this is >>>>> that, >>>>> > > this approach will make the REST catalog spec more confusing and >>>>> difficult >>>>> > > to manage as we add more Table Spec versions moving forward. The >>>>> discussion >>>>> > > on this mail list thread is I think a great demonstration of that >>>>> confusion >>>>> > > :) >>>>> > > 2. We could instead create separate Table Spec version specific >>>>> components >>>>> > > on the REST Catalog Open API Spec. For example, a Snapshot >>>>> component could >>>>> > > be anyOf SnapshotV1 and SnapshotV2, which match the Table Spec V1 >>>>> and V2 >>>>> > > definitions. I think creating explicit components that match the >>>>> spec >>>>> > > definitions will work in our favor when we continue to introduce >>>>> more Spec >>>>> > > changes and manage their lifecycles. And perhaps, maybe we could >>>>> also >>>>> > > indicate what format-versions the REST Catalog Server supports >>>>> through an >>>>> > > endpoint, and communicate it to a client application. >>>>> > > >>>>> > > I'd love to hear the community's opinion on suggestion (2)! I'm >>>>> very >>>>> > > curious to hear if we've considered it before. >>>>> > > >>>>> > > Sung >>>>> > > >>>>> > > On 2024/10/18 05:13:15 Péter Váry wrote: >>>>> > > > Hi Team, >>>>> > > > Apart from fixing this current issue by relaxing the current spec >>>>> > > > constraints, to support both v1 and v2 specifications, we should >>>>> think >>>>> > > > about how to handle table spec evolution for the long term. >>>>> > > > >>>>> > > > What are the base factors we can start from (please add your own >>>>> ideas >>>>> > > if I >>>>> > > > have missed something): >>>>> > > > - We evolve the specifications in a way that is backwards >>>>> compatible (v1 >>>>> > > > table could be read by v2 reader) but not forwards compatible >>>>> (v2 table >>>>> > > > could not be read by an old reader) >>>>> > > > - The rest spec ideally should conform to the currently used >>>>> table spec >>>>> > > > schema/constraints >>>>> > > > - REST catalogs sooner-or-later would like to drop support for >>>>> older >>>>> > > table >>>>> > > > spec. We need to avoid the situation of Hive Metastore, where the >>>>> > > decisions >>>>> > > > made 10 years ago prevented enhancing the APIs as the old >>>>> specifications >>>>> > > > were supported forever. >>>>> > > > >>>>> > > > Probably (when the spec difference becomes big enough) a >>>>> composit request >>>>> > > > (version + different content spec) or a different endpoint will >>>>> be >>>>> > > required. >>>>> > > > >>>>> > > > Thanks, Peter >>>>> > > > >>>>> > > > On Thu, Oct 17, 2024, 23:11 Yufei Gu <flyrain...@gmail.com> >>>>> wrote: >>>>> > > > >>>>> > > > > Hi Sung, >>>>> > > > > >>>>> > > > > It seems we are running to issues related to a mismatch >>>>> between the >>>>> > > REST >>>>> > > > > spec and table specifications. Currently, there's no clear >>>>> definition >>>>> > > of >>>>> > > > > how the REST spec is meant to support different table specs. >>>>> The >>>>> > > closest >>>>> > > > > reference I found is this statement >>>>> > > > > < >>>>> > > >>>>> https://github.com/apache/iceberg/blob/8e743a5b5209569f84b6bace36e1106c67e1eab3/open-api/rest-catalog-open-api.yaml#L30-L30 >>>>> > > > >>>>> > > > > in the REST spec. >>>>> > > > > >>>>> > > > > Implementations should ideally support both Iceberg table >>>>> specs v1 and >>>>> > > v2, >>>>> > > > >> with priority given to v2. >>>>> > > > > >>>>> > > > > >>>>> > > > > I agree that the REST spec should aim to support v1, v2, and >>>>> > > potentially >>>>> > > > > the upcoming v3. In practice, it seems like the choice of >>>>> table spec >>>>> > > might >>>>> > > > > ultimately be dictated by the REST catalog implementation. >>>>> > > > > >>>>> > > > > >>>>> > > > > A best practice would be for the server to strive to support >>>>> all >>>>> > > Iceberg >>>>> > > > > versions, but the REST spec itself should remain flexible >>>>> enough to >>>>> > > > > accommodate less strict table specs. For the case you >>>>> mentioned, it >>>>> > > should >>>>> > > > > be fine to make sequence number optional since the spec has to >>>>> support >>>>> > > v1 >>>>> > > > > table spec. It does feel confusing though. >>>>> > > > > >>>>> > > > > >>>>> > > > > WDYT? >>>>> > > > > >>>>> > > > > >>>>> > > > > Yufei >>>>> > > > > >>>>> > > > > >>>>> > > > > On Thu, Oct 17, 2024 at 1:56 PM Anton Okolnychyi < >>>>> > > aokolnyc...@gmail.com> >>>>> > > > > wrote: >>>>> > > > > >>>>> > > > >> Well, the spec says nothing about a top-level `operation` >>>>> field in >>>>> > > JSON >>>>> > > > >> [1]. Yet the Java implementation produces it [2] and removes >>>>> the >>>>> > > operation >>>>> > > > >> from the summary map. This seems inconsistent? >>>>> > > > >> >>>>> > > > >> - Anton >>>>> > > > >> >>>>> > > > >> [1] - https://iceberg.apache.org/spec/#snapshots >>>>> > > > >> [2] - >>>>> > > > >> >>>>> > > >>>>> https://github.com/apache/iceberg/blob/17f1c4d2205b59c2bd877d4d31bbbef9e90979c5/core/src/main/java/org/apache/iceberg/SnapshotParser.java#L63 >>>>> > > > >> >>>>> > > > >> >>>>> > > > >> чт, 17 жовт. 2024 р. о 10:06 Sung Yun <sun...@apache.org> >>>>> пише: >>>>> > > > >> >>>>> > > > >>> > 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 >>>>> > > > >>> > >>> > >> >>>>> > > > >>> > >>> > >> >>>>> > > > >>> > >>> > >>>>> > > > >>> > >>> >>>>> > > > >>> > >> >>>>> > > > >>> > >>>>> > > > >>> >>>>> > > > >> >>>>> > > > >>>>> > > >>>>> > >>>>> >>>>
