Additional tooling access methods, trait impls, and a bug fix #780

zslayton · 2024-05-31T16:50:57Z

This PR includes a variety of small fixes and improvements that I found myself needing as I went to upgrade the ion-cli to HEAD. I'd like to include these in the v1.0-rc4 release. I have completed the ion-cli upgrade in my local workspace using this branch, so I'm reasonably confident that these are the last items I need to get in.

Renames the RawReaderType enum to IonEncoding because it is generally useful in other contexts too.
Adds the ability for system and raw readers using AnyEncoding to report the encoding they're currently using.
Adds value_span and annotations_span methods to LazyValue. It already had a span method whose bytes included both the annotations and the value.
The binary 1.0 and 1.1 writers will now write f64s as f32s to save space when it can be done losslessly.
Adds WriteAsIon implementations for LazyList, LazySExp, and LazyStruct.
Adds IntoIterator impls for LazyList, LazySExp and LazyStruct. Previously, they only existed for borrowed (&) references to those types.
Fixes a bug in the StreamingRawReader that could cause a value's annotations span to be overwritten if reading that value consumed all of the data remaining in the buffer.
Renames the feature-gated LazyValue::lower method (which returns a LazyExpandedValue) to LazyValue::expanded() so I could add a raw() method alongside it that returns the underlying LazyRawValue when applicable.

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

* Renames the `RawReaderType` enum to `IonEncoding` because it is generally useful in other contexts too. * Adds the ability for system and raw readers using `AnyEncoding` to report the encoding they're currently using. * Adds `value_span` and `annotations_span` methods to `LazyValue` in addition to `span`, which includes both the annotations and the value. * The binary 1.0 and 1.1 writers will now write `f64`s as `f32`s to save space when it can be done losslessly. * Adds `WriteAsIon` implementations for `LazyList`, `LazySExp`, and `LazyStruct`. * Adds `IntoIterator` impls for `LazyList`, `LazySExp` and `LazyStruct`. Previously, they only existed for borrowed (`&`) references to those types. * Fixes a bug in the `StreamingRawReader` that could cause a value's annotations span to be overwritten if reading that value consumed all of the data remaining in the buffer. * Renames the feature-gated `LazyValue::lower` method (which returns a `LazyExpandedValue`) to `LazyValue::expanded()` so I could add a `raw()` method alongside it that returns the underlying `LazyRawValue` when applicable.

zslayton

🗺️ PR tour

zslayton · 2024-05-31T17:03:38Z

src/lazy/any_encoding.rs

-    type ReaderSavedState = RawReaderType;
+    type ReaderSavedState = IonEncoding;


🗺️ This type (an enum of the four Ion encodings) is now returned by the new (feature gated) SystemReader<AnyEncoding, _>::detected_encoding() method, allowing users to determine what encoding is currently being read.

zslayton · 2024-05-31T17:06:15Z

src/lazy/any_encoding.rs

+    fn annotations_span(&self) -> Span<'top> {
+        match &self.encoding {
+            LazyRawValueKind::Text_1_0(v) => v.annotations_span(),
+            LazyRawValueKind::Binary_1_0(v) => v.annotations_span(),
+            LazyRawValueKind::Text_1_1(v) => v.annotations_span(),
+            LazyRawValueKind::Binary_1_1(v) => v.annotations_span(),
+        }
+    }
+
+    fn value_span(&self) -> Span<'top> {
+        match &self.encoding {
+            LazyRawValueKind::Text_1_0(v) => v.value_span(),
+            LazyRawValueKind::Binary_1_0(v) => v.value_span(),
+            LazyRawValueKind::Text_1_1(v) => v.value_span(),
+            LazyRawValueKind::Binary_1_1(v) => v.value_span(),
+        }
+    }


🗺️ The span() method returns a byte slice containing the complete value--both annotations and data.

These methods allow you to access the annotations and data separately.

zslayton · 2024-05-31T17:08:23Z

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

+            let mut expected_encoding = vec![];
+            let float32 = *value as f32;
+            if float32 as f64 == *value {
+                expected_encoding.push(0x5C);
+                expected_encoding.extend_from_slice(&float32.to_le_bytes()[..]);
+            } else {
+                expected_encoding.push(0x5D);
+                expected_encoding.extend_from_slice(&value.to_le_bytes()[..]);
+            }


🗺️ We have a lot of unit tests where we re-invent the wheel for the sake of bootstrapping the encoder/decoder. We should look at migrating some of the more painful ones (like this one) to a roundtrip test using our recently established readers and writers.

zslayton · 2024-05-31T17:09:30Z

src/lazy/encoder/write_as_ion.rs

+            List(l) => value_writer.write(l),
+            SExp(s) => value_writer.write(s),
+            Struct(s) => value_writer.write(s),


🗺️ Giving these types their own WriteAsIon implementations streamlined this code path nicely, but my real aim was to make passing them to a writer in other contexts easier.

zslayton · 2024-05-31T17:10:21Z

src/lazy/encoding.rs

@@ -60,6 +60,8 @@ pub trait Encoding: Encoder + Decoder {
    ) -> IonResult<W> {
        Self::default_write_config().encode_all_to(output, values)
    }
+
+    fn encoding(&self) -> IonEncoding;


🗺️ Types implementing the Encoding trait now must report which variant of the IonEncoding enum they correspond to.

zslayton · 2024-05-31T17:11:03Z

src/lazy/sequence.rs

@@ -282,6 +297,15 @@ impl<'a, 'top, 'data: 'top, D: Decoder> IntoIterator for &'a LazySExp<'top, D> {
    }
 }

+impl<'top, 'data: 'top, D: Decoder> IntoIterator for LazySExp<'top, D> {


🗺️ Now that LazyList, LazySExp, and LazyStruct are Copy, it makes sense to allow them to be iterated over without having to borrow them first.

zslayton · 2024-05-31T17:12:57Z

src/lazy/streaming_raw_reader.rs

-            // If we've exhausted the buffer...
-            if bytes_read >= available_bytes.len() || matches!(result, Err(IonError::Incomplete(_)))
-            {
-                // ...try to pull more data from the data source. If we get more data, try again.


🗺️ This was a subtle bug enabled by my use of unsafe. If result was Ok(_) but also consumed all of the data in available_bytes, then this code path would have input shift its bytes to the beginning and pull in more data. Because result contained an Ok(value) referring to a slice of that buffer, its data would get clobbered.

zslayton · 2024-05-31T17:13:53Z

src/lazy/streaming_raw_reader.rs

@@ -319,6 +352,14 @@ impl<'a> IonInput for StdinLock<'a> {
    }
 }

+impl<T: Read, U: Read> IonInput for io::Chain<T, U> {


🗺️ The ion-cli uses the io::Chain type to link together the first few bytes of an input stream (used to detect gzip, zstd, etc) with the rest of the stream.

popematt

I noticed there were some functions where you removed the feature gate, and others where you kept the feature gate. How did you decide for which functions to remove vs retain the feature gate?

src/lazy/encoder/binary/v1_0/value_writer.rs

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

src/lazy/streaming_raw_reader.rs

zslayton · 2024-05-31T17:47:53Z

I noticed there were some functions where you removed the feature gate, and others where you kept the feature gate. How did you decide for which functions to remove vs retain the feature gate?

Some methods are on a fuzzy line between being reader-writer and tooling-apis. For example, LazyExpandedValue::context returns the EncodingContextRef used to decode that value. It's definitely part of the reader-writer API, but whether it's specifically tooling-access is debatable.

I decided I was ok with removing the feature gate from methods like that because they're still behind some feature gate, so either way there's no guarantee of stability being made.

popematt · 2024-05-31T19:30:10Z

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

-        if value == 0f32 && !value.is_sign_negative() {
-            self.push_byte(0x5A);
-            return Ok(());
+        match value.smallest_repr() {


When we add support for f16, we could also DRY it up like this:

fn write_float_repr(mut self, value: FloatRepr) -> IonResult<()> { match value { // ... } Ok(()) } pub fn write_f16(mut self, value: f16) -> IonResult<()> { self.write_float_repr(value.smallest_repr()) } pub fn write_f32(mut self, value: f32) -> IonResult<()> { self.write_float_repr(value.smallest_repr()) } pub fn write_f64(mut self, value: f64) -> IonResult<()> { self.write_float_repr(value.smallest_repr()) }

I was thinking the same thing 👍

zslayton requested review from popematt and nirosys May 31, 2024 16:51

zslayton commented May 31, 2024

View reviewed changes

popematt reviewed May 31, 2024

View reviewed changes

src/lazy/encoder/binary/v1_0/value_writer.rs Show resolved Hide resolved

src/lazy/encoder/binary/v1_1/value_writer.rs Outdated Show resolved Hide resolved

src/lazy/streaming_raw_reader.rs Show resolved Hide resolved

src/lazy/streaming_raw_reader.rs Show resolved Hide resolved

zslayton added 2 commits May 31, 2024 14:45

Adds SmallestFloatRepr trait for f64, f32

7d0128d

unsafe explanations

a63a5d0

zslayton requested a review from popematt May 31, 2024 19:13

popematt approved these changes May 31, 2024

View reviewed changes

zslayton merged commit 80088b4 into main May 31, 2024
32 checks passed

zslayton deleted the rc4-punch-list branch May 31, 2024 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional tooling access methods, trait impls, and a bug fix #780

Additional tooling access methods, trait impls, and a bug fix #780

zslayton commented May 31, 2024

zslayton left a comment

zslayton May 31, 2024

zslayton May 31, 2024

zslayton May 31, 2024

zslayton May 31, 2024

zslayton May 31, 2024

zslayton May 31, 2024

zslayton May 31, 2024

zslayton May 31, 2024

popematt left a comment

zslayton commented May 31, 2024

popematt May 31, 2024

zslayton May 31, 2024

		type ReaderSavedState = RawReaderType;
		type ReaderSavedState = IonEncoding;

Additional tooling access methods, trait impls, and a bug fix #780

Additional tooling access methods, trait impls, and a bug fix #780

Conversation

zslayton commented May 31, 2024

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

popematt left a comment

Choose a reason for hiding this comment

zslayton commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment