provide lazy iterator implementation #449

tychoish · 2024-01-01T17:06:38Z

Introduces a RawElement type which will can be used to lazily resolve bson values during iteration.

This does not change the functionality of any existing interfaces for compatibility concerns, (though the iter() functions do return different types) and the IntoIter implementations changed, but semantically it's all the same.

Happy to make changes if ya'll are interested. in this :)

abr-egn · 2024-01-02T18:39:20Z

Hi! This is definitely interesting, I'll be looking over the code in detail in the next few days. Just a clarification, when you said

This does change the functionality of any existing interfaces for compatibility concerns

that was a typo for

This does not change the functionality of any existing interfaces for compatibility concerns

correct? We're strict about maintaining backwards compatibility.

tychoish · 2024-01-02T23:53:52Z

Yes, sorry that's correct.

The only publicly visible change is the return type of the RawDocument's iter() method is now a wrapper around the new implementation (which has the previous type's name): but the returned iterator functions identically.

I'm not opposed to changing the name of the new (this) iterator and putting back a shim implementation with this name and the legacy functionality, though I don't know what things should be named (my inclination would be to call this RawIter, but I defer to you all, of course.

We're strict about maintaining backwards compatibility.

Oh, I know ;)

abr-egn · 2024-01-05T15:29:10Z

src/raw/iter.rs

+        self.size
+    }
+
+    pub fn key(&self) -> String {


Is there any particular reason to return the string as copy here rather than the ref?

nope, good call

abr-egn · 2024-01-05T15:30:03Z

src/raw/array.rs

@@ -275,7 +275,10 @@ impl<'a> Iterator for RawArrayIter<'a> {

    fn next(&mut self) -> Option<Result<RawBsonRef<'a>>> {
        match self.inner.next() {
-            Some(Ok((_, v))) => Some(Ok(v)),
+            Some(Ok(elem)) => match elem.value() {


Can this just be Some(Ok(elem)) => Some(elem.value()) ?

abr-egn · 2024-01-05T15:33:54Z

src/raw/document.rs

@@ -577,10 +594,10 @@ impl TryFrom<&RawDocument> for crate::Document {
 }

 impl<'a> IntoIterator for &'a RawDocument {
-    type IntoIter = Iter<'a>;
+    type IntoIter = Box<dyn Iterator<Item = Self::Item> + 'a>;


Unfortunately, this is a breaking change. I think this can be solved by what you suggested in the conversation - keep the existing functionality named Iter and call the new one RawIter.

alas, easy enough done

abr-egn · 2024-01-05T19:22:20Z

It looks like this fails the fuzz test; this program reproduces the failure:

use bson::RawDocument;

fn main() {
    let doc = RawDocument::from_bytes(&[25, 0, 0, 0, 5, 0, 0, 0, 0, 0, 0, 5, 0, 254, 255, 255, 127, 0, 5, 0, 255, 255, 255, 9, 0]).unwrap();
    for _ in doc {
        println!("_");
    }
}

tychoish · 2024-01-05T21:03:35Z

Nice! Thanks for the review. I'll look at this this weekend or early next week.

tychoish

thanks for the review; I made the changes and fixed the fuzz test.

tychoish · 2024-01-06T06:38:57Z

src/raw/iter.rs

+        self.size
+    }
+
+    pub fn key(&self) -> String {


nope, good call

tychoish · 2024-01-06T07:03:26Z

src/raw/array.rs

@@ -275,7 +275,10 @@ impl<'a> Iterator for RawArrayIter<'a> {

    fn next(&mut self) -> Option<Result<RawBsonRef<'a>>> {
        match self.inner.next() {
-            Some(Ok((_, v))) => Some(Ok(v)),
+            Some(Ok(elem)) => match elem.value() {


tychoish · 2024-01-06T07:05:16Z

src/raw/document.rs

@@ -577,10 +594,10 @@ impl TryFrom<&RawDocument> for crate::Document {
 }

 impl<'a> IntoIterator for &'a RawDocument {
-    type IntoIter = Iter<'a>;
+    type IntoIter = Box<dyn Iterator<Item = Self::Item> + 'a>;


alas, easy enough done

abr-egn · 2024-01-08T16:36:09Z

This fails the fuzzer with different input ([23, 0, 0, 0, 5, 0, 255, 255, 255, 255, 255, 255, 255, 3, 0, 0, 0, 247, 4, 247, 245, 0, 0]) and the corpus test (tests::spec::corpus::run) also fails with what is probably a related overflow error.

tychoish · 2024-01-08T17:50:33Z

This fails the fuzzer with different input ([23, 0, 0, 0, 5, 0, 255, 255, 255, 255, 255, 255, 255, 3, 0, 0, 0, 247, 4, 247, 245, 0, 0]) and the corpus test (tests::spec::corpus::run) also fails with what is probably a related overflow error.

Ok, this time I think I have it.

abr-egn · 2024-01-09T16:01:52Z

Still failing, I'm afraid ([17, 0, 0, 0, 5, 5, 0, 255, 255, 255, 127, 255, 255, 255, 11, 13, 0]). You can run the fuzzer test yourself (assuming you've installed cargo-fuzz and a nightly toolchain) with cargo +nightly fuzz run iterate -- -rss_limit_mb=4096 -max_total_time=60; probably a good idea to do that before pushing.

tychoish · 2024-01-10T17:22:00Z

I fixed it again, ran the fuzz test on my laptop for a while, nothing popped, and everything else passed (though my laptop is linux, it's not un-weird so I'm not sure what makes sense to validate.)

abr-egn

Looks good! Tagging in Isabel for review.

isabelatkinson

lgtm, thank you for the contribution!

tychoish · 2024-01-12T19:47:56Z

Thanks @isabelatkinson @abr-egn! Do you schedule merges for things like this? Is there a regular release cadence for these libraries?

Cheers,
sam

abr-egn · 2024-01-18T15:17:31Z

We don't have a specific release schedule, but I anticipate releasing 2.9.0 with this included in a week or so.

tychoish added 11 commits December 31, 2023 15:57

raw: lazy iterator base implementation

06bcd4f

all but one test

7f5ca0c

cleanup len

89c41c8

fix dbpointer

9d5c717

null terminated check

0abd2d0

expose lazy iter

f79c4c0

naming and cleanup

e5ff849

reduce churn

66555e5

fmt fixup

4d5bf47

less dyn

ba58151

more cleanup

364131f

abr-egn self-assigned this Jan 2, 2024

abr-egn self-requested a review January 2, 2024 18:37

abr-egn reviewed Jan 5, 2024

View reviewed changes

tychoish added 2 commits January 6, 2024 02:08

cr feedback

1056729

fix fuzz

e39ee2e

tychoish commented Jan 6, 2024

View reviewed changes

cast better

ee56e06

tychoish added 2 commits January 9, 2024 13:18

fix lint

5f39853

fix overflow

b822da5

fix clippy/fmt

093de42

abr-egn approved these changes Jan 10, 2024

View reviewed changes

abr-egn requested a review from isabelatkinson January 10, 2024 17:59

isabelatkinson approved these changes Jan 12, 2024

View reviewed changes

abr-egn merged commit 9294ee5 into mongodb:main Jan 18, 2024
11 checks passed

tychoish mentioned this pull request Feb 26, 2024

fix: export RawElement type #461

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provide lazy iterator implementation #449

provide lazy iterator implementation #449

tychoish commented Jan 1, 2024 •

edited

Loading

abr-egn commented Jan 2, 2024

tychoish commented Jan 2, 2024

abr-egn Jan 5, 2024

tychoish Jan 6, 2024

abr-egn Jan 5, 2024

tychoish Jan 6, 2024

abr-egn Jan 5, 2024

tychoish Jan 6, 2024

abr-egn commented Jan 5, 2024

tychoish commented Jan 5, 2024 •

edited

Loading

tychoish left a comment

tychoish Jan 6, 2024

tychoish Jan 6, 2024

tychoish Jan 6, 2024

abr-egn commented Jan 8, 2024

tychoish commented Jan 8, 2024 •

edited

Loading

abr-egn commented Jan 9, 2024

tychoish commented Jan 10, 2024

abr-egn left a comment

isabelatkinson left a comment

tychoish commented Jan 12, 2024

abr-egn commented Jan 18, 2024

provide lazy iterator implementation #449

provide lazy iterator implementation #449

Conversation

tychoish commented Jan 1, 2024 • edited Loading

abr-egn commented Jan 2, 2024

tychoish commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abr-egn commented Jan 5, 2024

tychoish commented Jan 5, 2024 • edited Loading

tychoish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abr-egn commented Jan 8, 2024

tychoish commented Jan 8, 2024 • edited Loading

abr-egn commented Jan 9, 2024

tychoish commented Jan 10, 2024

abr-egn left a comment

Choose a reason for hiding this comment

isabelatkinson left a comment

Choose a reason for hiding this comment

tychoish commented Jan 12, 2024

abr-egn commented Jan 18, 2024

tychoish commented Jan 1, 2024 •

edited

Loading

tychoish commented Jan 5, 2024 •

edited

Loading

tychoish commented Jan 8, 2024 •

edited

Loading