Skip to content

Comments

encode maps keeping keys in order.#30

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

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

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