Fix unstable map ordering for specific keys with numbers

[prashantv]

When comparing runes, don't skip equal runes if they are digits. This avoids issues when comparing values like 100 vs 11 since dropping the 1 causes a comparison of 00 vs 1.

Fixes #194

[prashantv]

Hi @niemeyer, would you be able to look at this change? I'd love to get this merged in and avoid maintaining a fork!

[geofffranks]

I would love to see this merged as well

[rogpeppe]

Thanks for this, but I'll need to think some more before submitting it. The keyList.Less method looks problematic already, in fact, as it looks like it's assuming that unicode.IsDigit implies that the rune value is between ASCII '0' and '9'.

[rogpeppe]

I suspect this may not be quite right. The original code always returned after comparing numbers, but this now means that we can carry on instead. At the least, this introduces quadratic behaviour which might be a problem for long keys with many decimal digits in. I'll need to think some more about this.

[aviadl]

@rogpeppe is there a chance that this would be fixed anytime soon, it really hurts us on some scenarios

[aviadl]

Can i offer to simple do a string comparison on the last case:
So the less function would look like this:

func (l keyList) Less(i, j int) bool {
	a := l[i]
	b := l[j]
	ak := a.Kind()
	bk := b.Kind()
	for (ak == reflect.Interface || ak == reflect.Ptr) && !a.IsNil() {
		a = a.Elem()
		ak = a.Kind()
	}
	for (bk == reflect.Interface || bk == reflect.Ptr) && !b.IsNil() {
		b = b.Elem()
		bk = b.Kind()
	}
	af, aok := keyFloat(a)
	bf, bok := keyFloat(b)
	if aok && bok {
		if af != bf {
			return af < bf
		}
		if ak != bk {
			return ak < bk
		}
		return numLess(a, b)
	}
	if ak != reflect.String || bk != reflect.String {
		return ak < bk
	}
	ar, br := a.String(), b.String()
	return ar < br
}

My mine use case is that the output would be consistent.
I don't really care about the order of map, as long as it is consitent

[jfmyers9]

Hey @rogpeppe,

We are also running into issues with the non-stable ordering in maps. Is there a timeline for when this or another fix may be merged?

Best,

@jfmyers9 && @tjvman

[niemeyer]

@jfmyers9 I'll fix it this week, somehow.

[cppforlife]

@niemeyer any updates?

[niemeyer]

This is fixed in dcd83b3, without significant changes to the algorithm overall behavior.

Please let me know how it goes.

[HeavyWombat]

Just in case someone comes here like me and thinks this is completely solved, I can still reproduce unstable map ordering with a use case I have. Run the following code a couple of times and you will observe different orders:

package main

import (
	"fmt"

	"gopkg.in/yaml.v2"
)

const example = `
properties:
  cc:
    quota_definitions:
      q256MB:
        non_basic_services_allowed: true
      q2GB:
        non_basic_services_allowed: true
      q4GB:
        non_basic_services_allowed: true
`

func main() {
	var data interface{}
	if err := yaml.Unmarshal([]byte(example), &data); err == nil {
		if out, err := yaml.Marshal(data); err == nil {
			fmt.Println(string(out))
		}
	}
}