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

Improved document key sorting and support for struct document ids #34

Closed
wants to merge 3 commits into from
Closed

Improved document key sorting and support for struct document ids #34

wants to merge 3 commits into from

Conversation

mjs
Copy link
Contributor

@mjs mjs commented Oct 7, 2014

Removed unnecessary resort of document keys


Introduce sortableDocKey

Precompute the nature and sort value for each doc key once instead of each time Less is called. This trades slight additional complexity and memory usage for sort speed. This also paves the way for sorting of struct document keys.


Support sorting of struct based document ids

Menno Smits added 3 commits October 7, 2014 08:25
docKeys() already sorts the returned document keys.
Precompute the nature and sort value for each doc key once instead of
each time Less is called. This trades slight additional complexity and
memory usage for sort speed. This also paves the way for sorting of
struct document keys.
@mjs mjs mentioned this pull request Oct 7, 2014
@niemeyer
Copy link
Contributor

niemeyer commented Oct 7, 2014

That's improving the previous approach with caching, but the underlying concept is still not good. There's no reason to pay the cost of marshaling of these structs simply for obtaining a well defined order. It's also pretending to work for things that actually do not work. Marshal will marshal anything, including things with an undefined order. These should continue to break.

We should be able to easily have an ordering mechanism for the structs we care about without resorting to marshaling.

@niemeyer niemeyer closed this Oct 12, 2014
@mjs mjs deleted the dockey-sorting-and-struct-ids branch May 8, 2015 03:47
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