Adds `LazyRawTextReader` support for structs #619

zslayton · 2023-08-10T00:26:10Z

Builds on outstanding PRs #609, #612, #613, #614, #616, and #617.

Adds support for reading structs to the LazyRawTextReader.

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

codecov · 2023-08-10T00:29:48Z

Codecov Report

Patch coverage: 74.43% and no project coverage change.

Comparison is base (cb1042a) 81.64% compared to head (37a2a50) 81.65%.

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #619    +/-   ##
========================================
  Coverage   81.64%   81.65%            
========================================
  Files         120      122     +2     
  Lines       21876    22235   +359     
  Branches    21876    22235   +359     
========================================
+ Hits        17860    18155   +295     
- Misses       2366     2411    +45     
- Partials     1650     1669    +19

Files Changed	Coverage Δ
src/lazy/encoding.rs	`0.00% <ø> (ø)`
src/lazy/text/matched.rs	`67.32% <57.57%> (-2.49%)`	⬇️
src/lazy/decoder.rs	`60.00% <60.00%> (ø)`
src/lazy/text/encoded_value.rs	`70.54% <60.71%> (+5.78%)`	⬆️
src/lazy/text/raw/reader.rs	`90.28% <61.53%> (-2.70%)`	⬇️
src/lazy/text/raw/struct.rs	`68.57% <68.57%> (ø)`
src/lazy/text/raw/sequence.rs	`53.27% <80.95%> (+28.27%)`	⬆️
src/lazy/text/buffer.rs	`88.25% <85.71%> (-0.48%)`	⬇️
src/lazy/raw_value_ref.rs	`71.09% <100.00%> (ø)`
src/lazy/text/value.rs	`44.73% <100.00%> (+8.37%)`	⬆️

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

zslayton

🗺️ PR tour

zslayton · 2023-08-10T00:27:39Z

src/lazy/encoding.rs

🗺️ This file housed lots of ToDo* types that existed to satisfy the LazyDecoder trait. This PR replaces them with actual implementations.

zslayton · 2023-08-10T00:30:34Z

src/lazy/text/buffer.rs

+            map(
+                match_and_length(Self::match_struct),
+                |(matched_struct, length)| {
+                    EncodedTextValue::new(MatchedValue::Struct, matched_struct.offset(), length)


🗺️ This PR changes the parser to require that containers be completely buffered before reading begins. This means that all value types have the same behavior for both buffering and skipping. If we later determine that this is too restrictive, we can change it.

zslayton · 2023-08-10T00:31:27Z

src/lazy/text/buffer.rs

@@ -331,6 +411,74 @@ impl<'data> TextBufferView<'data> {
        .parse(self)
    }

+    /// Matches a list.


🗺️ Introduces a matching function for the list type that finds its end offset.

zslayton · 2023-08-10T00:31:35Z

src/lazy/text/buffer.rs

+        Ok((remaining, matched))
+    }
+
+    /// Matches a struct.


🗺️ Introduces a matching function for the struct type that finds its end offset.

zslayton · 2023-08-10T00:33:36Z

src/lazy/text/buffer.rs

-                MatchedString::Short(MatchedShortString::new(contains_escaped_chars))
+                if contains_escaped_chars {
+                    MatchedString::ShortWithEscapes
+                } else {
+                    MatchedString::ShortWithoutEscapes
+                }


🗺️ The MatchedShortString enum variant used to have a nested (bool) that indicated whether the string contained escapes. Per @popematt's feedback on #616, I've broken it into two variants with descriptive names.

zslayton · 2023-08-10T00:34:37Z

src/lazy/text/buffer.rs

@@ -715,7 +867,13 @@ impl<'data> TextBufferView<'data> {
    /// Matches a quoted symbol (`'foo'`).
    fn match_quoted_symbol(self) -> IonParseResult<'data, MatchedSymbol> {
        delimited(char('\''), Self::match_quoted_symbol_body, char('\''))
-            .map(|(_matched, contains_escaped_chars)| MatchedSymbol::Quoted(contains_escaped_chars))


🗺️ As above, I've broken out the nested (bool) on the MatchedSymbol variant.

zslayton · 2023-08-10T00:36:47Z

src/lazy/text/encoded_value.rs

@@ -86,7 +91,13 @@ impl EncodedTextValue {
    //   'foo'
    //   "foo"
    //    $10
-    pub(crate) fn with_field_name(mut self, offset: usize, length: usize) -> EncodedTextValue {


🗺️ This now stores the MatchedSymbol, which indicates which symbol syntax was found in the input. (Quoted, identifier, symbol ID)

zslayton · 2023-08-10T00:40:19Z

src/lazy/text/value.rs

    fn read(&self) -> IonResult<RawValueRef<'data, TextEncoding>> {
-        let matched_input = self.input.slice(0, self.encoded_value.data_length());
+        let matched_input = self.input.slice(
+            self.encoded_value.data_offset() - self.input.offset(),
+            self.encoded_value.data_length(),
+        );


🗺️ Prior to this PR, the matched input slice always started with the value. Now that we have struct fields, it sometimes begins at the field name.

popematt · 2023-08-21T20:10:59Z

src/lazy/text/buffer.rs

+
+    /// Matches any amount of whitespace followed by a closing `}`.
+    fn match_struct_end(self) -> IonMatchResult<'data> {
+        whitespace_and_then(peek(tag("}"))).parse(self)


Does whitespace_and_then also account for comments?

Yes, it does. I wanted a more explicit name but couldn't get it short enough. ws_and_comments_and_then?

I'm ambivalent about the name. I was just looking to confirm my assumption about its behavior.

popematt · 2023-08-21T20:17:41Z

src/lazy/text/encoded_value.rs

@@ -60,6 +62,8 @@ pub(crate) struct EncodedTextValue {
    // value is stored. For others (e.g. a timestamp), the various components of the value are
    // recognized during matching and partial information like subfield offsets can be stored here.
    matched_value: MatchedValue,
+
+    field_name_syntax: Option<MatchedSymbol>,


Doc comment?

Did you consider creating a LazyRawTextSymbolToken or similar to represent annotations and field names? What made you decide to use the approach that you did?

Further down, I see that there is a LazyRawTextField. Is there any way we could put field name data (length, offset, etc) in that struct (or something analogous) rather than in EncodedTextValue?

Will do.

I'm not sure I understand the suggestion, but I can explain the approach. Both the binary version and the text version model their lazy values as a (field_id?, annotations?, value) tuple. That is, the field is modeled as being part of the value. (This language is loose; the data structures and approach to parsing couple the field with the value but I recognize that "the field is part of the value" doesn't feel quite right from an Ion data model perspective.) If I did introduce a LazyRawTextSymbolToken type, it would simply wrap a MatchedSymbol. Let me know if you'd like to discuss this further, I think an offline discussion would probably be helpful.

I think it's possible, yeah. Might even be a small performance win by shrinking the size of values that aren't in structs. I've opened Explore moving struct field info out of Lazy*Value and into Lazy*Field #631 to track investigating this.

Opened #632 to track investigating an optimized representation for annotations offset info.

popematt · 2023-08-21T21:14:56Z

src/lazy/text/raw/reader.rs

+            // Quoted symbol
+            'bar': 200,
+            // Short-form string
+            "baz": 300


Would be good to have some comments in unexpected (but legal) locations.

Suggested change

"baz": 300

"baz": 300,

/* Comments... */ qux /* comments */ : /* are */ 400 /* everywhere */,

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

Zack Slayton added 14 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

zslayton mentioned this pull request Aug 10, 2023

Adds LazyRawTextReader support for reading symbols #616

Merged

zslayton commented Aug 10, 2023

View reviewed changes

zslayton marked this pull request as ready for review August 10, 2023 00:40

zslayton requested review from desaikd and jobarr-amzn August 10, 2023 00:40

More doc comments

4fc9078

This was referenced Aug 10, 2023

Adds LazyRawTextReader support for reading IVMs #620

Merged

Initial impl of a LazyRawAnyReader #621

Merged

Adds lazy reader support for reading annotations #622

Merged

Adds lazy reader support for timestamps #623

Merged

This was referenced Aug 18, 2023

Lazy reader support for s-expressions #627

Merged

Adds lazy reader support for decimals #628

Merged

Adds lazy reader support for blobs #629

Merged

Adds lazy reader support for long strings #630

Merged

popematt reviewed Aug 21, 2023

View reviewed changes

zslayton mentioned this pull request Aug 21, 2023

Adds LazyRawTextReader support for reading strings #614

Merged

popematt approved these changes Aug 22, 2023

View reviewed changes

Base automatically changed from lazy-lists to main August 23, 2023 00:44

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

37a2a50

zslayton merged commit 998a5bf into main Aug 23, 2023
17 of 18 checks passed

zslayton deleted the lazy-structs branch August 23, 2023 03:18

zslayton self-assigned this Aug 29, 2023

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

Feedback from PR #619

f7fdc51

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

Adds `LazyRawTextReader` support for structs #619

Adds `LazyRawTextReader` support for structs #619

zslayton commented Aug 10, 2023

codecov bot commented Aug 10, 2023 •

edited

zslayton left a comment

zslayton Aug 10, 2023

zslayton Aug 10, 2023

zslayton Aug 10, 2023

zslayton Aug 10, 2023

zslayton Aug 10, 2023

zslayton Aug 10, 2023

zslayton Aug 10, 2023

zslayton Aug 10, 2023

popematt Aug 21, 2023

zslayton Aug 22, 2023

popematt Aug 22, 2023

popematt Aug 21, 2023

zslayton Aug 22, 2023

zslayton Aug 22, 2023

popematt Aug 21, 2023

	"baz": 300
	"baz": 300,
	/* Comments... / qux / comments / : / are / 400 / everywhere */,

Adds LazyRawTextReader support for structs #619

Adds LazyRawTextReader support for structs #619

Conversation

zslayton commented Aug 10, 2023

codecov bot commented Aug 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adds `LazyRawTextReader` support for structs #619

Adds `LazyRawTextReader` support for structs #619

codecov bot commented Aug 10, 2023 •

edited