Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@sungwy
Copy link
Collaborator

@sungwy sungwy commented Aug 8, 2024

Fixes: #1024

As well as a correctness issue when reading MOR tables with positional deletes

yield _to_requested_schema(
projected_schema, file_project_schema, batch, downcast_ns_timestamp_to_us=True, use_large_types=use_large_types
)
current_index += len(batch)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When working on fixing #1024 I realized a correctness issue was introduced here because we are using the length of the filtered batch instead of the original one when tracking the current_index. I think it'll be crucial to get this fix in with 0.7.1 as soon as possible to support our MOR users

Copy link
Contributor

Choose a reason for hiding this comment

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

Oof, that's a good find. Thanks @vhnguyenae for reporting this!

The order of applying filters also caught me when implementing positional deletes. In the long run, I think it would be good to push this down to Arrow, I created an issue a while ago: apache/arrow#35301 But that hasn't seen much traction.

@sungwy sungwy requested a review from Fokko August 8, 2024 21:17
@sungwy sungwy added this to the PyIceberg 0.7.1 release milestone Aug 8, 2024
@sungwy sungwy changed the title Fix: Handle Empty RecordBatch within _task_to_record_batches Fix: Handle Empty RecordBatch within _task_to_record_batches, fix correctness issue with positional deletes Aug 8, 2024
@Fokko
Copy link
Contributor

Fokko commented Aug 9, 2024

Thanks for fixing this so quickly @sungwy. Let's speed up the release of 0.7.1 and ship this to the users.

@Fokko Fokko merged commit 159805d into apache:main Aug 9, 2024
@sungwy sungwy deleted the fix-empty-batch branch August 9, 2024 12:42
sungwy added a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
sungwy added a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Table scan throws IndexError: list index out of range

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.