Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

submap sub map should maintain properties of parent map #235 #296

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

None yet

2 participants