Add OrderSet #46

cuviper · 2017-11-21T23:03:21Z

An OrderSet<T> wraps an OrderMap<T, ()>, and also adds set-like
functionality: union, intersection, etc.

The type and all of its supporting iterators are defined in the public
set module, and OrderSet is re-exported at the crate root. There is
also a new orderset! macro for construction.

Fixes #8.

bluss

Thanks for working through this!

bluss · 2017-11-22T08:13:16Z

src/macros.rs

+    ($($value:expr,)+) => { orderset!($($value),+) };
+    ($($value:expr),*) => {
+        {
+            let _cap = ordermap!(@count $($value),*);


I'd copy the count part of the macro to this place. It's the only way to support #[macro_use(orderset)] which I'd like to do whenever possible.

bluss · 2017-11-22T08:15:19Z

src/serde.rs

+    fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
+        where A: SeqAccess<'de>
+    {
+        let mut values = OrderSet::with_capacity_and_hasher(seq.size_hint().unwrap_or(0), Default::default());


S::default() is the neater style but I see it's just copying the existing code, so that's understandable.

I updated both cases.

bluss · 2017-11-22T08:16:34Z

src/set.rs

+/// insertion and removal calls on the set. The order does not depend on the
+/// values or the hash function at all.
+///
+/// All iterators traverse the set in *the order*.


We need a comment on the order in the set operation iterators

bluss · 2017-11-22T08:17:51Z

src/set.rs

+    }
+
+    /// Return an iterator over the values that are in `self` but not `other`.
+    pub fn difference<'a, S2>(&'a self, other: &'a OrderSet<T, S2>) -> Difference<'a, T, S2>


We need a comment on the order, not sure if there's anything interesting we can say. If we prefer to not guarantee anything that's ok. Difference, in particular, seems like a simple example of guaranteeing the order -- same as self, whenever an element appears.

I think it's fine to guarantee order on all of them. The tests I wrote do make sure of this, since they compare the iterators directly against range iterators.

I've documented them all.

bluss · 2017-11-22T08:24:01Z

src/set.rs

+    fn collect<C>(self) -> C
+        where C: FromIterator<Self::Item>
+    {
+        self.iter.map(|(x, ())| x).collect()


For this to reach its goal -- delegate down to the sliceiterator.map(f).collect() -- we need to not go through the OrderMap iterator here, right? The reason we delegate down to the libcore iterators is to get their TrustedLen etc optimizations for collect.

The other iterator methods should be free of this problem.

Ah, I didn't really understand why you were overriding collect() at all, but it makes sense to leverage the unstable traits this way. I can make this use self.iter.iter.map(...).collect() -- although once we're digging into private fields, maybe it would be better for set to just create and use the underlying vec/slice iterator directly, avoiding the doubled map(..) in the other methods.

I was mostly trying to avoid having the set use private features of the map, but maybe that's not so helpful. I was somewhat planning for the possibility that you might rather publish this as a separate orderset crate.

I think it is fine to ship in the same crate, they are closely related.

cuviper · 2017-11-22T20:17:29Z

I just moved iterator_methods! to a common place too, so that can be reused by the set iterators.

bluss · 2017-11-25T17:41:21Z

Thanks a lot!

lucab · 2017-12-01T21:14:38Z

~~I'd like to point out that the behavior of the OrderSet::insert() here is actually an "upsert": if the element already exist, its relative order in the set will be updated.~~ I'm not sure if this is intentional design.

If possible, I'd like to:

~~update insert to avoid changing order on re-insertion, similarly to other languages/libraries (see top note for Java LinkedHashSet)~~
have a dedicate upsert (or similar name)
explicitly document those two different behaviors

Please note that I'm not arguing which of the two designs is better, as I have (different) use cases for both of them.

EDIT: I misread the code and made a false statement at first, striked out.

cuviper · 2017-12-01T21:24:09Z

@lucab

I'd like to point out that the behavior of the OrderSet::insert() here is actually an "upsert": if the element already exist, its relative order in the set will be updated. I'm not sure if this is intentional design.

That's not my intention, and doesn't seem to be the case:

    let mut set: OrderSet<_> = (0..10).collect();
    println!("{:?}", set);
    set.insert(3);
    println!("{:?}", set);

That doesn't perturb the order AFAICS.

lucab · 2017-12-01T21:40:41Z

@cuviper you are right, my bad. I misread OrderMap::insert() implementation and didn't check with a running example. So minor remaining questions related to my comment above:

is this guaranteed/documented somewhere?
can we have an explicit OrderSet::upsert() for cases where I need to update the order on re-insertion?

For both cases I can happily provide docs/tests/code, depending on the answers.

cuviper · 2017-12-01T21:54:33Z

IMO this is fine to guarantee. I don't think this aspect of update behavior is documented on either the map or the set, but it's actually the easier way to implement it anyway.
An upsert (which I would expand as either update_insert or insert_update) has the same basic problem as remove -- what do you do with any following elements? Should it behave like swap_remove and move the last element into the hole? Should it do an O(n) shift like Add ordered_remove with O(n) perf. #15?

cuviper · 2017-12-01T21:57:58Z

(Huh, I didn't realize upsert was already a common term in databases. Maybe that's not too bad then.)

lucab · 2017-12-01T22:13:35Z

Yes, name suggestion comes from https://wiki.postgresql.org/wiki/UPSERT. I'm not attached to it, but at the same time it already exists for this exact behavior.

Regarding removal-then-insert, as a user I'd pretty much want the behavior of #15 even if it could be very expensive performance-wise. Pointing that out in doc will probably give me (as a library user) a clear hint that I should try a different design instead first, and fallback to upsert only as a last resort.

bluss · 2017-12-01T22:57:09Z

@lucab yes, if you want strict insertion order, then this is not the right data structure.

cuviper added 6 commits November 21, 2017 12:16

Add a basic OrderSet<T> wrapping OrderMap<T,()>

7a14c38

Add serde support for OrderSet

0ce8b5a

Implement more OrderSet comparisons

12df036

Test OrderSet comparisons

24df0d1

Add macro orderset!{a, b, c, ...}

a9de40c

Implement ops for OrderSet

b210ae2

bluss self-assigned this Nov 21, 2017

bluss reviewed Nov 22, 2017

View reviewed changes

cuviper added 5 commits November 22, 2017 12:06

Make the orderset! macro independent

8dab341

Use S::default() in serde

150b377

Comment *why* collect() is forwarded

e3bb1a0

Make set iterators use vec/slice iterators directly

1ba675c

Document the order of set operations

8763070

cuviper force-pushed the set branch from 65162c3 to 8763070 Compare November 22, 2017 20:06

Move iterator_methods! to macros, so set can use it

cf1be09

bluss merged commit 52251bb into indexmap-rs:master Nov 25, 2017

cuviper deleted the set branch July 23, 2020 21:27

cuviper mentioned this pull request Sep 8, 2020

IndexMap / IndexSet comparison isn't order-aware. #153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OrderSet #46

Add OrderSet #46

cuviper commented Nov 21, 2017

bluss left a comment

bluss Nov 22, 2017

cuviper Nov 22, 2017

bluss Nov 22, 2017

cuviper Nov 22, 2017

bluss Nov 22, 2017

cuviper Nov 22, 2017

bluss Nov 22, 2017

cuviper Nov 22, 2017

cuviper Nov 22, 2017

bluss Nov 22, 2017

bluss Nov 22, 2017

cuviper Nov 22, 2017

bluss Nov 22, 2017

cuviper Nov 22, 2017

cuviper commented Nov 22, 2017

bluss commented Nov 25, 2017

lucab commented Dec 1, 2017 •

edited

Loading

cuviper commented Dec 1, 2017

lucab commented Dec 1, 2017 •

edited

Loading

cuviper commented Dec 1, 2017

cuviper commented Dec 1, 2017

lucab commented Dec 1, 2017 •

edited

Loading

bluss commented Dec 1, 2017

Add OrderSet #46

Add OrderSet #46

Conversation

cuviper commented Nov 21, 2017

bluss 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

cuviper commented Nov 22, 2017

bluss commented Nov 25, 2017

lucab commented Dec 1, 2017 • edited Loading

cuviper commented Dec 1, 2017

lucab commented Dec 1, 2017 • edited Loading

cuviper commented Dec 1, 2017

cuviper commented Dec 1, 2017

lucab commented Dec 1, 2017 • edited Loading

bluss commented Dec 1, 2017

lucab commented Dec 1, 2017 •

edited

Loading

lucab commented Dec 1, 2017 •

edited

Loading

lucab commented Dec 1, 2017 •

edited

Loading