RUST-284: Incorporate rawbson code from rawbson = "0.2.1" in mod raw #229

jcdyer · 2021-02-07T16:01:45Z

Implements #133

saghm · 2021-02-08T19:04:03Z

Thanks for the pull request @jcdyer! Is it all right with you if I start pushing commits to this branch? We're interested in getting this out to users as soon as possible (specifically in our 2.0 release, which we just released an alpha for), so it will go more quickly if we can write the code for changes we want to make ourselves. We definitely would love your input as a reviewer for the code we write!

jcdyer · 2021-02-09T00:19:58Z

by all means! Use it as you see fit. I'm happy to answer questions, discuss implementation ideas, review code, or whatever helps.

src/raw/mod.rs

jcdyer · 2021-02-09T00:25:12Z

src/raw/mod.rs

+        DocBuf {
+            data: data.into_boxed_slice(),
+        }
+    }


This function doesn't actually do anything unsafe. I marked it that way out of a suspicion that it might someday in the future, so it's marked to prevent backwards compatibility semver issues, but everything interacts with the underlying slice in a completely safe manner.

Interesting; I was wondering why it was marked as unsafe. I think if it ever does need to become unsafe, we would probably prefer to just add a separate method and deprecate this one rather than marking it as unsafe even though it currently doesn't need to be, so I've removed that annotation in my follow-up commit.

jcdyer · 2021-02-09T00:28:04Z

src/raw/mod.rs

+/// Error to indicate that either a value was empty or it contained an unexpected
+/// type, for use with the direct getters.
+#[derive(Debug, PartialEq)]
+pub enum RawError {


It would be nice to merge this with the primary crate Error type. The main difference is needing to be able to signal malformed bson when accessing elements.

Unfortunately, we don't actually have a single error type in the crate, but each submodule defines their own (e.g. document::ValueAccessError, oid::Error, etc.). Given the current situation, it makes sense to leave this as-is for now, and maybe in a 2.0 release we can considering unifying the crate under a single error type.

src/raw/mod.rs

saghm · 2021-02-10T19:15:15Z

Thanks so much for this PR! We're going to hold off for a couple of weeks before merging the serde implementation in order to focus on the API that we need to be able to update the driver for the 2.0 release, but we definitely plan on using it once we're ready, and we'll make sure that you get proper credit for it.

I've pushed some small API and documentation changes as a separate commit and added the rest of the team as reviewers to the pull request.

patrickfreed

Hi @jcdyer, thanks again for the PR on this--it looks great so far! To avoid possibly duplicating any work though, @saghm will handle making any code changes @isabelatkinson and I request going forward, but feel free to chime in during the review as you see fit.

src/raw/elem.rs

src/raw/mod.rs

src/raw/props.rs

jcdyer · 2021-02-14T15:42:49Z

To summarize some of my comments from the other day, I'd caution against thinking about (or advertising) RawDocument simply as a faster Document, or about trying to force the two to look exactly the same at the API level. It is a different data structure with different performance characteristics. In many cases, it is faster, but not all.

In some cases, as the benchmarks show, RawDocument is significantly slower than Document. For instance, reading an element out of a document by name is O(n) in RawDocument, since you have to start at the beginning and read each element out of the RawDocument until you find the one you want, while Document is O(1), since it is just a hashmap. If you only need a single element, RawDocument is faster since the parsing time overwhelms the measurement, but as the number of elements accessed goes up, raw document becomes O(M * N) (M= number of accesses, N=size of document), while Document is O(M * 1).

Where there is clear similarity between the two types, it makes sense to keep a similar API, but document mutation is not one of those situations. Raw document can support some mutations fairly simply, but others are quite complex, and should be discouraged:

RawDocument append

Easy. O(1) performance (assuming a reasonably sized element)

Remove the trailing \0
add the new element
append a trailing \0
rewrite the length marker at the beginning of the document.

Requires an O(n) read to guarantee non-duplicate keys, because you have to iterate over the document to compare with the existing keys.

RawArray append

Note: there currently isn't an owning RawArray type, since that's not part of the bson spec. If such a type were created, this could be implemented.

Easy, O(n) performance:

Iterate the document to get the current length
append as above with length as the key.

Non-size-changing element modification

(e.g., changing the value of a fixed size element, without changing its type)

Access the desired element (O(n))
Modify the element.

General element modification / element replacement

Slow, moderately complicated

Find the desired element (O(n))
Reserve all following elements to a buffer. (O(n) write)
Modify / replace the element.
Append the reserved Elements (O(n) write)
Rewrite the length marker at the beginning of the document.

Modifying a nested document:

Slow and complicated (O(N * depth of nesting)).

For a nested key x.y.z:

Find element x. Copy all following elements to a buffer as for general modification
Find element y in embedded document x. Copy all following elements to a buffer as for general element modification.
Find element z in embedded document y: Copy all following elements to a buffer as for general element modification.
Modify element z.
Append reserved elements for document y.
Rewrite the length marker at the beginning of y.
Append reserved elements for document x.
Rewrite the length marker at the beginning of x.
Append reserved elements for top-level document.
Rewrite the length marker for top-level document.

Note, this algorithmic complexity is exacerbated by the fact that the data is owned by the top-level document, so inner documents aren't even capable of resizing themselves to accomodate modified elements.

Summary

I would recommend supporting the following operations:

non-key-checking (O(1)) push and extend.
key-checking (O(n)) push and extend.
impl FromIterator<Item=(&str, &RawBson)> for RawDocument

Then document that other modifications should be made either by iterating over the existing document and collecting to a fresh RawDocument, or by deserializing to a more appropriate data structure to perform mutations (a Document or custom type), then serializing back to RawDocument.

patrickfreed

Thanks for the detailed overview @jcdyer ! I definitely agree that RawDocument is not a strict improvement performance-wise for all use-cases. I think the lion's share of the perf benefits will come from within the driver actually, since it should allow us to skip an intermediate serialization step for insertions and potentially reduce a lot of heap allocations in other places. It's unclear if deserialization will improve significantly though, since that requires mostly key lookups.

I also agree with the summary / suggested operations. One additional operation that may be worth including is entry deletion, since that should be pretty straightforward as well (will be O(n) as with all other key-based operations though). This would make it easier for a user to do a replace should they have to, though again we should try to encourage them to use a different type if they plan to do mutations of that sort. I guess they could also trivially implement it via a filter + collect + non-key-checking append, though, so maybe its not necessary.

src/raw/elem.rs

src/raw/mod.rs

src/raw/props.rs

saghm · 2021-02-16T19:52:13Z

src/raw/error.rs

@@ -0,0 +1,27 @@
+/// An error that occurs when attempting to parse raw BSON bytes.
+#[derive(Debug, PartialEq)]
+pub enum Error {


As part of refactoring this into a separate submodule, I removed the Raw prefix from the Error and Result types defined here. This is consistent with how we define errors elsewhere in the library (e.g. bson::de::Error and bson::ser::Error), and users will need to import it with bson::raw::{Error, Result} anyhow.

As for RawDocument and friends, I do not think we should remove the prefix here. raw::Error and raw::Result are essentially semantically equivalent to other Error and Result types both within this library and externally, Document and RawDocument are semantically different in significant ways. Put another way, I don't think it would be confusing for readers of code using this library to gloss over what the exact Result or Error type is given that their expectations of how they work will be correct, but glossing over whether a regular Document or a RawDocument is used seems much more dangerous.

patrickfreed

It looks like the tests aren't compiling after the module reorganization.

isabelatkinson · 2021-02-17T14:43:31Z

src/raw/array.rs

+    }
+}
+
+/// An iterator over borrwed raw BSON array values.


nit: typo here (should be borrowed)

isabelatkinson · 2021-02-17T14:47:25Z

src/raw/doc.rs

+/// # use bson::raw::{RawDocument, Error};
+/// let doc = RawDocument::new(b"\x13\x00\x00\x00\x02hi\x00\x06\x00\x00\x00y'all\x00\x00".to_vec())?;
+/// let mut iter = doc.iter();
+/// let (key, value) = iter.next().unwrap()?;


is it necessary to have both unwrap and ? here?

Yes. The return type is Option<Result<,>>. It could also be .unwrap().unwrap().

jcdyer · 2021-02-19T16:49:41Z

Tangential to this, are there any plans to derive(Copy) for ObjectId? Its current impl is stack allocated, fixed size, and smaller than the pointer for a &[u8] on 64 bit platforms, and would clean up a lot of code.

saghm · 2021-02-19T17:27:43Z

Tangential to this, are there any plans to derive(Copy) for ObjectId? Its current impl is stack allocated, fixed size, and smaller than the pointer for a &[u8] on 64 bit platforms, and would clean up a lot of code.

Yeah, I think this probably is worth doing. Feel free to open a separate PR with this change!

jcdyer · 2021-04-14T01:42:57Z

Where does this stand? From what I can see, it seems to have stalled out again. Is there anything I can do to help get it moving again?

patrickfreed · 2021-04-14T21:06:55Z

We had to put this on the backburner for a little while to focus on getting the 2.0 release of the driver out as well as a few other internal priorities. We'll be picking this up again in the near term future after the 2.0 stable release is out. Thanks again for your continued patience and all the work you've put into this so far, and sorry about the delay!

patrickfreed · 2021-10-05T00:02:10Z

Hey @jcdyer, just wanted to let you know that we've picked this up again now that the 2.0 has been released. As a first step, I'll be tidying things up and addressing any of the outstanding comments from the last round of reviews. After that's done, I'll open it up to the team for another review. As with last time, feel free to chime in with any comments as you see fit.

Thanks again for getting the ball rolling on this!

patrickfreed

The recent batch of commits made a number of API and implementation changes, most notably:

Rewrote RawBson as an enum. If we want to introduce an API that matches the old RawBson (i.e. a struct that wraps a reference to a slice of bytes), then I think an Entry API might be a good opportunity for that.
Renamed RawDocumentRef to RawDoc. This name matches the convention of String/str, and it is also slightly more correct, since the type isn't a reference itself but has to be referred to as part of one, being dynamically sized.
- Something like RawDocumentBuf/RawDocument would also work fine IMO (matches PathBuf/Path)
Renamed RawArray to RawArr. This was done for similar reasons as the RawDoc rename, and it also makes room for an owned version of RawArray should we need it.
Baked in a bunch of bounds checking into the code that parses the raw BSON into Rust types.
Introduced a ValueAccessError type for the typed getters. Originally, we were planning on moving away from this paradigm, but given that we decided to keep it for the typed getters on Document, I think it made sense to reintroduce it here.
Broke off error enums into ErrorKind and ValueAccessErrorKind to enable storing of the key / other metadata on the actual error structs.
Implemented Copy for Decimal128
Introduced a RawDbPointer type
Removed the RawTimestamp type - we can just rely on the regular one since it's Copy
Hooked up the raw document API to the corpus tests
Touched up the documentation
Moved code into separate files (document.rs and iter.rs are new files containing mostly code that already existing in the PR, props.rs was simply moved to the test directory)
Marked RawDoc and RawArr as #[repr(transparent)], which provides the necessary safety guarantees for doing the casts we do in new_unchecked. This means we'll need to commit to the byte layout of these types too in addition to their APIs.

I filed a ticket to cover the work for introducing an append method to RawDocument, which can be done separately from this PR.

I think this PR is good to be reviewed again, let me know if you have any questions about the above changes.

patrickfreed · 2021-10-06T19:23:34Z

src/raw/array.rs

+    }
+}
+
+/// An iterator over borrwed raw BSON array values.


src/raw/error.rs

src/raw/mod.rs

src/raw/test/mod.rs

abr-egn

LGTM!

isabelatkinson · 2021-10-15T18:51:22Z

src/raw/doc.rs

+    /// Note that the internal structure of the bytes representing the
+    /// BSON elements is _not_ validated at all by this method. If the
+    /// bytes do not conform to the BSON spec, then method calls on
+    /// the RawDocument will return Errors where appropriate.


Suggested change

/// the RawDocument will return Errors where appropriate.

/// the RawDoc will return Errors where appropriate.

Fixed this and a few of the other non-linkified references to types in this file.

isabelatkinson · 2021-10-15T20:01:58Z

src/raw/array.rs

+    inner: Iter<'a>,
+}
+
+impl<'a> Iterator for RawArrIter<'a> {


One validation we're missing here is checking to make sure that the keys in the array are well-formed (i.e. incrementing integers starting with 0). This could lead to some confusing behavior in which malformed BSON bytes that include an "\x04" tag followed by a regular document are successfully interpreted as an array. I think we should add some checks here to ensure that the document we're dealing with is a proper array-style document.

Surprisingly, accepting array documents with messed up indexes is actually required by the bson corpus tests. e.g. the "Single Element array with index set incorrectly to ab" test is included under "valid": https://github.com/mongodb/specifications/blob/master/source/bson-corpus/tests/array.json#L23

Huh, that's interesting.

isabelatkinson · 2021-10-15T20:05:45Z

Cargo.toml

 uuid = { version = "0.8.1", features = ["serde", "v4"] }
+proptest = "0.10"


looks like this crate is now at 1.0.0 -- should we bump the dependency?

Good catch, fixed.

isabelatkinson

lgtm!

isabelatkinson · 2021-10-18T17:40:32Z

src/raw/array.rs

+    inner: Iter<'a>,
+}
+
+impl<'a> Iterator for RawArrIter<'a> {


Huh, that's interesting.

patrickfreed · 2021-10-22T22:04:17Z

Sounds good, went ahead and renamed RawDocument to RawDocumentBuf and RawDoc to RawDocument. I also renamed RawArr to RawArray and rexported all three types from lib.rs as part of that.

patrickfreed · 2021-10-25T22:55:54Z

Thanks again for spearheading this @jcdyer; we're super happy to have this finally merged!

jcdyer · 2021-10-26T19:01:08Z

@patrickfreed That's fantastic! Looking forward to seeing the new speediness in action.

jcdyer mentioned this pull request Feb 7, 2021

Support RawBson objects. #133

Closed

jcdyer commented Feb 9, 2021

View reviewed changes

saghm force-pushed the jcdyer/rawbson branch from b3888e6 to 2c30181 Compare February 10, 2021 19:10

saghm requested review from isabelatkinson and patrickfreed February 10, 2021 19:14

patrickfreed reviewed Feb 11, 2021

View reviewed changes

patrickfreed reviewed Feb 16, 2021

View reviewed changes

saghm reviewed Feb 16, 2021

View reviewed changes

patrickfreed reviewed Feb 17, 2021

View reviewed changes

isabelatkinson reviewed Feb 17, 2021

View reviewed changes

pooooodles changed the title ~~Incorporate rawbson code from rawbson = "0.2.1" in mod raw~~ RUST-284: Incorporate rawbson code from rawbson = "0.2.1" in mod raw Feb 18, 2021

pooooodles added the tracked-in-jira Ticket filed in Mongo's Jira system label Feb 18, 2021

pooooodles mentioned this pull request Apr 5, 2021

Add support for borrowed deserialization #231

Closed

patrickfreed force-pushed the jcdyer/rawbson branch from d6f1b7b to b552bb8 Compare October 4, 2021 23:53

patrickfreed reviewed Oct 13, 2021

View reviewed changes

src/raw/array.rs Outdated

}

}

/// An iterator over borrwed raw BSON array values.

Copy link

Contributor

patrickfreed Oct 6, 2021

Choose a reason for hiding this comment

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

fixed

patrickfreed requested review from isabelatkinson and abr-egn October 13, 2021 23:27

abr-egn reviewed Oct 14, 2021

View reviewed changes

src/raw/error.rs Outdated Show resolved Hide resolved

src/raw/mod.rs Outdated Show resolved Hide resolved

src/raw/test/mod.rs Outdated Show resolved Hide resolved

abr-egn approved these changes Oct 14, 2021

View reviewed changes

isabelatkinson reviewed Oct 15, 2021

View reviewed changes

isabelatkinson approved these changes Oct 18, 2021

View reviewed changes

src/raw/array.rs Outdated

inner: Iter<'a>,

}

impl<'a> Iterator for RawArrIter<'a> {

Copy link

Contributor

isabelatkinson Oct 18, 2021

Choose a reason for hiding this comment

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

Huh, that's interesting.

patrickfreed added 24 commits October 22, 2021 18:01

store the key in the error, reduce possibility of panics

7f641d8

wip struct -> enum

308e6d4

all but dbpointer done

24dc86a

implement dbpointer, cleanup

4e48b01

corpus wip

be6b9fe

finish corpus decode errors

bba68ed

wip typed helpers

ddd120a

finish typed helpers, add key to ValueAccessError, various cleanup

e59b0a7

rename RawDocumentRef to RawDoc, update documentation

e7ff8fb

rename RawArray to RawArr

69fc427

split code into different files

f67f9b1

various cleanup

f71b02e

add debug impl for rawdoc wrappers

1ef11d5

fix tests

164de7c

fix clippy

c064857

fix rustdoc

454c05a

fix test

a06068e

minor cleanup

2379298

test roundtrip

2220a3f

use map_err

bd2daee

use impl Intoi<String> in error constructor

617235d

fix up docstrings

82099e5

bump proptest to 1.0.0

260c245

rename RawDoc -> RawDocument, RawDocument -> RawDocumentBuf

eecedc8

patrickfreed force-pushed the jcdyer/rawbson branch from 007137f to eecedc8 Compare October 22, 2021 22:03

kmahar approved these changes Oct 24, 2021

View reviewed changes

patrickfreed merged commit a5c1e20 into mongodb:master Oct 25, 2021

	/// the RawDocument will return Errors where appropriate.
	/// the RawDoc will return Errors where appropriate.

		uuid = { version = "0.8.1", features = ["serde", "v4"] }
		proptest = "0.10"

RUST-284: Incorporate rawbson code from rawbson = "0.2.1" in mod raw #229

RUST-284: Incorporate rawbson code from rawbson = "0.2.1" in mod raw #229

Uh oh!

Conversation

jcdyer commented Feb 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saghm commented Feb 8, 2021

Uh oh!

jcdyer commented Feb 9, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcdyer Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

saghm commented Feb 10, 2021

Uh oh!

patrickfreed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcdyer commented Feb 14, 2021

RawDocument append

RawArray append

Non-size-changing element modification

General element modification / element replacement

Modifying a nested document:

Summary

Uh oh!

patrickfreed left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickfreed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcdyer commented Feb 19, 2021

Uh oh!

saghm commented Feb 19, 2021

Uh oh!

jcdyer commented Apr 14, 2021

Uh oh!

patrickfreed commented Apr 14, 2021

Uh oh!

patrickfreed commented Oct 5, 2021

Uh oh!

patrickfreed left a comment

jcdyer commented Feb 7, 2021 •

edited

Loading

jcdyer Feb 9, 2021 •

edited

Loading

patrickfreed left a comment •

edited

Loading