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

encode maps keeping keys in order. #30

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benoitc
Copy link
Contributor

@benoitc benoitc commented Oct 22, 2017

to keep maps in order, we first sort it by keys by transforming them as
list. Then we process the keys/values in order.

ex:

2> B = sext:encode(#{ a => 1, b=>1 }).
<<17,1,0,0,0,2,12,176,128,8,10,0,0,0,2,12,177,0,8,10,0,0,
  0,2>>
3> B = sext:encode(#{ b => 1, a=>1 }).
<<17,1,0,0,0,2,12,176,128,8,10,0,0,0,2,12,177,0,8,10,0,0,
  0,2>>

to keep maps in order, we first sort it by keys by transforming them as
list. Then we process the keys/values in order.

ex:

2> B = sext:encode(#{ a => 1, b=>1 }).
<<17,1,0,0,0,2,12,176,128,8,10,0,0,0,2,12,177,0,8,10,0,0,
  0,2>>
3> B = sext:encode(#{ b => 1, a=>1 }).
<<17,1,0,0,0,2,12,176,128,8,10,0,0,0,2,12,177,0,8,10,0,0,
  0,2>>
@uwiger
Copy link
Owner

uwiger commented Oct 23, 2017

I noticed that there are no quickcheck tests for maps, so I started adding some. This highlighted that some more implementation is needed – not least prefix encoding.

@benoitc
Copy link
Contributor Author

benoitc commented Oct 23, 2017

i can probably add that. Did you already started the work on it?

@uwiger
Copy link
Owner

uwiger commented Oct 23, 2017

I started working on it. I got pretty far, but then – after starting a 1M test run of prop_sort(), discovered that the keysort order in maps differs from the normal term ordering: floats are always considered larger than ints.

Needless to say, this poses a significant challenge for sext. I haven't yet figured out how to solve that in a backwards-compatible way (or even breaking compatibility just for map keys).

@uwiger uwiger mentioned this pull request Oct 24, 2017
@uwiger
Copy link
Owner

uwiger commented Oct 24, 2017

@benoitc could you check out #31 please?

@benoitc
Copy link
Contributor Author

benoitc commented Nov 6, 2017

@uwiger sorry I missed it ... I will later today thanks :)

@uwiger
Copy link
Owner

uwiger commented Nov 6, 2017

Actually, we (Thomas Arts and I) have determined that there is still some work needed. I'll try to get back to it as quickly as I can.

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