From cd58e49c788ed47ca59c7ac6e4945cd20bad666f Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Tue, 6 Jan 2015 16:13:17 -0800 Subject: [PATCH] remove 'i' and nasty rounding; test --- pkg/api/resource/quantity.go | 46 ++++++++----------- pkg/api/resource/quantity_test.go | 76 +++++++++++++++++++++++++------ pkg/api/resource/suffix.go | 4 +- 3 files changed, 82 insertions(+), 44 deletions(-) diff --git a/pkg/api/resource/quantity.go b/pkg/api/resource/quantity.go index 7fc2e633e34..3b04fcd91c6 100644 --- a/pkg/api/resource/quantity.go +++ b/pkg/api/resource/quantity.go @@ -41,22 +41,20 @@ import ( // ::= "+" | "-" // ::= | // ::= | | -// ::= i | Ki | Mi | Gi | Ti | Pi | Ei +// ::= Ki | Mi | Gi | Ti | Pi | Ei // (International System of units; See: http://physics.nist.gov/cuu/Units/binary.html) // ::= m | "" | k | M | G | T | P | E // (Note that 1024 = 1Ki but 1000 = 1k; I didn't choose the capitalization.) // ::= "e" | "E" // // No matter which of the three exponent forms is used, no quantity may represent -// a number greater than 2^63-1 in magnitude, nor may it have more than 3 digits -// of precision. Numbers larger or more precise will be capped or rounded. +// a number greater than 2^63-1 in magnitude, nor may it have more than 3 decimal +// places. Numbers larger or more precise will be capped or rounded up. // (E.g.: 0.1m will rounded up to 1m.) // This may be extended in the future if we require larger or smaller quantities. // // When a Quantity is parsed from a string, it will remember the type of suffix // it had, and will use the same type again when it is serialized. -// One exception: numbers with a Binary SI suffix less than bigOne will be changed -// to Decimal SI suffix. E.g., .5i becomes 500m. [NOT 512m!] // // Before serializing, Quantity will be put in "canonical form". // This means that Exponent/suffix will be adjusted up or down (with a @@ -137,10 +135,12 @@ var ( big1024 = big.NewInt(1024) // Commonly needed inf.Dec values-- treat as read only! - decZero = inf.NewDec(0, 0) - decOne = inf.NewDec(1, 0) - decMinusOne = inf.NewDec(-1, 0) - decThousand = inf.NewDec(1000, 0) + decZero = inf.NewDec(0, 0) + decOne = inf.NewDec(1, 0) + decMinusOne = inf.NewDec(-1, 0) + decThousand = inf.NewDec(1000, 0) + dec1024 = inf.NewDec(1024, 0) + decMinus1024 = inf.NewDec(-1024, 0) // Largest (in magnitude) number allowed. maxAllowed = inf.NewDec((1<<63)-1, 0) // == max int64 @@ -241,16 +241,14 @@ func (q *Quantity) Canonicalize() (string, suffix) { switch format { case DecimalExponent, DecimalSI: case BinarySI: - switch q.Amount.Cmp(decZero) { - case 0: // exactly equal 0, that's fine - case 1: // greater than 0 - if q.Amount.Cmp(decOne) < 0 { - // This avoids rounding and hopefully confusion, too. - format = DecimalSI - } - case -1: - if q.Amount.Cmp(decMinusOne) > 0 { - // This avoids rounding and hopefully confusion, too. + if q.Amount.Cmp(decMinus1024) > 0 && q.Amount.Cmp(dec1024) < 0 { + // This avoids rounding and hopefully confusion, too. + format = DecimalSI + } else { + tmp := &inf.Dec{} + tmp.Round(q.Amount, 0, inf.RoundUp) + if tmp.Cmp(q.Amount) != 0 { + // Don't lose precision-- show as DecimalSI format = DecimalSI } } @@ -281,16 +279,8 @@ func (q *Quantity) Canonicalize() (string, suffix) { case BinarySI: tmp := &inf.Dec{} tmp.Round(q.Amount, 0, inf.RoundUp) - amount := tmp.UnscaledBig() - exponent := int(-q.Amount.Scale()) - // Apply the (base-10) shift. This will lose any fractional - // part, which is intentional. - for exponent > 0 { - amount.Mul(amount, bigTen) - exponent-- - } - amount, exponent = removeFactors(amount, big1024) + amount, exponent := removeFactors(tmp.UnscaledBig(), big1024) suffix, _ := quantitySuffixer.construct(2, exponent*10, format) number := amount.String() return number, suffix diff --git a/pkg/api/resource/quantity_test.go b/pkg/api/resource/quantity_test.go index f34873ffb89..9b0ab7281b1 100644 --- a/pkg/api/resource/quantity_test.go +++ b/pkg/api/resource/quantity_test.go @@ -62,8 +62,18 @@ func TestQuantityParse(t *testing.T) { expect Quantity }{ {"0", Quantity{dec(0, 0), DecimalSI}}, + {"0m", Quantity{dec(0, 0), DecimalSI}}, + {"0Ki", Quantity{dec(0, 0), BinarySI}}, + {"0k", Quantity{dec(0, 0), DecimalSI}}, + {"0Mi", Quantity{dec(0, 0), BinarySI}}, + {"0M", Quantity{dec(0, 0), DecimalSI}}, + {"0Gi", Quantity{dec(0, 0), BinarySI}}, + {"0G", Quantity{dec(0, 0), DecimalSI}}, + {"0Ti", Quantity{dec(0, 0), BinarySI}}, + {"0T", Quantity{dec(0, 0), DecimalSI}}, + // Binary suffixes - {"9i", Quantity{dec(9, 0), BinarySI}}, + {"1Ki", Quantity{dec(1024, 0), BinarySI}}, {"8Ki", Quantity{dec(8*1024, 0), BinarySI}}, {"7Mi", Quantity{dec(7*1024*1024, 0), BinarySI}}, {"6Gi", Quantity{dec(6*1024*1024*1024, 0), BinarySI}}, @@ -153,19 +163,13 @@ func TestQuantityParse(t *testing.T) { {"0.5Mi", Quantity{dec(.5*1024*1024, 0), BinarySI}}, {"0.05Gi", Quantity{dec(536870912, -1), BinarySI}}, {"0.025Ti", Quantity{dec(274877906944, -1), BinarySI}}, - // These get rounded though - {"0.0001i", Quantity{dec(1, -3), DecimalSI}}, - {"0.005i", Quantity{dec(5, -3), DecimalSI}}, - {"0.05i", Quantity{dec(50, -3), DecimalSI}}, - // Also, if below you expect (512, -3), you're wrong in two ways: - // In the sequence [1024*1024*1024, 1024*1024, 1024, ?], the last term is "1" not 1/1024. - // Even if it were, 500 * 1/1024 = .48828125, NOT .512 - // I cannot recommend using this feature, it is confusing. - {"0.5i", Quantity{dec(500, -3), DecimalSI}}, // Things written by trolls + {"0.000001Ki", Quantity{dec(2, -3), DecimalSI}}, // rounds up, changes format {".001", Quantity{dec(1, -3), DecimalSI}}, + {".0001k", Quantity{dec(100, -3), DecimalSI}}, {"1.", Quantity{dec(1, 0), DecimalSI}}, + {"1.G", Quantity{dec(1, 9), DecimalSI}}, } for _, item := range table { @@ -220,6 +224,9 @@ func TestQuantityParse(t *testing.T) { "0.1mi", "0.1am", "aoeu", + ".5i", + "1i", + "-3.01i", } for _, item := range invalid { _, err := ParseQuantity(item) @@ -239,7 +246,7 @@ func TestQuantityString(t *testing.T) { {Quantity{dec(6*1024, 0), BinarySI}, "6Ki"}, {Quantity{dec(1001*1024*1024*1024, 0), BinarySI}, "1001Gi"}, {Quantity{dec(1024*1024*1024*1024, 0), BinarySI}, "1Ti"}, - {Quantity{dec(5, 0), BinarySI}, "5i"}, + {Quantity{dec(5, 0), BinarySI}, "5"}, {Quantity{dec(500, -3), BinarySI}, "500m"}, {Quantity{dec(1, 9), DecimalSI}, "1G"}, {Quantity{dec(1000, 6), DecimalSI}, "1G"}, @@ -253,10 +260,11 @@ func TestQuantityString(t *testing.T) { {Quantity{dec(300, 6), DecimalSI}, "300M"}, {Quantity{dec(1, 12), DecimalSI}, "1T"}, {Quantity{dec(1234567, 6), DecimalSI}, "1234567M"}, - {Quantity{dec(1234567, -3), BinarySI}, "1235i"}, + {Quantity{dec(1234567, -3), BinarySI}, "1234567m"}, {Quantity{dec(3, 3), DecimalSI}, "3k"}, + {Quantity{dec(1025, 0), BinarySI}, "1025"}, {Quantity{dec(0, 0), DecimalSI}, "0"}, - {Quantity{dec(0, 0), BinarySI}, "0i"}, + {Quantity{dec(0, 0), BinarySI}, "0"}, {Quantity{dec(1, 9), DecimalExponent}, "1e9"}, {Quantity{dec(1, -3), DecimalExponent}, "1e-3"}, {Quantity{dec(80, -3), DecimalExponent}, "80e-3"}, @@ -287,6 +295,44 @@ func TestQuantityString(t *testing.T) { } } +func TestQuantityParseEmit(t *testing.T) { + table := []struct { + in string + expect string + }{ + {"1Ki", "1Ki"}, + {"1Mi", "1Mi"}, + {"1Gi", "1Gi"}, + {"1024Mi", "1Gi"}, + {"1000M", "1G"}, + {".000001Ki", "2m"}, + } + + for _, item := range table { + q, err := ParseQuantity(item.in) + if err != nil { + t.Errorf("Couldn't parse %v", item.in) + continue + } + if e, a := item.expect, q.String(); e != a { + t.Errorf("%#v: expected %v, got %v", item.in, e, a) + } + } + for _, item := range table { + q, err := ParseQuantity("-" + item.in) + if err != nil { + t.Errorf("Couldn't parse %v", item.in) + continue + } + if q.Amount.Cmp(decZero) == 0 { + continue + } + if e, a := "-"+item.expect, q.String(); e != a { + t.Errorf("%#v: expected %v, got %v", item.in, e, a) + } + } +} + var fuzzer = fuzz.New().Funcs( func(q *Quantity, c fuzz.Continue) { q.Amount = &inf.Dec{} @@ -347,9 +393,9 @@ func TestMilliNewSet(t *testing.T) { {1, DecimalSI, "1m", true}, {1000, DecimalSI, "1", true}, {1234000, DecimalSI, "1234", true}, - {1024, BinarySI, "2i", false}, // Rounded up + {1024, BinarySI, "1024m", false}, // Format changes {1000000, "invalidFormatDefaultsToExponent", "1e3", true}, - {1024 * 1024, BinarySI, "1049i", false}, + {1024 * 1024, BinarySI, "1048576m", false}, // Format changes } for _, item := range table { diff --git a/pkg/api/resource/suffix.go b/pkg/api/resource/suffix.go index a3a4012aa39..6f0dbd5f279 100644 --- a/pkg/api/resource/suffix.go +++ b/pkg/api/resource/suffix.go @@ -73,13 +73,15 @@ type suffixHandler struct { func newSuffixer() suffixer { sh := &suffixHandler{} - sh.binSuffixes.addSuffix("i", bePair{2, 0}) sh.binSuffixes.addSuffix("Ki", bePair{2, 10}) sh.binSuffixes.addSuffix("Mi", bePair{2, 20}) sh.binSuffixes.addSuffix("Gi", bePair{2, 30}) sh.binSuffixes.addSuffix("Ti", bePair{2, 40}) sh.binSuffixes.addSuffix("Pi", bePair{2, 50}) sh.binSuffixes.addSuffix("Ei", bePair{2, 60}) + // Don't emit an error when trying to produce + // a suffix for 2^0. + sh.decSuffixes.addSuffix("", bePair{2, 0}) sh.decSuffixes.addSuffix("m", bePair{10, -3}) sh.decSuffixes.addSuffix("", bePair{10, 0})