Skip to content

Conversation

jeff303
Copy link
Contributor

@jeff303 jeff303 commented Sep 16, 2020

TODO: figure out how to handle transients (i.e. submap!)

Adding new SubMap protocol to handle the different implementations of key selection (including one that maintains order to be used for sorted-map)

Adding test to ensure order preserved for a large map

@jeff303
Copy link
Contributor Author

jeff303 commented Sep 16, 2020

I'm not sure how to handle transients here (i.e. submap!).

  • There is no such thing as a transient version of PersistentTreeMap, so the problem where the previous navigator didn't preserve order doesn't apply there
  • For TransientArrayMap and TransientHashMap, the order already appears preserved by select-keys (see REPL session below)
# for TransientHashMap
(select-keys (transient {:a 1 :b 2 :c 3 :d 4 :e 5 :f 6 :g 7 :h 8 :i 9 :j 10 :k 11 :l 12 :m 13}) [:a :c :e :g :i :k :m])
{:a 1, :c 3, :e 5, :g 7, :i 9, :k 11, :m 13}
# for TransientArrayMap
(select-keys (transient {:a 1 :b 2 :c 3 :d 4 :e 5 :f 6 :g 7}) [:g :f :e :d :c :b :a])
{:g 7, :f 6, :e 5, :d 4, :c 3, :b 2, :a 1}

@nathanmarz
Copy link
Collaborator

Couple comments:

  • I think the only property that's important to maintain is order in the case of sorted-map. So I think the code can be simplified by having a case for Object which does select-keys and only having a special case for PersistentTreeMap.
  • The tests should test against a sorted map with a custom comparator and verify the submap has the same custom comparator.

@nathanmarz
Copy link
Collaborator

Also, transients aren't supported on PersistentTreeMap, so submap! doesn't need any changes.

@jeff303 jeff303 force-pushed the issue-235 branch 2 times, most recently from 85361b1 to a8151c6 Compare September 18, 2020 16:49
…bs#235

Adding new SubMap protocol to provide a different implementation of
select-keys for TreeMap

Adding tests to ensure order preserved for a large map, as well as one
with a custom comparator
@jeff303
Copy link
Contributor Author

jeff303 commented Sep 18, 2020

Added the test for the actual comparator (couldn't quickly figure out if/how to make that work in cljs).

Also, transients aren't supported on PersistentTreeMap, so submap! doesn't need any changes

OK, thanks for the confirmation. The starter comment on #235 is what got me looking at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants