Element reader #485

zslayton · 2023-03-15T21:12:21Z

This PR introduces an updated ElementReader trait that provides methods for materializing values from the reader into Elements, and is available for all user-level reader implementations regardless of format or blocking strategy.

Other changes:

Adds static convenience methods to the Element type that mirror the method
names in the ElementReader trait, simplifying one-off parsing.
- Element::read_one(), Element::read_next(), Element::read_all()
Removes NativeElementReader, which used to be part of the native/ion-c dichotomy.
Streamlines some of the integration test logic in element-test-vectors.

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

codecov · 2023-03-15T21:25:41Z

Codecov Report

Patch coverage: 92.36% and project coverage change: -0.92 ⚠️

Comparison is base (747ee44) 90.37% compared to head (f19d455) 89.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #485      +/-   ##
==========================================
- Coverage   90.37%   89.45%   -0.92%     
==========================================
  Files          78       77       -1     
  Lines       13749    13683      -66     
==========================================
- Hits        12425    12240     -185     
- Misses       1324     1443     +119

Impacted Files	Coverage Δ
src/ion_hash/mod.rs	`100.00% <ø> (ø)`
src/lib.rs	`88.84% <ø> (ø)`
src/reader.rs	`80.18% <ø> (-2.39%)`	⬇️
src/value/mod.rs	`90.76% <ø> (ø)`
src/value/reader.rs	`92.94% <87.34%> (-7.06%)`	⬇️
src/value/builders.rs	`93.70% <100.00%> (ø)`
src/value/element_stream_reader.rs	`86.54% <100.00%> (-0.05%)`	⬇️
src/value/native_writer.rs	`94.54% <100.00%> (ø)`
src/value/owned.rs	`89.81% <100.00%> (+0.35%)`	⬆️
tests/ion_hash_tests.rs	`80.98% <100.00%> (ø)`

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ 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-03-15T21:31:51Z

src/binary/constants.rs

@@ -1,10 +1,13 @@
+// This module and its contents are visible as a workaround for:
+// https://github.com/amazon-ion/ion-rust/issues/484


🗺️ Clickable link: #484.

zslayton · 2023-03-15T21:35:00Z

src/value/builders.rs

@@ -11,7 +11,7 @@ use crate::Symbol;
 ///     .push("foo")
 ///     .build()
 ///     .into();
-/// let expected = Element::parse(r#"[1, true, "foo"]"#).unwrap();
+/// let expected = Element::read_one(r#"[1, true, "foo"]"#).unwrap();


🗺️ In the previous PR, we introduced the method Element::parse(ion_data) -> IonResult<Element>.

The new ElementReader trait has three read_ methods:

read_next_element()

read_one_element()

read_all_elements()

In this PR, I added analogous convenience methods to Element:

Element::read_first() -- (It's stateless, so you can't read more than the first one.)

Element::read_one() -- This was previously called parse. It enforces that the input has exactly one value.

Element::read_all()

A lot of the changes that follow are renaming Element::parse to Element::read_one.

zslayton · 2023-03-15T21:41:40Z

src/value/native_reader.rs

🗺️ The API for the NativeElementReader was constrained by needing to be compatible with the IonCElementReader which has since been removed. I copied over some helper method logic, but started with a fresh version of the ElementReader trait.

zslayton · 2023-03-15T21:44:20Z

src/value/owned.rs

+    )]
+    #[case::struct_(
+        ion_struct!{"greetings": "hello"},
+        Element::read_one(r#"{greetings: "hello"}"#).unwrap()


🗺️ These changes were merge conflicts with #481. There's no logic change.

zslayton · 2023-03-15T21:48:46Z

tests/element_test_vectors.rs

-        };
-
-        reader.read_all(buffer.as_slice())
-    }


🗺️ I was able to remove the RoundTrip trait and its implementations. The one-size-fits-all serialize method below performs much of the same logic without needing the trait.

zslayton · 2023-03-15T21:50:40Z

tests/element_test_vectors.rs

 }

 trait ElementApi {
-    type ReaderApi: ElementReader;


🗺️ I was also able to remove this associated type. Now different readers can be configured and returned as the same (dynamically dispatched) Reader type.

zslayton · 2023-03-15T21:51:27Z

tests/element_test_vectors.rs

@@ -249,13 +230,15 @@ trait ElementApi {
        Ok(())
    }

-    fn read_file(reader: &Self::ReaderApi, file_name: &str) -> IonResult<Vec<Element>> {
-        // TODO have a better API that doesn't require buffering into memory everything...


🗺️ Removing this old comment; he native reader implementations don't have to load the entire input into memory. 🎉

zslayton · 2023-03-15T21:51:57Z

tests/element_test_vectors.rs

-        fn make_reader() -> Self::ReaderApi {
-            NativeElementReader
+        fn make_reader(data: &[u8]) -> IonResult<Reader<'_>> {
+            ReaderBuilder::default().build(data)


🗺️ Constructs a blocking, format-agnostic reader.

zslayton · 2023-03-15T21:52:44Z

tests/element_test_vectors.rs

@@ -645,8 +622,20 @@ mod non_blocking_native_element_tests {
            &[]
        }

-        fn make_reader() -> Self::ReaderApi {
-            NonBlockingNativeElementReader
+        fn make_reader(data: &[u8]) -> IonResult<Reader<'_>> {


🗺️ This method needs to construct a format-agnostic non-blocking reader, which isn't currently possible with the existing ReaderBuilder. Fixing that is being tracked in #484.

Is that worth leaving a comment here so that #484 cleanup can easily find all the relevant pieces of code?

jobarr-amzn · 2023-03-16T17:53:59Z

src/value/native_reader.rs

src/value/native_writer.rs

jobarr-amzn · 2023-03-16T18:27:05Z

tests/element_test_vectors.rs

 }

 trait ElementApi {
-    type ReaderApi: ElementReader;


jobarr-amzn · 2023-03-16T18:27:23Z

tests/element_test_vectors.rs

-        };
-
-        reader.read_all(buffer.as_slice())
-    }


tests/element_test_vectors.rs

jobarr-amzn · 2023-03-16T18:32:17Z

tests/element_test_vectors.rs

@@ -249,13 +230,15 @@ trait ElementApi {
        Ok(())
    }

-    fn read_file(reader: &Self::ReaderApi, file_name: &str) -> IonResult<Vec<Element>> {
-        // TODO have a better API that doesn't require buffering into memory everything...


jobarr-amzn · 2023-03-16T18:33:44Z

tests/element_test_vectors.rs

@@ -645,8 +622,20 @@ mod non_blocking_native_element_tests {
            &[]
        }

-        fn make_reader() -> Self::ReaderApi {
-            NonBlockingNativeElementReader
+        fn make_reader(data: &[u8]) -> IonResult<Reader<'_>> {


Is that worth leaving a comment here so that #484 cleanup can easily find all the relevant pieces of code?

Co-authored-by: jobarr-amzn <[email protected]>

zslayton added 5 commits March 15, 2023 16:44

Adds ElementReader trait

ab997dc

Manual fixes for missed renames

6f58178

Merge remote-tracking branch 'origin/main' into element-reader

164af4f

Removed old comment

6ea5b68

links to GitHub issue 484

4760f6f

zslayton added 2 commits March 15, 2023 17:27

one more link to GitHub issue 484

2fbde84

formatting

dbec271

zslayton commented Mar 15, 2023

View reviewed changes

undoing IDE renames

8f5eefe

zslayton marked this pull request as ready for review March 15, 2023 21:54

zslayton requested review from popematt and desaikd March 15, 2023 21:54

zslayton mentioned this pull request Mar 16, 2023

Mass renaming, container Display impls #486

Merged

jobarr-amzn approved these changes Mar 16, 2023

View reviewed changes

zslayton and others added 3 commits March 16, 2023 14:54

Remove unnecessary type ascription

d124638

Co-authored-by: jobarr-amzn <[email protected]>

Remove unnecessary type ascription 2

ceaa38b

Co-authored-by: jobarr-amzn <[email protected]>

added link to GitHub issue #484 in integration tests

f19d455

zslayton merged commit abf4634 into main Mar 16, 2023

zslayton deleted the element-reader branch March 16, 2023 19:14

tgregg mentioned this pull request Mar 20, 2023

Iteratively parse top-level values in inspect and to amazon-ion/ion-cli#47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Element reader #485

Element reader #485

zslayton commented Mar 15, 2023

codecov bot commented Mar 15, 2023 •

edited

Loading

zslayton left a comment

zslayton Mar 15, 2023

zslayton Mar 15, 2023

zslayton Mar 15, 2023

zslayton Mar 15, 2023

zslayton Mar 15, 2023

jobarr-amzn Mar 16, 2023

zslayton Mar 15, 2023

jobarr-amzn Mar 16, 2023

zslayton Mar 15, 2023

jobarr-amzn Mar 16, 2023

zslayton Mar 15, 2023

zslayton Mar 15, 2023

jobarr-amzn Mar 16, 2023

jobarr-amzn Mar 16, 2023

jobarr-amzn Mar 16, 2023

jobarr-amzn Mar 16, 2023

jobarr-amzn Mar 16, 2023

jobarr-amzn Mar 16, 2023

		@@ -1,10 +1,13 @@
		// This module and its contents are visible as a workaround for:
		// https://github.com/amazon-ion/ion-rust/issues/484

Element reader #485

Element reader #485

Conversation

zslayton commented Mar 15, 2023

codecov bot commented Mar 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 15, 2023 •

edited

Loading