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

Make iterate() lazily evaluated on wasm #527

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

diegoreis42
Copy link
Contributor

#514

Introduces a new feature for lazy evaluation in the Statement.raw().iterate() method and includes related changes in both the test and implementation files. The most important changes include adding a test case for lazy evaluation, creating a RowIterator struct, and modifying the iterate method to use this new struct.

Everything seems to works fine, but suggestions on code improvement and test use cases are welcoming.

@penberg
Copy link
Collaborator

penberg commented Dec 21, 2024

@diegoreis42 Looks great! I am guessing some auto-formatted turned the diff on the test file into something that's pretty unreadable. Can you please change back to 4 space indent to make that change readable?

@Lemon-Peppermint
Copy link

Lemon-Peppermint commented Dec 21, 2024

@diegoreis42 FYI, js_sys::Array implements FromIterator, you could move your RowIterator.next() method to an implementation of Iterator then use that to construct your Array instead of going through an intermediate Object.

Edit: Never mind , just checked the Array::from_iter() implementation, it still eagerly evaluates the arguments.

@diegoreis42
Copy link
Contributor Author

@diegoreis42 Looks great! I am guessing some auto-formatted turned the diff on the test file into something that's pretty unreadable. Can you please change back to 4 space indent to make that change readable?

Yeah sure, sorry about that.

@diegoreis42
Copy link
Contributor Author

diegoreis42 commented Dec 21, 2024

Hey, I thought that CI test suit would run wasm tests but, apparently, it doesn't. Some changes are needed to conform with previous tests, I'm working on this.

In order to test if the rows are lazily loaded, I wrote a SQL trigger, but it has not been implemented yet by Limbo, so it throws an error. Any inputs in how to test if the rows are lazily loaded?

Btw, perhaps it would be nice to include wasm testing on CI, afaik only wasm build is executed, I can try do this in another PR.

@diegoreis42
Copy link
Contributor Author

@penberg in the iterator, when a row is busy it returns undefined, it's fine or other behavior is required?

Copy link
Collaborator

@pereman2 pereman2 left a comment

Choose a reason for hiding this comment

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

Can you rebase, it will be unreviewable if I cannot find the changes.

@diegoreis42
Copy link
Contributor Author

Can you rebase, it will be unreviewable if I cannot find the changes.

Done.

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.

4 participants