Fix Int indexes to make them sortable. #114
Conversation
There was a problem hiding this comment.
Minor comments but looks good!
Our team had some non-blocking comments about the readability of TestIntFieldIndexSortability - do you think it might be clearer to represent the numbers in 0b_ binary form to better see the bit comparisons between the inputs?
Not all the test cases but some of the edge cases (comparing -0b_10000000 and 0b_1 for instance) that would have failed before this PR would be nice to see.
index.go
Outdated
| switch size { | ||
| case 1: | ||
| buf[0] = uint8(scaled) | ||
| case 2: | ||
| binary.BigEndian.PutUint16(buf, uint16(scaled)) | ||
| case 4: | ||
| binary.BigEndian.PutUint32(buf, uint32(scaled)) | ||
| case 8: | ||
| binary.BigEndian.PutUint64(buf, uint64(scaled)) | ||
| } |
There was a problem hiding this comment.
We should add a default: panic case in case there's a programmer error in passing an unsupported size
| func encodeInt(val int64, size int) []byte { | ||
| buf := make([]byte, size) | ||
|
|
||
| scaled := val ^ int64(-1<<(size*8-1)) |
There was a problem hiding this comment.
Could we add a comment explaining what this line does?
|
I made the two changes with inline comments, but I can't figure out how to make the sortability tests more readable with the binary formatting. Some of the numbers would literally be 64 characters long! If you want to send over something more readable I would be happy to include it in this PR, or I won't have any hard feelings if someone wants to fix this up after it's in. |
|
@kisunji do you know when this might make it into a release? I think i'm actually being bitten by this bug now. |
Just tagged a new version now! |
Fixes #113