Add 1.1 binary reader support for length-prefixed structs #768

nirosys · 2024-05-15T08:16:29Z

Issue #, if available: #664

Description of changes:
This PR adds support to the 1.1 raw binary reader for length-prefixed structs.

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

zslayton · 2024-05-15T21:00:20Z

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

+enum StructType {
+    Invalid,
+    FlexSym,
+    SymbolAddress,
+}


Why not limit it to FlexSym/SymbolAddress and communicate Invalid via a Result<StructType>?

I didn't use a result, because at the time I was thinking I'd have to somehow work a Result into the different iter functions, which felt like a much bigger ripple than I wanted to happen. I've reworked that now, so that it only holds FlexSym, and SymbolAddress. If new has an unknown opcode it will panic, since the struct iterator shouldn't be created without a valid struct.

zslayton · 2024-05-15T21:03:27Z

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

+        let (flex_sym, after) = buffer.read_flex_int()?;
+        let sym_value = flex_sym.value();
+        let (sym, after) = match sym_value.cmp(&0) {


Could you move this bit of logic to the FlexSym type?

zslayton · 2024-05-15T21:05:58Z

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

+    }
+
+    /// Helper function called from [`Self::next`] to parse the current field and value from the
+    /// struct. On success, returns an both the field pair via [`LazyRawFieldExpr`] as well as the


Suggested change

/// struct. On success, returns an both the field pair via [`LazyRawFieldExpr`] as well as the

/// struct. On success, returns both the field pair via [`LazyRawFieldExpr`] as well as the

zslayton · 2024-05-15T21:08:05Z

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

+            (
+                // { $10: <NOP>, $11: 0e0 } - with nops, skip the NOP'd fields.
+                &[ 0xC4, 0x15, 0xEC, 0x17, 0x5A ],
+                &[
+                    (11usize.into(), IonType::Float),
+                ],
+            ),


Could you add a test case where the NOP(s) are at the end of the container?

zslayton · 2024-05-15T21:10:38Z

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

+                // { "foo": 1, $11: 2 }
+                &[ 0xD9, 0xFB, 0x66, 0x6F, 0x6F, 0x51, 0x01, 0x17, 0x91, 0x02],
+                &[ ("foo".into(), IonType::Int), (11usize.into(), IonType::Symbol)],


I realize you're implementing an older copy of the spec, but the most recent version merged the distinct struct "kinds" into a single kind that can switch from symbol addresses to flexsym partway through a struct by emitting a FlexUInt 0 in field name position. This can wait for a follow-on PR since we have lots of updates to make around opcodes.

zslayton · 2024-05-15T21:11:32Z

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

+            // Symbol Address
+            (
+                // { $10: 1, $11: 2 }
+                &[0xC6, 0x15, 0x51, 0x01, 0x17, 0x51, 0x02],
+                &[
+                    (10usize.into(), IonType::Int),
+                    (11usize.into(), IonType::Int),
+                ]
+            ),


Could you add test cases for the empty struct case and a nested struct?

zslayton · 2024-05-29T14:09:59Z

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

+        let (flex_sym, after) = buffer.read_flex_sym()?;
+        let (sym, after) = match flex_sym.value() {
+            FlexSymValue::SymbolRef(sym_ref) => (sym_ref, after),
+            FlexSymValue::Opcode(_opcode) => todo!(),


zslayton · 2024-05-29T14:12:04Z

src/lazy/encoder/binary/v1_1/flex_sym.rs

+    /// `offset` is the position of the slice in some larger input stream. It is only used to
+    ///          populate an appropriate error message if reading fails.
+    pub fn read(input: &'top [u8], offset: usize) -> IonResult<FlexSym<'top>> {
+        use crate::{result::IonFailure, IonError};


Out of interest, does your editor pull in traits/imports to the nearest scope itself or are you doing that manually?

I do it manually. I try to keep it local to where it's used if it's not something that's going to be used elsewhere.

I would love it if my editor could promote the imports to higher scope if there was an existing local import though, or even if clippy could just point it out.

codecov · 2024-05-29T22:03:54Z

Codecov Report

Attention: Patch coverage is 2.64151% with 258 lines in your changes are missing coverage. Please review.

Project coverage is 75.39%. Comparing base (fa3167d) to head (40e8b15).
Report is 65 commits behind head on main.

Files	Patch %	Lines
src/lazy/binary/raw/v1_1/reader.rs	0.00%	114 Missing ⚠️
src/lazy/binary/raw/v1_1/struct.rs	0.00%	97 Missing ⚠️
src/lazy/encoder/binary/v1_1/flex_sym.rs	0.00%	30 Missing ⚠️
src/lazy/binary/raw/v1_1/immutable_buffer.rs	33.33%	6 Missing ⚠️
src/lazy/binary/raw/v1_1/type_descriptor.rs	0.00%	6 Missing ⚠️
src/lazy/binary/raw/v1_1/value.rs	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #768      +/-   ##
==========================================
- Coverage   80.60%   75.39%   -5.21%     
==========================================
  Files         140      129      -11     
  Lines       28209    27098    -1111     
  Branches    28209    27098    -1111     
==========================================
- Hits        22737    20430    -2307     
- Misses       3641     5098    +1457     
+ Partials     1831     1570     -261

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

nirosys mentioned this pull request May 15, 2024

Binary read support for length-prefixed containers #664

Open

6 tasks

nirosys marked this pull request as ready for review May 15, 2024 10:52

zslayton reviewed May 15, 2024

View reviewed changes

nirosys force-pushed the rgiliam/1_1_binary_reader_structs branch from 4f173e6 to 7980ee2 Compare May 21, 2024 07:17

nirosys requested a review from zslayton May 28, 2024 20:41

zslayton approved these changes May 29, 2024

View reviewed changes

nirosys force-pushed the rgiliam/1_1_binary_reader_structs branch from 7980ee2 to 030623a Compare May 29, 2024 21:56

nirosys added 3 commits May 29, 2024 14:58

Add 1.1 binary reader support for length-prefixed structs

ba56598

Implement HasSpan and HasRange for LazyRawBinaryFieldName_1_1

54301b3

Address PR feedback

40e8b15

nirosys force-pushed the rgiliam/1_1_binary_reader_structs branch from 030623a to 40e8b15 Compare May 29, 2024 22:01

nirosys merged commit eab334e into amazon-ion:main May 29, 2024
27 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 1.1 binary reader support for length-prefixed structs #768

Add 1.1 binary reader support for length-prefixed structs #768

nirosys commented May 15, 2024

zslayton May 15, 2024

nirosys May 21, 2024 •

edited

zslayton May 15, 2024

zslayton May 15, 2024

zslayton May 15, 2024

zslayton May 15, 2024

zslayton May 15, 2024

zslayton May 29, 2024

zslayton May 29, 2024

nirosys May 29, 2024

codecov bot commented May 29, 2024

	/// struct. On success, returns an both the field pair via [`LazyRawFieldExpr`] as well as the
	/// struct. On success, returns both the field pair via [`LazyRawFieldExpr`] as well as the

Add 1.1 binary reader support for length-prefixed structs #768

Add 1.1 binary reader support for length-prefixed structs #768

Conversation

nirosys commented May 15, 2024

Choose a reason for hiding this comment

nirosys May 21, 2024 • edited

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 May 29, 2024

Codecov Report

nirosys May 21, 2024 •

edited