Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

return lazy iterator in get tombstone methods #452

Merged
merged 1 commit into from
Oct 9, 2021

Conversation

houqp
Copy link
Member

@houqp houqp commented Oct 9, 2021

Description

It's better to not force clone and iterator materialization in the API. Instead, we can return an iterator of reference and let caller decide whether they need to clone and when to materialize into a collection.

@houqp houqp requested review from rtyler, xianwill, mosyp and fvaleye October 9, 2021 02:29
@houqp
Copy link
Member Author

houqp commented Oct 9, 2021

@xianwill the the logic below, we have:

 let use_extended_remove_schema = tombstones
        .iter()
        .all(|r| r.extended_file_metadata == Some(true) && r.size.is_some());

Do we really check for all the tombstones? I thought it should be enough to set use_extended_remove_schema to false if one of the tombstone doesn't match the filer?

@houqp houqp enabled auto-merge (squash) October 9, 2021 03:07
Copy link
Collaborator

@fvaleye fvaleye left a comment

Choose a reason for hiding this comment

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

Nice!

@houqp houqp merged commit 9000bd4 into delta-io:main Oct 9, 2021
@houqp houqp deleted the qp_tombstone branch October 9, 2021 17:35
@mosyp
Copy link
Contributor

mosyp commented Oct 11, 2021

@houqp I feel like that's exactly what all is doing, e.g. from the rustdoc

    /// `all()` is short-circuiting; in other words, it will stop processing
    /// as soon as it finds a `false`, given that no matter what else happens,
    /// the result will also be `false`.

@houqp
Copy link
Member Author

houqp commented Oct 12, 2021

Ha, good point @mosyp!

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.

3 participants