Removes IonElement trait, ElementRef impl #475

zslayton · 2023-03-07T15:10:02Z

IonElement provides a trait abstraction over two flavors of Element API:

The 'borrowed' API, where each ElementRef has a lifetime tied to the lifetime of the source data.
The 'owned' API, where each Element is self-contained, with no connection to the source data.

While the borrowed API was intended to be lighter weight, the current implementation materializes nearly the same amount of data as the owned API. Collections (list, sexp, struct) are heap-allocated. Of the scalar types, only strings and lobs avoid any allocation in the borrowed API.

The IonElement trait was designed before GATs were stabilized and had some rough edges caused by the inherent difficulty in abstracting over lifetime constraints in traits in the absence of GATs.

In practice, users that need an Element API are exclusively using the owned variant.

In light of these observations and in anticipation of the upcoming v1.0 release, this PR removes both the IonElement trait and the borrowed implementation, leaving only the owned Element API. These constructs can be reintroduced in the future if and when they are needed.

Follow-on pull requests will:

Offer builder APIs to construct Struct, List, and SExpression more ergonomically.
Merge ElementReader's functionality into IonReader.
Merge ElementWriter's functionality into IonWriter.
Restore the documentation and examples to reflect the new APIs.

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

`IonElement` provides a trait abstraction over two flavors of `Element` API: 1. The 'borrowed' API, where each `ElementRef` has a lifetime tied to the lifetime of the source data. 2. The 'owned' API, where each `Element` is self-contained, with no connection to the source data. While the borrowed API should be lighter weight, the current implementation performs materializes nearly the same amount of data as the owned API. Collections (`list`, `sexp`, `struct`) are heap-allocated; only `string`s and `lob`s avoid allocation. The `IonElement` trait was designed before GATs were stabilized and had some rough edges caused by the inherent difficulty in abstracting over lifetime constraints in traits in the absence of GATs. In practice, users that need an `Element` API are exclusively using the `owned` variant. In light of these observations and in anticipation of the upcoming v1.0 release, this PR removes both the `IonElement` trait and the `borrowed` implementation, leaving only the owned `Element` API. These constructs can be reintroduced in the future if and when they are needed.

codecov · 2023-03-07T15:38:11Z

Codecov Report

Patch coverage: 89.15% and project coverage change: -0.26 ⚠️

Comparison is base (2a8e839) 90.40% compared to head (3ef873c) 90.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #475      +/-   ##
==========================================
- Coverage   90.40%   90.15%   -0.26%     
==========================================
  Files          77       76       -1     
  Lines       14179    13548     -631     
==========================================
- Hits        12819    12214     -605     
+ Misses       1360     1334      -26

Impacted Files	Coverage Δ
src/types/integer.rs	`89.18% <ø> (+0.33%)`	⬆️
src/value/iterators.rs	`80.00% <ø> (ø)`
src/value/owned.rs	`83.02% <60.29%> (-11.83%)`	⬇️
src/value/element_stream_reader.rs	`86.58% <85.71%> (-0.05%)`	⬇️
src/value/native_writer.rs	`91.22% <88.88%> (ø)`
src/value/mod.rs	`92.26% <98.78%> (+3.55%)`	⬆️
ion-hash/src/element_hasher.rs	`98.00% <100.00%> (ø)`
ion-hash/src/lib.rs	`100.00% <100.00%> (ø)`
ion-hash/src/representation.rs	`99.00% <100.00%> (ø)`
ion-hash/src/type_qualifier.rs	`100.00% <100.00%> (ø)`
... and 15 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

zslayton

🗺️ PR tour

While most of the changes in the diff are pretty mechanical (removing generics in favor of Element), there are a few other changes that I'll point out below.

zslayton · 2023-03-07T15:41:48Z

src/value/owned.rs

+        match integer_val {
+            Integer::I64(i64_int) => i64_int.into(),
+            Integer::BigInt(big_int) => big_int.into(),
+        }


🗺️ We already had From<i64> (e.g. 16.into()) and From<BigInt>, adding From<Integer> allows simple conversions in places where we've already constructed an Integer enum.

zslayton · 2023-03-07T15:42:25Z

src/value/owned.rs

+        Value::String(string_val.to_owned()).into()
+    }
+}
+


🗺️ Allows let s: Element = "hello".into();

zslayton · 2023-03-07T15:43:29Z

src/value/owned.rs

-    type AnnotationsIterator<'a> = SymbolsIterator<'a>;
-
-    fn ion_type(&self) -> IonType {
+impl Element {


🗺️ For now I've converted the functionality that was associated with the IonElement trait into concrete methods on the Element type. I expect to refactor these in a follow-on PR.

zslayton · 2023-03-07T15:44:28Z

src/value/owned.rs

        match &self.value {
            Value::Integer(i) => Some(i),
            _ => None,
        }
    }

-    fn as_f64(&self) -> Option<f64> {
+    pub fn as_float(&self) -> Option<f64> {


🗺️ I renamed this to reflect the Ion type (float) rather than the Rust type (f64) as we do in the other methods.

zslayton · 2023-03-07T15:45:32Z

src/value/owned.rs

        match &self.value {
            Value::Timestamp(t) => Some(t),
            _ => None,
        }
    }

-    fn as_str(&self) -> Option<&str> {
+    pub fn as_text(&self) -> Option<&str> {
        match &self.value {
            Value::String(text) => Some(text),
            Value::Symbol(sym) => sym.text(),
            _ => None,
        }
    }


🗺️ Rather than have a single as_string() method that returns text for either String or Symbol elements, we now have as_string() for String elements, as_symbol() for Symbol elements, and as_text() for the old behavior that doesn't care which text type the Element is.

zslayton · 2023-03-07T15:52:34Z

ion-hash/src/representation.rs

@@ -110,7 +95,7 @@ where
                    return Ok(());
                }

-                self.update_escaping(&v.to_be_bytes());
+                self.update_escaping(v.to_be_bytes());


🗺️ Incorporating a clippy suggestion.

zslayton · 2023-03-07T15:52:41Z

ion-hash/src/representation.rs

@@ -154,7 +136,7 @@ where

    fn write_repr_string(&mut self, value: Option<&str>) -> IonResult<()> {
        match value {
-            Some(s) if s.len() > 0 => self.update_escaping(s.as_bytes()),
+            Some(s) if !s.is_empty() => self.update_escaping(s.as_bytes()),


🗺️ Incorporating a clippy suggestion.

zslayton · 2023-03-07T15:52:45Z

ion-hash/src/representation.rs

@@ -163,17 +145,14 @@ where

    fn write_repr_blob(&mut self, value: Option<&[u8]>) -> IonResult<()> {
        match value {
-            Some(bytes) if bytes.len() > 0 => self.update_escaping(bytes),
+            Some(bytes) if !bytes.is_empty() => self.update_escaping(bytes),


🗺️ Incorporating a clippy suggestion.

zslayton · 2023-03-07T15:53:32Z

src/value/owned.rs

        match &self.value {
            Value::Symbol(sym) => Some(sym),
            _ => None,
        }
    }

-    fn as_bool(&self) -> Option<bool> {
+    pub fn as_boolean(&self) -> Option<bool> {


🗺️ Renaming to reflect the Ion type (boolean) rather than the Rust type (bool).

zslayton · 2023-03-07T15:54:42Z

src/value/owned.rs

        match &self.value {
            Value::Boolean(b) => Some(*b),
            _ => None,
        }
    }

-    fn as_bytes(&self) -> Option<&[u8]> {
+    pub fn as_lob(&self) -> Option<&[u8]> {


🗺️ As with the string/symbol/text, I've broken this into clob/blob/lob.

desaikd

Looks good 🚀
There are still some clippy warnings for element_stream_reader though.

zslayton · 2023-03-07T17:03:18Z

There are still some clippy warnings for element_stream_reader though.

Those are related to #472.

zslayton requested review from nirosys and desaikd March 7, 2023 15:10

Updates ion-hash to use owned::Element API

bbd73bd

zslayton added 2 commits March 7, 2023 10:48

Breaks as_bytes() into as_lob(), as_blob(), and as_clob()

325333e

Clippy suggestions for test suite

61fd443

zslayton commented Mar 7, 2023

View reviewed changes

cargo fmt

3ef873c

desaikd approved these changes Mar 7, 2023

View reviewed changes

zslayton merged commit e3f7bd1 into main Mar 7, 2023

zslayton deleted the element branch March 7, 2023 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Removes IonElement trait, ElementRef impl #475

Removes IonElement trait, ElementRef impl #475

Uh oh!

zslayton commented Mar 7, 2023

Uh oh!

codecov bot commented Mar 7, 2023 •

edited

Loading

Uh oh!

zslayton left a comment •

edited

Loading

Uh oh!

zslayton Mar 7, 2023

Uh oh!

zslayton Mar 7, 2023

Uh oh!

zslayton Mar 7, 2023

Uh oh!

zslayton Mar 7, 2023

Uh oh!

zslayton Mar 7, 2023

Uh oh!

zslayton Mar 7, 2023

Uh oh!

zslayton Mar 7, 2023

Uh oh!

zslayton Mar 7, 2023

Uh oh!

zslayton Mar 7, 2023

Uh oh!

zslayton Mar 7, 2023

Uh oh!

desaikd left a comment

Uh oh!

zslayton commented Mar 7, 2023

Uh oh!

Uh oh!

Removes IonElement trait, ElementRef impl #475

Removes IonElement trait, ElementRef impl #475

Uh oh!

Conversation

zslayton commented Mar 7, 2023

Uh oh!

codecov bot commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zslayton left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

desaikd left a comment

Choose a reason for hiding this comment

Uh oh!

zslayton commented Mar 7, 2023

Uh oh!

Uh oh!

codecov bot commented Mar 7, 2023 •

edited

Loading

zslayton left a comment •

edited

Loading