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

Base.isdone for Stream #428

Merged
merged 2 commits into from
May 31, 2023
Merged

Base.isdone for Stream #428

merged 2 commits into from
May 31, 2023

Conversation

baumgold
Copy link
Member

@baumgold baumgold commented Apr 14, 2023

Implement Base.isdone for Arrow.Stream

https://github.com/JuliaLang/julia/blob/v1.8.5/base/essentials.jl#L882-L895

This enables calling Base.isempty(::Arrow.Stream) without affecting the state of the Arrow.Stream object.

@baumgold baumgold requested a review from quinnj April 14, 2023 22:58
@baumgold baumgold force-pushed the isdone branch 5 times, most recently from de5b7a2 to 3105964 Compare April 15, 2023 00:24
src/table.jl Outdated Show resolved Hide resolved
@quinnj
Copy link
Member

quinnj commented Apr 23, 2023

Hmmmm, I'm not sure I'm following the changes or reasoning here. Can you explain how isempty changes the current Arrow.Stream state? I'm not super familiar w/ Base.isdone or how it's used for other types or why; if you could share an example, that would be great. Another question that comes to mind is why we shouldn't define isempty(::Arrow.Stream) ourselves if it's causing problems?

@baumgold
Copy link
Member Author

@quinnj - Iterating on Arrow.Stream modifies the inputindex field, making it stateful iteration rather than stateless. The default implementation for Base.isempty uses iteration to check if there are further elements remaining:

https://github.com/JuliaLang/julia/blob/v1.8.5/base/essentials.jl#L785-L789

Unfortunately this doesn't work for stateful iterators since calling Base.isdone modifies the iterator's state. A solution to this is to define Base.isdone:

https://github.com/JuliaLang/julia/blob/v1.8.5/base/essentials.jl#L882-L895

Note the doc-string explains this quite clearly:

This function provides a fast-path hint for iterator completion. This is useful for mutable iterators that want to avoid having elements consumed, if they are not going to be exposed to the user (e.g. to check for done-ness in isempty or zip). Mutable iterators that want to opt into this feature should define an isdone method that returns true/false depending on whether the iterator is done or not. Stateless iterators need not implement this function. If the result is missing, callers may go ahead and compute iterate(x, state...) === nothing to compute a definite answer.

@quinnj quinnj merged commit d1b5326 into apache:main May 31, 2023
@baumgold baumgold deleted the isdone branch May 31, 2023 01:43
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.

2 participants