Skip to content

encode maps keeping keys in order.#30

Open
benoitc wants to merge 2 commits into
uwiger:masterfrom
benoitc:fix/map-cmp
Open

encode maps keeping keys in order.#30
benoitc wants to merge 2 commits into
uwiger:masterfrom
benoitc:fix/map-cmp

Conversation

@benoitc

@benoitc benoitc commented Oct 22, 2017

Copy link
Copy Markdown
Contributor

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

uwiger commented Oct 23, 2017

Copy link
Copy Markdown
Owner

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

benoitc commented Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

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

@uwiger

uwiger commented Oct 23, 2017

Copy link
Copy Markdown
Owner

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

uwiger commented Oct 24, 2017

Copy link
Copy Markdown
Owner

@benoitc could you check out #31 please?

@benoitc

benoitc commented Nov 6, 2017

Copy link
Copy Markdown
Contributor Author

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

@uwiger

uwiger commented Nov 6, 2017

Copy link
Copy Markdown
Owner

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