Add support for delimited containers to the 1.1 binary reader #787

nirosys · 2024-06-07T14:09:16Z

Issue #, if available: #665

Description of changes:
This PR adds delimited container support for the binary 1.1 reader.

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

nirosys · 2024-06-17T19:14:17Z

Found an issue while rebasing that's causing unit tests to fail. Fixing that before opening this up to review.

codecov · 2024-06-18T23:43:22Z

Codecov Report

Attention: Patch coverage is 77.92553% with 83 lines in your changes missing coverage. Please review.

Project coverage is 76.98%. Comparing base (fa3167d) to head (fd917bd).
Report is 75 commits behind head on main.

❗ Current head fd917bd differs from pull request most recent head f89c91f

Please upload reports for the commit f89c91f to get more accurate results.

Files	Patch %	Lines
src/lazy/binary/raw/v1_1/reader.rs	71.64%	0 Missing and 38 partials ⚠️
src/lazy/binary/raw/v1_1/immutable_buffer.rs	82.03%	7 Missing and 16 partials ⚠️
src/lazy/binary/raw/v1_1/struct.rs	86.15%	6 Missing and 3 partials ⚠️
src/lazy/binary/raw/v1_1/sequence.rs	75.75%	8 Missing ⚠️
src/lazy/binary/raw/v1_1/type_descriptor.rs	44.44%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #787      +/-   ##
==========================================
- Coverage   80.60%   76.98%   -3.63%     
==========================================
  Files         140      131       -9     
  Lines       28209    28369     +160     
  Branches    28209    28369     +160     
==========================================
- Hits        22737    21839     -898     
- Misses       3641     4754    +1113     
+ Partials     1831     1776      -55

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

zslayton · 2024-06-21T14:14:45Z

src/lazy/binary/raw/v1_1/immutable_buffer.rs

+        if opcode.is_delimited() {
+            self.peek_delimited_container(opcode)
+        } else {


The peek_delimited_sequence and peek_delimited_struct methods reproduce a lot of the code below this point. Would it be possible to refactor them to return their expression cache and the length consumed so they could still share the EncodedValue construction code in this method?

Missed this comment in the previous commit, following up quick to address it.

zslayton · 2024-06-21T14:15:06Z

src/lazy/binary/raw/v1_1/immutable_buffer.rs

+            } else if let Some(value) = ImmutableBuffer::peek_sequence_value_expr(input)? {
+                offsets.push(input.offset());
+                input = input.consume(value.range().len());
+            }


peek_sequence_value_expr(input) is fully lexing the value, is there a reason you decided to store the offset rather than the LazyRawValueExpr (value) itself?

I had a lifetime issue with the values when I first started working on it. Now that I think about it though, I had some mismatched lifetimes that I ended up fixing later that could have been the problem. I'l revisit.

zslayton · 2024-06-21T14:19:40Z

src/lazy/binary/raw/v1_1/sequence.rs

+            if offsets.len() <= 1 {
+                None
+            } else {
+                let offset = offsets.first().unwrap(); // Safety: Already tested that there's > 1 item.


To flatten the method a bit, you could do:

let offset = offsets.first()?; // <-- Early returns on `None`.

zslayton · 2024-06-21T14:22:01Z

src/lazy/binary/raw/v1_1/struct.rs

+            FlexSymValue::Opcode(Opcode{ opcode_type: OpcodeType::DelimitedContainerClose, ..}) => {
+                return Ok(None)
+            }


Slightly more concise:

FlexSymValue::Opcode(o) if o.opcode_type == DelimitedContainerClose => {...}

Or it might be nice to have an is_delimited_end() helper method to complement the is_delimited_container() method?

Opcode(o) if o.is_delimited_end() => {...},

zslayton · 2024-06-21T14:23:42Z

src/lazy/binary/raw/v1_1/struct.rs

+enum SymAddressFieldName<'top> {
+    ModeChange,
+    FieldName(LazyRawBinaryFieldName_1_1<'top>),
+}


zslayton · 2024-06-21T14:28:53Z

src/lazy/binary/raw/v1_1/struct.rs

+    ) -> IonResult<Option<(LazyRawFieldExpr<'top, BinaryEncoding_1_1>, usize)>> {
+        let mut buffer = input;


Could/should this method return a ParseResult<LazyRawFieldExpr<_>> instead so the caller gets the remaining input? (And can deduce the length as desired.)

EDIT: I see now that this is being used to populate the offsets cache. I think this is another place where it would make sense to store the LazyRawFieldExpr in the cache instead of just the offset to avoid later rework.

zslayton · 2024-06-21T14:30:37Z

src/lazy/binary/raw/v1_1/struct.rs

+            if offsets.len() <= 1 {
+                None
+            } else {
+                let offset = offsets.first().unwrap();


let offset = offsets.first()?;

zslayton · 2024-06-21T14:34:40Z

src/lazy/binary/raw/v1_1/type_descriptor.rs

+            (0xF, 0x0) => (DelimitedContainerClose, low_nibble, None),
+            (0xF, 0x1) => (ListDelimited, low_nibble, Some(IonType::List)),
+            (0xF, 0x2) => (SExpressionDelimited, low_nibble, Some(IonType::SExp)),
+            (0xF, 0x3) => (StructDelimited, low_nibble, Some(IonType::Struct)),


I really like this jump table. I've found myself wondering if we should add a higher-level OpcodeKind enum alongside the OpcodeType that captures some of the natural groupings that currently require testing for several OpcodeType variants. For example, having:

DelimitedContainer for 0xF1/0xF2/0xF3

EExp for 0x00 .. 0x5F

etc

No TODO here, just thinking out loud.

…ypes; Fix delimited struct delimiters

…lify some expressions

zslayton · 2024-07-12T16:16:30Z

src/lazy/binary/raw/v1_1/reader.rs

@@ -712,6 +713,57 @@ mod tests {
        Ok(())
    }

+    #[test]
+    fn nested_sequence() -> IonResult<()> {
+        let ion_data: &[u8] = &[0xF1, 0x61, 0x01, 0xF1, 0x61, 0x02, 0xF0, 0x61, 0x03, 0xF0];


Could you add a comment with the text equivalent?

zslayton · 2024-07-12T16:32:09Z

src/lazy/binary/raw/v1_1/struct.rs

-            struct_type: match opcode_type {
-                // TODO: Delimited struct handling
-                OpcodeType::Struct => StructType::SymbolAddress,
+            // bytes_to_skip: 0,


zslayton · 2024-07-12T16:32:54Z

src/lazy/binary/raw/v1_1/type_descriptor.rs

@@ -125,6 +129,14 @@ impl Opcode {
        self.low_nibble
    }

+    pub fn is_delimited(&self) -> bool {


Maybe is_delimited_start for symmetry with is_delimited_end?

zslayton · 2024-07-12T16:38:06Z

src/lazy/binary/raw/v1_1/struct.rs

 pub struct RawBinaryStructIterator_1_1<'top> {
    source: ImmutableBuffer<'top>,
-    bytes_to_skip: usize,
-    struct_type: StructType,
+    // bytes_to_skip: usize,


…ader_delimited_containers

zslayton · 2024-08-10T16:03:15Z

Can we merge this or is there something blocking it?

nirosys · 2024-08-12T17:40:44Z

Can we merge this or is there something blocking it?

Just had to push up the merge from main. CI failed, looking into that now.

…ader_delimited_containers

… ion

nirosys · 2024-08-12T18:22:35Z

CI fixed, and feedback addressed. Merging.

nirosys force-pushed the rgiliam/1_1_binary_reader_delimited_containers branch from 0387b31 to fd917bd Compare June 18, 2024 23:39

nirosys marked this pull request as ready for review June 18, 2024 23:55

nirosys requested a review from zslayton June 18, 2024 23:55

zslayton reviewed Jun 21, 2024

View reviewed changes

nirosys added 5 commits July 9, 2024 14:18

Add support for delimited containers to the 1.1 binary reader

0f9abb6

Update length-prefixed structs to merge flexsym, and symbol address t…

c76d03e

…ypes; Fix delimited struct delimiters

Address clippy checks

6831f95

Address PR feedback; Stores LazyRawFieldExpr instead of offsets, simp…

21b6803

…lify some expressions

Refactor encoded value creation to reduce redundant code

127bf4c

nirosys force-pushed the rgiliam/1_1_binary_reader_delimited_containers branch from 05d0296 to 127bf4c Compare July 9, 2024 21:19

clippy: remove left-over mut

4acb8bc

nirosys requested a review from zslayton July 9, 2024 21:47

zslayton approved these changes Jul 12, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into rgiliam/1_1_binary_re…

aa02a3f

…ader_delimited_containers

Address PR feedback and remove unused imports

ce5da7a

nirosys added 4 commits August 12, 2024 10:43

Merge remote-tracking branch 'origin/main' into rgiliam/1_1_binary_re…

b1e82a5

…ader_delimited_containers

Add delimited contents to LazyRawBinaryValue; broke after merge

a724aaf

PR feedback; missed a left-over comment and request to comment binary…

776edc4

… ion

Remove left-over comment

c016bb3

nirosys merged commit bc2e976 into amazon-ion:main Aug 12, 2024
22 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for delimited containers to the 1.1 binary reader #787

Add support for delimited containers to the 1.1 binary reader #787

nirosys commented Jun 7, 2024 •

edited

Loading

nirosys commented Jun 17, 2024

codecov bot commented Jun 18, 2024 •

edited

Loading

zslayton Jun 21, 2024

nirosys Jul 9, 2024

zslayton Jun 21, 2024

nirosys Jun 25, 2024

zslayton Jun 21, 2024

zslayton Jun 21, 2024 •

edited

Loading

zslayton Jun 21, 2024

zslayton Jun 21, 2024

zslayton Jun 21, 2024

zslayton Jun 21, 2024

zslayton Jul 12, 2024

zslayton Jul 12, 2024

zslayton Jul 12, 2024

zslayton Jul 12, 2024

zslayton commented Aug 10, 2024

nirosys commented Aug 12, 2024

nirosys commented Aug 12, 2024

		) -> IonResult<Option<(LazyRawFieldExpr<'top, BinaryEncoding_1_1>, usize)>> {
		let mut buffer = input;

Add support for delimited containers to the 1.1 binary reader #787

Add support for delimited containers to the 1.1 binary reader #787

Conversation

nirosys commented Jun 7, 2024 • edited Loading

nirosys commented Jun 17, 2024

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

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 Jun 21, 2024 • edited Loading

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 Aug 10, 2024

nirosys commented Aug 12, 2024

nirosys commented Aug 12, 2024

nirosys commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

zslayton Jun 21, 2024 •

edited

Loading