Adds traits for the lazy reader API to abstract over format #596

zslayton · 2023-07-10T21:26:35Z

The lazy reader API introduced in #546 only supports binary Ion. In order to eventually support text Ion alongside binary, this PR adds a new family of traits (LazyFormat) and makes the existing binary reader API an implementation of those traits. Providing a text implementation of the traits will happen in one or more follow-on PRs.

The LazyFormat trait requires implementations to specify each type they will use to represent various parts of the lazy reader API.

pub trait LazyFormat<'data>: Sized + Debug + Clone {
    type Reader: LazyRawReader<'data, Self>;
    type Sequence: LazyRawSequence<'data, Self>;
    type Struct: LazyRawStruct<'data, Self>;
    type Value: LazyRawValue<'data, Self>;
    type AnnotationsIterator: Iterator<Item = IonResult<RawSymbolTokenRef<'data>>>;
}

Types that do not specify a format name (for example: LazyValue) have been renamed to reflect that they are specific to binary (now: LazyBinaryValue). The format-agnostic names are now used for the implementation that is generic over a LazyFormat impl.

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

codecov · 2023-07-10T21:36:49Z

Codecov Report

Patch coverage: 72.80% and project coverage change: -0.14 ⚠️

Comparison is base (93dce09) 82.53% compared to head (11664eb) 82.39%.

❗ Current head 11664eb differs from pull request most recent head 5347a4a. Consider uploading reports for the commit 5347a4a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #596      +/-   ##
==========================================
- Coverage   82.53%   82.39%   -0.14%     
==========================================
  Files         110      111       +1     
  Lines       19486    19550      +64     
  Branches    19486    19550      +64     
==========================================
+ Hits        16082    16108      +26     
- Misses       1861     1899      +38     
  Partials     1543     1543

Impacted Files	Coverage Δ
src/lazy/binary/encoded_value.rs	`93.33% <ø> (ø)`
src/lazy/binary/format.rs	`0.00% <0.00%> (ø)`
src/lazy/raw_stream_item.rs	`52.94% <ø> (ø)`
src/lazy/system_stream_item.rs	`0.00% <ø> (ø)`
src/lazy/value_ref.rs	`73.79% <33.33%> (ø)`
src/lazy/binary/raw/lazy_raw_sequence.rs	`60.93% <60.00%> (-6.41%)`	⬇️
src/lazy/struct.rs	`66.66% <60.86%> (ø)`
src/lazy/binary/raw/struct.rs	`62.22% <62.22%> (ø)`
src/lazy/sequence.rs	`73.07% <62.50%> (ø)`
src/lazy/reader.rs	`66.66% <64.28%> (ø)`
... and 7 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

zslayton

🗺️ PR tour

zslayton · 2023-07-11T15:00:27Z

examples/lazy_read_all_values.rs

-    use ion_rs::lazy::binary::lazy_reader::LazyReader;
-    use ion_rs::lazy::binary::system::lazy_sequence::LazySequence;
-    use ion_rs::lazy::binary::system::lazy_struct::LazyStruct;
-    use ion_rs::lazy::binary::system::lazy_value::LazyValue;
+    use ion_rs::lazy::r#struct::LazyBinaryStruct;
+    use ion_rs::lazy::reader::LazyBinaryReader;
+    use ion_rs::lazy::sequence::LazyBinarySequence;
+    use ion_rs::lazy::value::LazyBinaryValue;


🗺️ This file (lazy_read_all_values) is an example program that demonstrates the lazy reader API. We currently only have a binary implementation, so the names of the various types have been changed to reflect that.

The names that don't mention a format are generic, while the names that do are just type aliases over a specific implementation of that generic type.

For example, LazyValue is generic over a format:

pub struct LazyValue<'top, 'data, F: LazyFormat<'data>> { pub(crate) raw_value: F::Value, pub(crate) symbol_table: &'top SymbolTable, }

while LazyBinaryValue is a type alias over a specific implementation of LazyValue:

pub type LazyBinaryValue<'top, 'data> = LazyValue<'top, 'data, BinaryFormat>;

zslayton · 2023-07-11T15:04:07Z

src/lazy/binary/format.rs

🗺️ An implementation of the LazyFormat trait is just a collection of concrete types that collectively form an API for that format.

I'm open to better names than LazyFormat/BinaryFormat.

LazyDecoder for BinaryEncoding? I don't know. 🤷‍♂️

zslayton · 2023-07-11T15:06:30Z

src/lazy/binary/raw/lazy_raw_sequence.rs

+impl<'data> LazyContainerPrivate<'data, BinaryFormat> for LazyRawBinarySequence<'data> {
+    fn from_value(value: LazyRawBinaryValue<'data>) -> Self {
+        LazyRawBinarySequence { value }
+    }
+}


🗺️ There are a few *Private traits that house implementation details we need but do not wish to expose to users. See the src/lazy/format.rs file for an explanation of how this works.

zslayton · 2023-07-11T15:08:14Z

src/lazy/binary/raw/lazy_raw_struct.rs

🗺️ This functionality was moved, not deleted. However, LazyRawStruct was also renamed to LazyRawBinaryStruct, so it seems that git couldn't keep track.

zslayton · 2023-07-11T15:10:06Z

src/lazy/binary/raw/mod.rs

🗺️ Some of the module names in the hierarchy were getting pretty repetitive. I removed name elements that were already reflected in parent modules. e.g. crate::lazy::binary::raw::lazy_raw_reader became crate::lazy::binary::raw::reader.

zslayton · 2023-07-11T15:12:04Z

src/lazy/binary/raw/struct.rs

🗺️ As called out prior, git thought this was file was deleted but it was just moved and its types renamed.

zslayton · 2023-07-11T15:12:41Z

src/lazy/binary/raw/value.rs

+    fn ion_type(&self) -> IonType {
+        self.ion_type()
+    }
+
+    fn annotations(&self) -> RawBinaryAnnotationsIterator<'data> {
+        self.annotations()
+    }
+
+    fn read(&self) -> IonResult<RawValueRef<'data, BinaryFormat>> {
+        self.read()
+    }


🗺️ These impls just forward the method call to the inherent implementation on the type.

zslayton · 2023-07-11T15:13:22Z

src/lazy/binary/system/mod.rs

🗺️ The 'system' types no longer live in the binary module, as the system level is format-agnostic.

zslayton · 2023-07-11T15:15:26Z

src/lazy/reader.rs

 }

-impl<'data> LazyReader<'data> {
-    pub fn new(ion_data: &'data [u8]) -> IonResult<LazyReader<'data>> {


🗺️ As LazyReader is now generic over a LazyFormat, the new() method now lives in the concretely-typed LazyBinaryReader below.

zslayton · 2023-07-11T15:16:36Z

src/lazy/sequence.rs

            symbol_table: self.symbol_table,
-            initial_offset: self


🗺️ The initial_offset member field was a copy/pasted holdover from the raw implementation. I've removed it in each of the user-level types that had erroneously retained it.

popematt · 2023-07-11T17:15:22Z

src/lazy/binary/raw/mod.rs

-pub mod lazy_raw_value;
-pub mod raw_annotations_iterator;
+pub mod reader;
+pub mod r#struct;


I suppose you could call it strukt. (But actually, please don't.)

popematt · 2023-07-11T17:34:00Z

src/lazy/binary/format.rs

LazyDecoder for BinaryEncoding? I don't know. 🤷‍♂️

popematt · 2023-07-11T17:48:14Z

examples/lazy_read_all_values.rs

@@ -41,7 +41,7 @@ mod lazy_reader_example {
        // We're going to recursively visit and read every value in the input stream, counting
        // them as we go.
        let mut count = 0;
-        let mut reader = LazyReader::new(ion_data)?;
+        let mut reader = LazyBinaryReader::new(ion_data)?;


Is the user required to know whether the data is binary or text ahead of time? (Or this just a temporary thing because the lazy text reader is not implemented yet?)

(Or this just a temporary thing because the lazy text reader is not implemented yet?)

This one!

zslayton added 3 commits July 10, 2023 17:16

added traits for lazy API to abstract over format

cd343b1

Hides trait impl details in private module

60b9c32

Fixed private doc links

b0a7504

zslayton added 3 commits July 10, 2023 17:54

Removed unused import from example

01783aa

Reorganized modules

6b42d03

Fix internal doc links

80844e6

zslayton marked this pull request as ready for review July 11, 2023 14:54

zslayton commented Jul 11, 2023

View reviewed changes

zslayton requested review from almann and popematt July 11, 2023 15:17

popematt reviewed Jul 11, 2023

View reviewed changes

zslayton and others added 2 commits July 11, 2023 14:45

Merge branch 'main' into lazy-traits

11664eb

Renames LazyFormat to LazyDecoder, BinaryFormat -> BinaryEncoding

5347a4a

popematt approved these changes Jul 11, 2023

View reviewed changes

zslayton merged commit 420a276 into main Jul 11, 2023

zslayton deleted the lazy-traits branch July 11, 2023 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds traits for the lazy reader API to abstract over format #596

Adds traits for the lazy reader API to abstract over format #596

zslayton commented Jul 10, 2023 •

edited

Loading

codecov bot commented Jul 10, 2023 •

edited

Loading

zslayton left a comment

zslayton Jul 11, 2023

zslayton Jul 11, 2023

popematt Jul 11, 2023

zslayton Jul 11, 2023

zslayton Jul 11, 2023

zslayton Jul 11, 2023

zslayton Jul 11, 2023

zslayton Jul 11, 2023

zslayton Jul 11, 2023

zslayton Jul 11, 2023

zslayton Jul 11, 2023

popematt Jul 11, 2023

popematt Jul 11, 2023

popematt Jul 11, 2023

zslayton Jul 11, 2023

Adds traits for the lazy reader API to abstract over format #596

Adds traits for the lazy reader API to abstract over format #596

Conversation

zslayton commented Jul 10, 2023 • edited Loading

codecov bot commented Jul 10, 2023 • edited Loading

Codecov Report

zslayton 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

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

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

zslayton commented Jul 10, 2023 •

edited

Loading

codecov bot commented Jul 10, 2023 •

edited

Loading