Migrate Number from float64 to big.Float #191
Comments
|
Hey @gavv can assign this to me? I am little busy with switching jobs but this problem seems interesting. Would pickup as soon as I get some time. |
|
Sure, thanks! |
|
Hey @gavv converting type Number struct {
noCopy noCopy
chain *chain
value float64
}to type Number struct {
noCopy noCopy
chain *chain
value big.Float
}will cause |
Yes, that's what I was suggesting.
Yes. Yeah, sorry, I was not clear on this. The idea is the following:
This is actually a common approach for breaking APIs: deprecate, wait, then remove or change. Wait period gives users time to reacts to the change without hurry. |
|
Hey @gavv I have few questions.
find places where we decode JSON: canonValue, Response, WebsocketMessage; configure JSON decoder to use json.Number instead of float64 (see https://pkg.go.dev/encoding/json#Decoder.UseNumber) ensure that canonValue() and other canonization functions does not cause precision loss For ☝️ do you suggest to have a type assertion wherever the functions are called? Because as far as I can see. All of the canon functions are using interfaces to decode json rather than any specific type. |
|
|
So let's suppose we have this function - func canonValue(opChain *chain, in interface{}) (interface{}, bool) {
b, err := json.Marshal(in)
if err != nil {
opChain.fail(AssertionFailure{
Type: AssertValid,
Actual: &AssertionValue{in},
Errors: []error{
errors.New("expected: marshalable value"),
err,
},
})
return nil, false
}
var out interface{}
if err := json.Unmarshal(b, &out); err != nil {
opChain.fail(AssertionFailure{
Type: AssertValid,
Actual: &AssertionValue{in},
Errors: []error{
errors.New("expected: unmarshalable value"),
err,
},
})
return nil, false
}
return out, true
}How do you want it to be modified to use json.Number? Because from what I can see we are using Or Am I understanding this wrong? If so can you please explain this? |
|
Ah, I see. canonValue and other methods use json.Unmarshal to unmarshal object, array, etc. When json.Unmarshal encounters a number somewhere in json, it unmarshals it into float64. This may cause precision loss if the number can't be represented as float without loss (if it's a large 64-bit integer for example).
We need to use this option everywhere when we unmarshal json, to avoid precision loss. Our maps, arrays, etc. should have json.Number instead of float64. When the user will create |
|
Understood. Thanks. 😄 |
|
Some thoughts:
|
|
Sure thing. Then let me do one thing. I will make the current changes pass the checks, so that these can be merged. Later on I will divide newer development into multiple tasks. |
|
Note that if you change decoding to use json.Number, it will break many things, and it will take time to fix all of them. I'd recommend to cut it from first PR. |
|
But I already started making those fixes. 😅 |
|
Oh I see, great :) |
|
Hey @gavv Test cases are failing in t.Run("Decode into empty interface", func(t *testing.T) {
reporter := newMockReporter(t)
value := NewNumber(reporter, 10.1)
var target interface{}
value.Decode(&target)
value.chain.assertNotFailed(t)
assert.Equal(t, *big.NewFloat(10.1), target)
})Below is the output of test case. The error is caused because |
|
I think you need to convert big.Float to json.Number and then pass it to json decoder. |
I don't think we can directly convert big.Float to json.Number because big.Float is a struct. Rather I was using a reverse conversion and then passing it to json decoder. Check out this snippet - func canonNumberDecode(opChain *chain, value big.Float, target interface{}) {
if target == nil {
opChain.fail(AssertionFailure{
Type: AssertUsage,
Errors: []error{
errors.New("unexpected nil target argument"),
},
})
return
}
t := reflect.Indirect(reflect.ValueOf(target)).Kind()
fmt.Println("type", t)
switch t {
case reflect.Float64:
f, _ := value.Float64()
canonDecode(opChain, f, target)
case reflect.Float32:
f, _ := value.Float32()
canonDecode(opChain, f, target)
case reflect.Int8:
i, _ := value.Int64()
canonDecode(opChain, i, target)
case reflect.Int16:
i, _ := value.Int64()
canonDecode(opChain, i, target)
case reflect.Int32:
i, _ := value.Int64()
canonDecode(opChain, i, target)
case reflect.Int64:
i, _ := value.Int64()
canonDecode(opChain, i, target)
case reflect.Int:
i, _ := value.Int64()
fmt.Println("int", i)
canonDecode(opChain, i, target)
case reflect.Uint8:
i, _ := value.Int64()
canonDecode(opChain, i, target)
case reflect.Uint16:
i, _ := value.Int64()
canonDecode(opChain, i, target)
case reflect.Uint32:
i, _ := value.Int64()
canonDecode(opChain, i, target)
case reflect.Uint64:
i, _ := value.Int64()
canonDecode(opChain, i, target)
case reflect.Uint:
i, _ := value.Int64()
canonDecode(opChain, i, target)
case reflect.Interface:
f, _ := value.Float64()
canonDecode(opChain, f, target)
default:
canonDecode(opChain, value, target)
}
} |
|
This would work too... does it handle wrapped types, e.g. I'd rather use json.Number + json.Unmarshal because it's less code and we can be sure that we didn't miss any details. json.Number is just a wrapped string, so "conversion" is something like |
The problem with using json.Number and json.Unmarshal is json.Number uses string. But the target can be of any type. So if an user wants to convert big.Float to int. |
|
Hey @gavv there's one more issue. There is no NaN representation in big.Float/big.Int . There is only a type v1 := NewNumber(reporter, math.NaN())The above snippet in v6 := NewNumber(reporter, math.NaN())
v6.NotInDelta(1234.0, 0.1)
v6.chain.assertFailed(t)this portion of the test case fails. Can I use |
Oh, cool.
Sorry, I don't know why I mentioned json.Number at all, it has nothing to do with this. What I was trying to say is that we can convert big.Float to string and use stock json decoder, like this: https://go.dev/play/p/uyl7R19QBYa Can this work for us? |
|
Regarding NaN. We can't use any other floating point value to represent NaN: zero, -inf, +inf - all are valid values that can be passed by user and we should not confuse them with NaN. The only option we have is to handle NaN specially. Since it seems that NaN is the only unsupported value (both -Inf and +Inf are seem to be supported), I think the easiest way to handle it is to use Then we'll need to check for nil in all methods. And I guess we'll need to add NaN tests for all methods. |
|
Hey @gavv#6022 I don't think our approach is right. For example :- func TestArray_Decode(t *testing.T) {
t.Run("target is empty interface", func(t *testing.T) {
reporter := newMockReporter(t)
testValue := []interface{}{"Foo", 123.0}
arr := NewArray(reporter, testValue)
var target interface{}
arr.Decode(&target)
arr.chain.assertNotFailed(t)
assert.Equal(t, testValue, target)
})
t.Run("target is slice of empty interfaces", func(t *testing.T) {
reporter := newMockReporter(t)
testValue := []interface{}{"Foo", 123.0}
arr := NewArray(reporter, testValue)
var target []interface{}
arr.Decode(&target)
arr.chain.assertNotFailed(t)
assert.Equal(t, testValue, target)
})
t.Run("target is slice of structs", func(t *testing.T) {
reporter := newMockReporter(t)
type S struct {
Foo int `json:"foo"`
}
actualStruct := []S{{123}, {456}}
testValue := []interface{}{
map[string]interface{}{
"foo": 123,
},
map[string]interface{}{
"foo": 456,
},
}
arr := NewArray(reporter, testValue)
var target []S
arr.Decode(&target)
arr.chain.assertNotFailed(t)
assert.Equal(t, actualStruct, target)
})
t.Run("target is unmarshable", func(t *testing.T) {
reporter := newMockReporter(t)
testValue := []interface{}{"Foo", 123.0}
arr := NewArray(reporter, testValue)
arr.Decode(123)
arr.chain.assertFailed(t)
})
t.Run("target is nil", func(t *testing.T) {
reporter := newMockReporter(t)
testValue := []interface{}{"Foo", 123.0}
arr := NewArray(reporter, testValue)
arr.Decode(nil)
arr.chain.assertFailed(t)
})
}Check the var Now when we decode the function will internally call func jsonDecode(opChain *chain, b []byte, target interface{}) {
reader := bytes.NewReader(b)
dec := json.NewDecoder(reader)
dec.UseNumber()
for {
if err := dec.Decode(target); err == io.EOF || target == nil {
break
} else if err != nil {
opChain.fail(AssertionFailure{
Type: AssertValid,
Actual: &AssertionValue{target},
Errors: []error{
errors.New("expected: value can be decoded into target argument"),
},
})
return
}
}
}To handle the issue I wrote something like func (a *Array) Decode(target interface{}) *Array {
opChain := a.chain.enter("Decode()")
defer opChain.leave()
if opChain.failed() {
return a
}
canonDecode(opChain, a.value, target)
if targetPointer, ok := target.(*interface{}); ok {
if data, ok := (*targetPointer).([]interface{}); ok {
for i, d := range data {
if v, ok := d.(json.Number); ok {
if f, err := v.Float64(); err == nil {
fmt.Println("json", f)
if hasPrecisionLoss(f) {
numStr := v.String()
num, _ := big.NewFloat(0).SetString(numStr)
data[i] = num
} else {
fmt.Println("json", f)
data[i] = f
}
}
}
}
*targetPointer = data
}
}
return a
}But ☝️ will only work when the target is Now, I don't see any way to convert json.Number back to either float64 or big.Float without manually checking for all the cases where the structure may vary. |
|
Hi, and sorry for delay. When we were discussing this in chat, my thoughts were the following (I think I didn't explain all of the idea accurately enough):
A lot of bullet points... but I tried to give comprehensive overview this time. In short, there are actually only a few key points:
And that's it. Does this solve the problem you mentioned? BTW I want to thank you for patience and all these long discussions on discord. I was not expecting all the obstacles when creating the task, and I think these discussions have generated a rather good solution. |
Now I understand. I although have an issue with the overhead of conversion. But I think this is inevitable if we are keep it compatible. In v3 we can remove this overhead easily as we will be allowed to have breaking changes. Even I wasn't expecting so many hurdles. But I guess it's good that had this detail conversation. It is much clear now. |
|
Regarding overhead, I suppose it's comparably with json marshalling and unmarshalling, which is as well based on reflect. So here we don't change much in terms of performance. |
Got it. Btw for now what I am following is every number that will not cause precision loss will be converted to float64 by default and numbers which will cause precision loss will be converted to big.Float. And this will be the default behaviour for now. Once I fix all the test cases and this PR is merged. I will add a config var which will use either float64 or big.Float as per the user choice. I think this would be the ideal way to go forward. Otherwise it will increase complexity to a already complex PR. |
Currently, Number always converts its value to float64. In many cases it works well, however float64 can't represent big integer values (larger than 2^62) without precision loss.
Actually, there is no native type that can represent ALL values of int64, uint64, and float64 without loss. However, there is a library type that can do it: big.Float (see https://pkg.go.dev/math/big).
Here are the steps:
replace underlying value of Number from float64 to big.Float
by default, use precision that is sufficient to represent all values of int64, uint64, and float64 without loss
allow user to set higher precision in Config (this may be useful if user wants to parse Number from String that contains bigger integer or float)
rework canonNumber() to return big.Float instead of float64; it should automatically detect precision and create big.Float from any of these types:
type MyInt int)rework NewNumber constructor: it should accept interface{} instead of float64 and use canonNumber() to convert it to big.Float
rework all Number operations, so that they will operate on big.Float values (one stored in Number, another returned from canonNumber for operation argument)
rework Number.EqualDelta and Number.NotEqualDelta: they should accept interface{} instead of float64; this will allow user to pass big.Float instead of float64, when necessary
rework JSON decoding:
find places where we decode JSON: canonValue, Response, WebsocketMessage; configure JSON decoder to use json.Number instead of float64 (see https://pkg.go.dev/encoding/json#Decoder.UseNumber)
ensure that canonValue() and other canonization functions does not cause precision loss
rework Number creation
Expect.Number() should accept interface{} instead of float64
rework Value.Number() - it should not force float64 type
rework String.AsNumber() - it should correctly parse big floats and integers
find all invocations to newNumber() that have unnecessary casts to float64 and pass integers instead; e.g.
newNumber(a.chain, float64(len(a.value)))can be now replaced withnewNumber(a.chain, len(a.value))because Number now supports integersadjust equality checks:
find all places that do deep equality checks (e.g. in Object, Array) using reflect.DeepEqual
ensure that they will work when original values have different representations for same numbers (e.g. json.Number vs float64 vs big.Float)
cover these cases with unit tests
add new getters for Number:
add 4 new methods: AsInt (returns int64), AsUint (return uint64), AsFloat (return float64), AsBig (return big.Float)
each method should report failure if underlying value can't be converted to requested types without loss
add IsBig (similar to other Is methods, see Implement Number IsInt, IsUint, IsFloat, IsNan #155)
mark Number.Raw as deprecated and ask user to use AsFloat instead; in httpexpect v3, when we will be able to break compatibility, we'll undeprecate Raw, but change its return type to big.Float
adjust DefaultFormatter to handle big.Float values:
if value is integer, print it as integer
otherwise print it in non-scientific form (see also Add DefaultFormatter.EnableScientific #190)
adjust tests:
add unit tests for Number to check how it works with big integers and floats
add unit tests for Value.Number() to check how it works with json.Number and integers that can't be represented without loss as float64
rework tests for String.AsNumber() - rework float_precision test, add tests for big floats and integers that can't be parsed with default precision, but can be parsed when setting higher precision in Config
cover big integers and floats, and different precisions, in e2e tests (e.g. in e2e_basic_test.go)
The text was updated successfully, but these errors were encountered: