validate added data files for snapshot compatibility #2050

New issue

Jump to bottom

Merged

Fokko merged 10 commits into apache:main from kaushiksrini:feat/validate_added_data_files

Jun 24, 2025

Contributor

kaushiksrini commented May 28, 2025

Closes #1929

Rationale for this change

Since we want to support snapshot write compatibility (feat: validate snapshot write compatibility #1772) and is part of the following parent issue Support IsolationLevels and Concurrency Safety Validation Checks #819

Are these changes tested?

Yes

Are there any user-facing changes?

No

kaushiksrini added 3 commits

May 27, 2025 19:56


          add validations for added data files

5285cac


          make function hidden in validate

3dd8a74


          add tests for added data files validations

38a4cdf

kaushiksrini changed the title ~~Feat/validate added data files~~ validate added data files by supporting validateAddedDataFiles

kaushiksrini changed the title ~~validate added data files by supporting validateAddedDataFiles~~ validate added data files for snapshot compatibility

sungwy reviewed

View reviewed changes

Collaborator

sungwy left a comment

Hi @kaushiksrini thank you for working on this PR!

The logic looks sound to me, I have some nit picks regarding doc strings and reducing duplication of code.

Please let me know what you think about the feedback!

pyiceberg/table/update/validate.py Outdated Show resolved Hide resolved

pyiceberg/table/update/validate.py

+                  partition_set: Optional[dict[int, set[Record]]],
+                  parent_snapshot: Optional[Snapshot],
+              ) -> Iterator[ManifestEntry]:
+                  if parent_snapshot is None:

Collaborator

sungwy May 29, 2025

Could we introduce a doc string for each function?

Contributor

Fokko Jun 24, 2025

I don't think this if makes sense, why not make the parent_snapshot non-optional?

Contributor

jayceslesar Jun 24, 2025

Its what the java implementation does right? https://github.com/apache/iceberg/blob/3a29199e73f2e9ae0f8f92a1a0732a338c66aa0d/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L685

pyiceberg/table/update/validate.py Outdated Show resolved Hide resolved

kaushiksrini added 3 commits

June 1, 2025 13:50


          docs: add docstrings to the functions

98c47ef


          change variable name to be more specific

5adb926


          refactor code to avoid redundancies in validations

9081c33

jayceslesar mentioned this pull request

feat: ManifestEntryEvaluator #2056

Closed

Contributor

Fokko commented Jun 2, 2025

@kaushiksrini could you resolve the conflicts? Thanks!


          Merge branch 'main' into feat/validate_added_data_files

dc0860b

jayceslesar reviewed

View reviewed changes

pyiceberg/table/update/validate.py Outdated Show resolved Hide resolved


          Update pyiceberg/table/update/validate.py

5fef4e8

Co-authored-by: Jayce Slesar <47452474+jayceslesar@users.noreply.github.com>

Fokko approved these changes

View reviewed changes

Contributor

Fokko left a comment

Looks good @kaushiksrini Some small comments, but I think we can move this forward 👍

pyiceberg/table/update/validate.py Outdated Show resolved Hide resolved

pyiceberg/table/update/validate.py Outdated Show resolved Hide resolved

pyiceberg/table/update/validate.py

Comment on lines 154 to +159

                   for manifest in manifests:
                       for entry in manifest.fetch_manifest_entry(table.io, discard_deleted=False):
-                          if entry.snapshot_id not in snapshot_ids:
-                              continue
-                          if entry.status != ManifestEntryStatus.DELETED:
-                              continue
-                          if data_filter is not None and evaluator(entry.data_file) is ROWS_CANNOT_MATCH:
-                              continue
-                          if partition_set is not None:
-                              spec_id = entry.data_file.spec_id
-                              partition = entry.data_file.partition
-                              if spec_id not in partition_set or partition not in partition_set[spec_id]:
-                                  continue
-                          yield entry
+                          if _filter_manifest_entries(
+                              entry, snapshot_ids, data_filter, partition_set, ManifestEntryStatus.DELETED, table.schema()
+                          ):
+                              yield entry

Contributor

Fokko Jun 24, 2025

Ignore this comment, but a missed opportunity for some syntactic sugar:

for manifest in manifests:
    yield from (
        entry
        for entry in manifest.fetch_manifest_entry(table.io, discard_deleted=False)
        if _filter_manifest_entries(
            entry, snapshot_ids, data_filter, partition_set, ManifestEntryStatus.DELETED, table.schema()
        )
    )

pyiceberg/table/update/validate.py

+                  partition_set: Optional[dict[int, set[Record]]],
+                  parent_snapshot: Optional[Snapshot],
+              ) -> Iterator[ManifestEntry]:
+                  if parent_snapshot is None:

Contributor

Fokko Jun 24, 2025

I don't think this if makes sense, why not make the parent_snapshot non-optional?

Fokko added 2 commits

June 24, 2025 20:12


          Update docstring

9c33f36


          Update docstring

ff6094e

Fokko merged commit c32aa04 into apache:main

10 checks passed

Contributor

Fokko commented Jun 24, 2025

This looks great @kaushiksrini, let's move this forward 🚀 Thanks for the review @sungwy and @jayceslesar 🙌

amitgilad3 pushed a commit to amitgilad3/iceberg-python that referenced this pull request


          Validate added data files for snapshot compatibility (apache#2050)

8c47fb3

<!--
Thanks for opening a pull request!
-->

<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->

Closes apache#1929

# Rationale for this change

- Since we want to support snapshot write compatibility (apache#1772) and is
part of the following parent issue apache#819

# Are these changes tested?

Yes

# Are there any user-facing changes?

No

<!-- In the case of user-facing changes, please add the changelog label.
-->

---------

Co-authored-by: Jayce Slesar <47452474+jayceslesar@users.noreply.github.com>
Co-authored-by: Fokko Driesprong <fokko@apache.org>

gabeiglio pushed a commit to Netflix/iceberg-python that referenced this pull request


          Validate added data files for snapshot compatibility (apache#2050)

4152ee4

<!--
Thanks for opening a pull request!
-->

<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->

Closes apache#1929

# Rationale for this change

- Since we want to support snapshot write compatibility (apache#1772) and is
part of the following parent issue apache#819

# Are these changes tested?

Yes

# Are there any user-facing changes?

No

<!-- In the case of user-facing changes, please add the changelog label.
-->

---------

Co-authored-by: Jayce Slesar <47452474+jayceslesar@users.noreply.github.com>
Co-authored-by: Fokko Driesprong <fokko@apache.org>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet