Lazy reader support for s-expressions #627

zslayton · 2023-08-18T19:33:36Z

Builds on outstanding PRs #612, #613, #614, #616, #617, #619, #620, #621, #622, and #623.

Adds support for reading text s-expressions in the lazy reader.

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

codecov · 2023-08-18T19:36:47Z

Codecov Report

Patch coverage: 61.09% and project coverage change: -0.25% ⚠️

Comparison is base (eda4e2b) 81.54% compared to head (f935c64) 81.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #627      +/-   ##
==========================================
- Coverage   81.54%   81.30%   -0.25%     
==========================================
  Files         123      123              
  Lines       23158    23446     +288     
  Branches    23158    23446     +288     
==========================================
+ Hits        18885    19062     +177     
- Misses       2549     2649     +100     
- Partials     1724     1735      +11

Files Changed	Coverage Δ
src/lazy/decoder.rs	`60.00% <ø> (ø)`
src/lazy/encoding.rs	`0.00% <ø> (ø)`
src/lazy/raw_value_ref.rs	`71.67% <ø> (ø)`
src/lazy/text/encoded_value.rs	`75.38% <0.00%> (-0.59%)`	⬇️
src/lazy/value_ref.rs	`74.07% <ø> (ø)`
src/lazy/binary/raw/sequence.rs	`45.34% <32.35%> (-14.66%)`	⬇️
src/lazy/any_encoding.rs	`42.47% <38.35%> (-6.91%)`	⬇️
src/lazy/text/raw/sequence.rs	`53.07% <42.85%> (-0.21%)`	⬇️
src/lazy/sequence.rs	`69.78% <65.33%> (-3.30%)`	⬇️
src/lazy/text/raw/reader.rs	`87.74% <75.00%> (-0.91%)`	⬇️
... and 7 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zslayton

🗺️ PR tour

zslayton · 2023-08-18T19:34:44Z

examples/lazy_read_all_values.rs

🗺️ cargo fmt rearranged the imports a bit in this file.

zslayton · 2023-08-18T19:40:08Z

examples/lazy_read_all_values.rs

+            List(s) => count_sequence_children(s.iter())?,
+            SExp(s) => count_sequence_children(s.iter())?,


🗺️ In binary Ion, the parsing logic for the bodies of lists and for s-expressions is identical. However, in text the parsing is substantially different. Not only do they have different delimiters for opening ([ vs (), closing (] vs ), and between values (, vs whitespace-or-nothing), but s-expressions also allow for the special grammar production of operators.

In order to accommodate these differences without introducing runtime overhead via branching or dynamic dispatch, I had to breaking the LazySequence type into LazyList and LazySExp types that could house their own logic. This change was also made in the raw level and accounts for the majority of this diff.

Meanwhile, the Sequence type is still unified in the Element API. I think this is reasonable, as the Element API is intentionally divorced from the reading logic needed to deserialize a stream into Elements.

zslayton · 2023-08-18T19:41:40Z

examples/lazy_read_all_values.rs

-    fn count_sequence_children(lazy_sequence: &LazyBinarySequence) -> IonResult<usize> {
+    fn count_sequence_children<'a, 'b>(
+        lazy_sequence: impl Iterator<Item = IonResult<LazyBinaryValue<'a, 'b>>>,
+    ) -> IonResult<usize> {


🗺️ This example program takes an iterator to generically handle either a list or sexp. We can always introduce a LazySequence trait later if desired.

zslayton · 2023-08-18T19:43:25Z

src/lazy/any_encoding.rs

🗺️ All of the changes in this file come from splitting the LazyRawAnySequence type into LazyRawAnyList and LazyRawAnySExp. There is no interesting new logic.

zslayton · 2023-08-18T19:44:48Z

src/lazy/binary/raw/reader.rs

@@ -188,7 +188,7 @@ mod tests {
        let lazy_list = reader.next()?.expect_value()?.read()?.expect_list()?;
        // Exercise the `Debug` impl
        println!("Lazy Raw Sequence: {:?}", lazy_list);
-        let mut list_values = lazy_list.iter();
+        let mut list_values = lazy_list.sequence.iter();


🗺️ In the binary reader, the LazyRawBinaryList and LazyRawBinarySExp types each wrap an instance of a LazyRawBinarySequence helper type that houses their shared parsing logic.

zslayton · 2023-08-18T19:48:30Z

src/lazy/sequence.rs

🗺️ The changes in this file all come from splitting the LazySequence type into distinct LazyList and LazySExp types. There is no interesting new logic.

zslayton · 2023-08-18T19:49:40Z

src/lazy/sequence.rs

-        match self.ion_type() {
-            IonType::SExp => {
-                write!(f, "(")?;
-                for value in self {
-                    write!(
-                        f,
-                        "{:?} ",
-                        value
-                            .map_err(|_| fmt::Error)?
-                            .read()
-                            .map_err(|_| fmt::Error)?
-                    )?;
-                }
-                write!(f, ")")?;
-            }
-            IonType::List => {
-                write!(f, "[")?;
-                for value in self {
-                    write!(
-                        f,
-                        "{:?},",
-                        value
-                            .map_err(|_| fmt::Error)?
-                            .read()
-                            .map_err(|_| fmt::Error)?
-                    )?;
-                }
-                write!(f, "]")?;
-            }
-            _ => unreachable!("LazySequence is only created for list and sexp"),


🗺️ This removal gives a taste of the sort of split logic a unified LazySequence would need to house at each level of abstraction. Now the List and SExp types can simply house their respective logic.

zslayton · 2023-08-18T19:55:11Z

src/lazy/text/buffer.rs

+    /// Matches an optional annotation sequence and a trailing value.
+    pub fn match_annotated_value(self) -> IonParseResult<'data, LazyRawTextValue<'data>> {
+        pair(
+            opt(Self::match_annotations),
+            whitespace_and_then(Self::match_value),
+        )
+        .map(|(maybe_annotations, mut value)| {
+            if let Some(annotations) = maybe_annotations {
+                value.encoded_value = value
+                    .encoded_value
+                    .with_annotations_sequence(annotations.offset(), annotations.len());
+                // Rewind the value's input to include the annotations sequence.
+                value.input = self.slice_to_end(annotations.offset() - self.offset());
+            }
+            value
+        })
+        .parse(self)
+    }
+


🗺️ This method was moved, not changed. You'll see the removal immediately following.

zslayton · 2023-08-18T19:59:05Z

src/lazy/text/raw/sequence.rs

🗺️ Half of the changes in this file come from changind LazyRawTextSequence into LazyRawTextList. The other half come from introducing the new LazyRawTextSExp type; these contain interesting new logic. I'll call out where the new logic begins.

zslayton · 2023-08-18T20:00:17Z

src/lazy/text/raw/sequence.rs

@@ -171,6 +146,139 @@ impl<'data> Iterator for RawTextSequenceIterator<'data> {
    }
 }

+// ===== S-Expressions =====


🗺️ New logic begins here.

popematt · 2023-08-22T02:47:35Z

src/lazy/text/buffer.rs

+            "(a)",
+            "(a b)",
+            "(a++)",
+            "(++a)",


Test case with operator with different characters in it.

Suggested change

"(++a)",

"(++a)",

"(a+=b)",

popematt · 2023-08-22T02:53:07Z

src/lazy/text/matched.rs

 }

 impl MatchedSymbol {
    pub fn read<'data>(
        &self,
        matched_input: TextBufferView<'data>,
    ) -> IonResult<RawSymbolTokenRef<'data>> {
+        use MatchedSymbol::*;


Having a local use right before the match expression really cleans it up a lot.

This is something new-ish for you, I think. I've noticed this in a few places in the lazy reader PRs, but I don't remember seeing it when I was working on Ion Rust 6-9 months ago. Anyway, it's very nice, and I'm going to be copying this from you.

Feedback from PRs: * #609 * #614 * #616 * #619 * #620 * #627 * #628 * #638 * #639

Zack Slayton added 21 commits July 24, 2023 16:54

Top-level nulls, bools, ints

e0a83d8

Consolidate impls of AsUtf8 w/helper fn

89f79aa

Improved TextBufferView docs, removed DataSource

840be4d

Adds lazy text floats

5db1ff0

Adds LazyRawTextReader support for comments

07d4a70

Adds LazyRawTextReader support for reading strings

181e0a5

clippy fixes

357ca8f

Fix a couple of unit tests

716ff34

Less ambitious float eq comparison

e29fec5

Adds LazyRawTextReader support for reading symbols

8f79a36

Adds more doc comments

4cb9b2b

More doc comments

54470d2

Adds LazyRawTextReader support for reading lists

78014e7

Adds LazyRawTextReader support for structs

a6a3aa8

More doc comments

4fc9078

Adds LazyRawTextReader support for reading IVMs

11174ac

Initial impl of a LazyRawAnyReader

719dbaa

Improved comments.

f603872

Adds LazyRawTextReader support for annotations

4696ca5

Adds lazy reader support for timestamps

c7129ac

Lazy reader support for s-expressions

44435ea

zslayton commented Aug 18, 2023

View reviewed changes

Fixed doc comments

d50e05b

zslayton marked this pull request as ready for review August 18, 2023 20:02

zslayton requested review from popematt and desaikd August 18, 2023 20:02

Fix internal doc link

8283422

This was referenced Aug 19, 2023

Adds lazy reader support for decimals #628

Merged

Adds lazy reader support for blobs #629

Merged

zslayton mentioned this pull request Aug 21, 2023

Adds lazy reader support for long strings #630

Merged

popematt approved these changes Aug 22, 2023

View reviewed changes

Zack Slayton added 4 commits August 23, 2023 10:23

Merge remote-tracking branch 'origin/main' into lazy-timestamps

4b53bb3

Incorporates review feedback

60d5a17

Matcher recognizes +00:00 as Zulu

db9718d

Merge remote-tracking branch 'origin/lazy-timestamps' into lazy-sexps

37264a3

Base automatically changed from lazy-timestamps to main August 28, 2023 16:34

Merge remote-tracking branch 'origin/main' into lazy-sexps

f935c64

zslayton self-assigned this Aug 29, 2023

zslayton merged commit efd90a7 into main Sep 1, 2023
17 of 18 checks passed

zslayton deleted the lazy-sexps branch September 1, 2023 14:54

zslayton pushed a commit that referenced this pull request Sep 7, 2023

Feedback from PR #627

04e7d85

zslayton mentioned this pull request Sep 7, 2023

Incorporates pending feedback from lazy reader PRs #642

Merged

zslayton added a commit that referenced this pull request Sep 7, 2023

Incorporates pending feedback from lazy reader PRs (#642)

ec91888

Feedback from PRs: * #609 * #614 * #616 * #619 * #620 * #627 * #628 * #638 * #639

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy reader support for s-expressions #627

Lazy reader support for s-expressions #627

zslayton commented Aug 18, 2023

codecov bot commented Aug 18, 2023 •

edited

zslayton left a comment

zslayton Aug 18, 2023

zslayton Aug 18, 2023

zslayton Aug 18, 2023

zslayton Aug 18, 2023

zslayton Aug 18, 2023

zslayton Aug 18, 2023

zslayton Aug 18, 2023

zslayton Aug 18, 2023

zslayton Aug 18, 2023

zslayton Aug 18, 2023

popematt Aug 22, 2023

popematt Aug 22, 2023

		List(s) => count_sequence_children(s.iter())?,
		SExp(s) => count_sequence_children(s.iter())?,

Lazy reader support for s-expressions #627

Lazy reader support for s-expressions #627

Conversation

zslayton commented Aug 18, 2023

codecov bot commented Aug 18, 2023 • edited

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

codecov bot commented Aug 18, 2023 •

edited