Adds IonData wrapper that uses Ion equivalence for equality #517

popematt · 2023-04-21T16:59:20Z

Issue #, if available:

None

Description of changes:

Adds IonData, a struct that can wrap anything that implements IonEq.
- For IonData<Element>, I added some delegations to Element I thought would be required for it to be useful. There's the read_*() functions and annotations(), and value().
Adds IonOrd, a trait for a total ordering that is consistent with IonEq.
Adds implementations of IonOrd for Element, Value, and others.
Moves IonEq from src/ to src/ion_data/ so that all of the Ion-data-model-semantics stuff is in one place.
Small, incidental changes:
- Adds derive PartialOrd and Ord to Bytes
- Moves Element.ion_type() implementation to Value; Element.ion_type() now delegates to Value.ion_type()
- Adds IntoIterator impl for &'a Sequence

I have no illusions that this is perfect, but I want to get something started because we need this for handling things like distinct elements efficiently in ion-schema-rust.

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

codecov · 2023-04-21T17:06:58Z

Codecov Report

Patch coverage: 98.09% and project coverage change: +0.25 🎉

Comparison is base (07779eb) 89.07% compared to head (4fda30d) 89.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   89.07%   89.33%   +0.25%     
==========================================
  Files          82       84       +2     
  Lines       13027    13256     +229     
==========================================
+ Hits        11604    11842     +238     
+ Misses       1423     1414       -9

Impacted Files	Coverage Δ
src/binary/decimal.rs	`84.00% <ø> (ø)`
src/element/list.rs	`83.87% <ø> (+9.67%)`	⬆️
src/element/reader.rs	`93.58% <ø> (ø)`
src/element/sexp.rs	`74.19% <ø> (ø)`
src/element/writer.rs	`92.45% <ø> (ø)`
src/lib.rs	`89.21% <ø> (+0.37%)`	⬆️
src/types/coefficient.rs	`91.75% <ø> (ø)`
src/types/timestamp.rs	`93.33% <87.50%> (+0.52%)`	⬆️
src/ion_data/ion_ord.rs	`97.72% <97.72%> (ø)`
src/element/mod.rs	`91.63% <98.46%> (+0.44%)`	⬆️
... and 11 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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/element/mod.rs

src/ion_data/mod.rs

src/lib.rs

src/ion_data/ion_ord.rs

popematt · 2023-04-21T22:26:45Z

@almann or @zslayton, can you recommend any good idioms for making this also work nicely with borrowed values (i.e. &Element)?

almann

A couple questions below, but overall I am good.

src/ion_data/mod.rs

popematt · 2023-04-21T22:32:57Z

src/ion_data/mod.rs

+impl<T: IonEq> From<T> for IonData<T> {
+    fn from(value: T) -> Self {
+        IonData(value)
+    }
+}


@almann, it's here.

almann

🚢 LGTM!

src/ion_data/mod.rs

zslayton

LGTM, but one request regarding doc comments below.

src/ion_data/mod.rs

… IonOrd for borrowed values

almann

Overall, I am happy with where this is going, however I am a bit puzzled by the added Bool/Float wrappers. I think if this is needed to hide IonEq we may want to reconsider doing this.

almann · 2023-04-25T19:40:19Z

src/element/mod.rs

            (Int(this), Int(that)) => this.ion_eq(that),
-            (Float(this), Float(that)) => this.ion_eq(that),
+            (Float(this), Float(that)) => float::Float::ion_eq_f64(this, that),


This is curious, probably worth a comment indicating why this is needed.

src/ion_data/ion_eq.rs

almann · 2023-04-25T19:55:42Z

src/ion_data/ion_eq.rs

-impl IonEq for bool {
+impl<R: Deref> IonEq for R
+where
+    <R as Deref>::Target: IonEq,


I have a slight preference for this kind of formulation, but this is totally subjective.

impl<E, R> IonEq for R where E: IonEq, R: Deref<Target = E>,

Actually, there is a difference here, and it causes an error when I change it. Here's a minimum reproducible case:

// Assuming we're using // impl<E, R> IonEq for R where E: IonEq, R: Deref<Target = E> { ... } #[test] fn foo() { let element_vec: Vec<Element> = Element::read_all("foo 1 true").unwrap(); bar(element_vec.into()); } fn bar<T: IonEq>(ion: IonData<T>) { // Do something }

Fails with:

error[E0277]: the size for values of type `[element::Element]` cannot be known at compilation time

I'm assuming that this is because the trait impl is monomorphized differently depending on how the signature is formulated. (While rearranging a where clause shouldn't do something like this, in this case we have added a second type parameter and we have changed how Deref::Target is specified.)

Do you have any insight or theory on the details of why this occurs?

Aha... it needs ?Sized:

impl<E, R> IonEq for R where E: IonEq + ?Sized, R: Deref<Target = E>,

I ended up sticking with my original factoring because it minimizes the number of type parameters, though I cleaned it up by getting rid of some redundant parts.

impl<R: Deref> IonEq for R where R::Target: IonEq,

almann · 2023-04-25T19:56:58Z

src/ion_data/ion_eq.rs

-        }
-        // For all other values, fall back to mathematical equivalence
-        self == other
+        <Self as Deref>::Target::ion_eq(self, other)


With my above declaration I think you can write this as:

E::ion_eq(self, other)

almann · 2023-04-25T19:57:30Z

src/ion_data/ion_ord.rs

+impl<R: Deref> IonOrd for R
+where
+    <R as Deref>::Target: IonOrd,
+{
    fn ion_cmp(&self, other: &Self) -> Ordering {
-        T::ion_cmp(self, other)
+        <Self as Deref>::Target::ion_cmp(self, other)


Ditto for above.

src/types/bool.rs

src/types/float.rs

popematt · 2023-04-25T23:05:48Z

Hopefully this is good to go now. Latest commit includes:

Remove Float and Bool in favor of just using ion_eq_f64(), etc. functions with a detailed note about why the functions exist.
Remove someAsRef impls that this PR added that weren't actually needed.
Minor clarifications, cleanups.

almann

LGTM

popematt requested a review from zslayton April 21, 2023 16:59

almann requested changes Apr 21, 2023

View reviewed changes

popematt requested a review from almann April 21, 2023 21:30

almann approved these changes Apr 21, 2023

View reviewed changes

src/ion_data/mod.rs Outdated Show resolved Hide resolved

src/ion_data/mod.rs Outdated Show resolved Hide resolved

popematt commented Apr 21, 2023

View reviewed changes

almann approved these changes Apr 22, 2023

View reviewed changes

almann reviewed Apr 22, 2023

View reviewed changes

src/ion_data/mod.rs Outdated Show resolved Hide resolved

zslayton approved these changes Apr 24, 2023

View reviewed changes

src/ion_data/mod.rs Outdated Show resolved Hide resolved

popematt added 6 commits April 25, 2023 12:14

Adds IonData wrapper that uses Ion equivalence for equality

5c2c69a

Adds suggested changes

a1141ce

Reduces visibility of IonEq to pub(crate) and adds inherent IonEq and…

f74c11d

… IonOrd for borrowed values

Fixes incorrect ordering of timestamps and decimals

ac2b875

Adds suggested changes

ccf5fc4

Updates IonData to work with Deref impls and improves docs

fc15cdf

popematt force-pushed the ion-data-lift branch from b76f61e to fc15cdf Compare April 25, 2023 19:14

Fixes broken doc links

3f20e44

almann requested changes Apr 25, 2023

View reviewed changes

Adds suggested changes

4fda30d

popematt requested a review from almann April 25, 2023 23:05

almann approved these changes Apr 25, 2023

View reviewed changes

popematt merged commit f69a0eb into amazon-ion:main Apr 25, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds IonData wrapper that uses Ion equivalence for equality #517

Adds IonData wrapper that uses Ion equivalence for equality #517

popematt commented Apr 21, 2023

codecov bot commented Apr 21, 2023 •

edited

popematt commented Apr 21, 2023

almann left a comment

popematt Apr 21, 2023

almann left a comment •

edited

zslayton left a comment

almann left a comment

almann Apr 25, 2023

almann Apr 25, 2023

popematt Apr 25, 2023 •

edited

popematt Apr 25, 2023

popematt Apr 25, 2023

almann Apr 25, 2023

almann Apr 25, 2023

popematt commented Apr 25, 2023

almann left a comment

Adds IonData wrapper that uses Ion equivalence for equality #517

Adds IonData wrapper that uses Ion equivalence for equality #517

Conversation

popematt commented Apr 21, 2023

codecov bot commented Apr 21, 2023 • edited

Codecov Report

popematt commented Apr 21, 2023

almann left a comment

Choose a reason for hiding this comment

popematt Apr 21, 2023

Choose a reason for hiding this comment

almann left a comment • edited

Choose a reason for hiding this comment

zslayton left a comment

Choose a reason for hiding this comment

almann left a comment

Choose a reason for hiding this comment

almann Apr 25, 2023

Choose a reason for hiding this comment

almann Apr 25, 2023

Choose a reason for hiding this comment

popematt Apr 25, 2023 • edited

Choose a reason for hiding this comment

popematt Apr 25, 2023

Choose a reason for hiding this comment

popematt Apr 25, 2023

Choose a reason for hiding this comment

almann Apr 25, 2023

Choose a reason for hiding this comment

almann Apr 25, 2023

Choose a reason for hiding this comment

popematt commented Apr 25, 2023

almann left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 21, 2023 •

edited

almann left a comment •

edited

popematt Apr 25, 2023 •

edited