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

Introduces Bytes, Blob, and Clob wrapper types #506

Merged
merged 3 commits into from
Apr 5, 2023
Merged

Introduces Bytes, Blob, and Clob wrapper types #506

merged 3 commits into from
Apr 5, 2023

Conversation

zslayton
Copy link
Contributor

@zslayton zslayton commented Apr 5, 2023

This PR introduces new opaque wrappers for Vec<u8>, which was used throughout the APIs to represent owned blobs and clobs.

Bytes captures the core functionality of an owned, immutable byte array, but does not include Ion type information.

Blob and Clob are both trivial tuple struct wrappers around Bytes, adding an Ion type to the data.

The Value::Blob and Value::Clob enum variants (which already provide an Ion type) now wrap an instance of Bytes.

This PR fixes #500.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Zack Slayton added 2 commits April 5, 2023 09:35
This PR introduces new opaque wrappers for `Vec<u8>`, which was used
throughout APIs to represent owned blobs and clobs.

`Bytes` captures the core functionality of an owned byte array, but
does not include Ion type information.

`Blob` and `Clob` are both trivial tuple struct wrappers around
`Bytes`, adding an Ion type to the data.

The `Value::Blob` and `Value::Clob` enum variants (which already
provide an Ion type) now wrap an instance of `Bytes`.

This PR fixes #500.
Copy link
Contributor Author

@zslayton zslayton left a comment

Choose a reason for hiding this comment

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

🗺️ PR tour

fn read_blob(&mut self) -> IonResult<Vec<u8>> {
self.read_blob_bytes().map(Vec::from)
fn read_blob(&mut self) -> IonResult<Blob> {
self.read_blob_bytes().map(Vec::from).map(Blob::from)
Copy link
Contributor Author

@zslayton zslayton Apr 5, 2023

Choose a reason for hiding this comment

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

🗺️ The IonReader trait's blob- and clob-related methods had to be updated to return Blob and Clob.

Before:

fn read_blob(&mut self) -> IonResult<Vec<u8>>;
fn read_clob(&mut self) -> IonResult<Vec<u8>>;

After:

fn read_blob(&mut self) -> IonResult<Blob>;
fn read_clob(&mut self) -> IonResult<Clob>;

This change had to be made in each implementation of IonReader, of which there are several.

}
}

impl AsRef<[u8]> for Blob {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ There are a lot of trait impls here that feel repetitive. I'm confident that we can represent them more concisely if needed (perhaps via a macro or a Lob trait with blanket implementations), but I want to leave that for a future PR.

Blob(Vec<u8>),
Clob(Vec<u8>),
Blob(Bytes),
Clob(Bytes),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ Notice that we can still handle blob and clob values in a single match/if let arm:

if let Blob(b) | Clob(b) = value {
  // ...do stuff with `b`...
}

Comment on lines +188 to 199
impl From<Blob> for Value {
fn from(blob: Blob) -> Self {
let bytes: Bytes = blob.into();
Value::Blob(bytes)
}
}

impl From<Clob> for Value {
fn from(clob: Clob) -> Self {
let bytes: Bytes = clob.into();
Value::Clob(bytes)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ The standalone Blob and Clob types can be trivially widened to Value.

Comment on lines +150 to +151
Clob => Value::Clob(self.reader.read_clob()?.into()),
Blob => Value::Blob(self.reader.read_blob()?.into()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ The .into() here is converting the lobs into their inner Bytes representations. From a memory layout perspective, this is a no-op as Bytes is the only field in Blob/Clob.

@zslayton zslayton merged commit 6e14332 into main Apr 5, 2023
@zslayton zslayton deleted the bytes-type branch April 5, 2023 16:15
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.

Value/Content APIs Factoring to Capture Structurally Similar Content Types
2 participants