Skip to content

Add Spark interop test for reading Puffin deletion vectors - #3476

Open
moomindani wants to merge 3 commits into
apache:mainfrom
moomindani:moomindani/puffin-dv-reader-interop-test
Open

Add Spark interop test for reading Puffin deletion vectors#3476
moomindani wants to merge 3 commits into
apache:mainfrom
moomindani:moomindani/puffin-dv-reader-interop-test

Conversation

@moomindani

@moomindanimoomindani commented Jun 10, 2026

Copy link
Copy Markdown

Rationale for this change

Extracted from #3474 per review feedback (#3474 (comment)): this integration test verifies that PyIceberg can read Puffin deletion vectors written by Spark, which holds independently of the PuffinWriter changes in that PR.

Are these changes tested?

This PR is test-only. The test passed CI as part of #3474 (integration-test job) before being extracted.

Are there any user-facing changes?

No.

ebyhr
ebyhr approved these changes Jun 10, 2026
View reviewed changes

@ebyhrebyhr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me except for comments.

Comment threadtests/integration/test_puffin_spark_interop.py Outdated
Comment threadtests/integration/test_puffin_spark_interop.py Outdated
Comment threadtests/integration/test_puffin_spark_interop.py Outdated
@moomindanimoomindani mentioned this pull request Jun 10, 2026
Open
@moomindani
Address review comments: move Puffin DV interop test to test_deletes.py
33aef58
- Move test_read_spark_written_puffin_dv from test_puffin_spark_interop.py
to test_deletes.py; delete the old file
- Replace run_spark_commands helper calls with direct spark.sql() calls
- Tighten entries assertion from > 0 to == 1
@moomindani
Tighten delete manifest assertion to == 1
2fc1c58
Co-authored-by: Isaac
rambleraptor
rambleraptor approved these changes Jun 22, 2026
View reviewed changes

@rambleraptorrambleraptor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thanks for doing this!

@moomindani

moomindani commented Jun 24, 2026

Copy link
Copy Markdown
Author

Hi @ebyhr, thanks for the review! I've addressed all your comments (moved the test to test_deletes.py, replaced run_spark_commands with direct spark.sql() calls, and switched to the == 1 assertions). Could you take another look and merge if it looks good? CI is green.

@moomindanimoomindani mentioned this pull request Jun 25, 2026
Open
@moomindani

moomindani commented Jul 8, 2026

Copy link
Copy Markdown
Author

Gentle reminder — this has two approvals and CI is green. Could a committer help merge this when convenient?

@moomindani

moomindani commented Jul 29, 2026

Copy link
Copy Markdown
Author

Following up on the reminder from three weeks ago — this is waiting on a committer rather than on further review.

It's test-only: one file (tests/integration/test_deletes.py), no user-facing change, all 16 CI checks green. Reviewed and approved by @ebyhr and @rambleraptor. It was extracted from #3474 at review request, and the test already passed the integration-test job as part of that PR before extraction.

@Fokko this test covers the DV read path you added in #1516 — would you mind taking a look and merging if it looks good? Happy to rebase first if you'd prefer.

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

Reviewers

2 more reviewers
@rambleraptorrambleraptorrambleraptor approved these changes
@ebyhrebyhrebyhr approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@moomindani@rambleraptor@ebyhr