msgpack_ext: add string convertion for decimal datetime interval #491
Conversation
|
TestTarantoolBCDCompatibility is FAIL. |
|
return d.Decimal.String() - this method true, but slowly. I will correct the code and comments, at the moment I am looking for a solution to the problem. |
oleg-jukovec
left a comment
oleg-jukovec
left a comment
There was a problem hiding this comment.
Please, squash commits into a single relevant-one.
| return ptr.UnmarshalMsgpack(b) | ||
| } | ||
|
|
||
| // This method converts Datetime to String - formats to ISO8601. |
There was a problem hiding this comment.
The go-idiomatic way to comment a method.
| // This method converts Datetime to String - formats to ISO8601. | |
| // String converts Datetime to String - formats to ISO8601. |
| } | ||
|
|
||
| func TestDatetimeString(t *testing.T) { | ||
|
|
| exponent := d.Decimal.Exponent() | ||
|
|
||
| // If exponent is positive, then we use the standard method. | ||
| if exponent > 0 { | ||
| return d.Decimal.String() | ||
| } | ||
|
|
||
| scale := -exponent | ||
|
|
||
| if !coefficient.IsInt64() { | ||
| return d.Decimal.String() | ||
| } | ||
|
|
||
| int64Value := coefficient.Int64() | ||
|
|
||
| return d.stringFromInt64(int64Value, int(scale)) | ||
| } | ||
|
|
||
| // StringFromInt64 is an internal method for converting int64 | ||
| // and scale to a string (for numbers up to 19 digits) | ||
| func (d Decimal) stringFromInt64(value int64, scale int) string { | ||
|
|
||
| var buf [48]byte | ||
| pos := 0 | ||
|
|
||
| negative := value < 0 | ||
| if negative { | ||
| if value == math.MinInt64 { | ||
| return d.handleMinInt64(scale) | ||
| } | ||
| buf[pos] = '-' | ||
| pos++ | ||
| value = -value | ||
| } | ||
|
|
||
| str := strconv.FormatInt(value, 10) | ||
| length := len(str) | ||
|
|
||
| if scale == 0 { | ||
| if pos+length > len(buf) { | ||
| return d.Decimal.String() | ||
| } | ||
| copy(buf[pos:], str) | ||
| pos += length | ||
| return string(buf[:pos]) | ||
| } | ||
|
|
||
| if scale >= length { | ||
|
|
||
| required := 2 + (scale - length) + length | ||
| if pos+required > len(buf) { | ||
| return d.Decimal.String() | ||
| } | ||
|
|
||
| buf[pos] = '0' | ||
| buf[pos+1] = '.' | ||
| pos += 2 | ||
|
|
||
| zeros := scale - length | ||
| for i := 0; i < zeros; i++ { | ||
| buf[pos] = '0' | ||
| pos++ | ||
| } | ||
|
|
||
| copy(buf[pos:], str) | ||
| pos += length | ||
| } else { | ||
|
|
||
| integerLen := length - scale | ||
|
|
||
| required := integerLen + 1 + scale | ||
| if pos+required > len(buf) { | ||
| return d.Decimal.String() | ||
| } | ||
|
|
||
| copy(buf[pos:], str[:integerLen]) | ||
| pos += integerLen | ||
|
|
||
| buf[pos] = '.' | ||
| pos++ | ||
|
|
||
| copy(buf[pos:], str[integerLen:]) | ||
| pos += scale | ||
| } | ||
|
|
||
| return string(buf[:pos]) | ||
| } | ||
| func (d Decimal) handleMinInt64(scale int) string { | ||
| const minInt64Str = "9223372036854775808" | ||
|
|
||
| var buf [48]byte | ||
| pos := 0 | ||
|
|
||
| buf[pos] = '-' | ||
| pos++ | ||
|
|
||
| length := len(minInt64Str) | ||
|
|
||
| if scale == 0 { | ||
| if pos+length > len(buf) { | ||
| return "-" + minInt64Str // Fallback. | ||
| } | ||
| copy(buf[pos:], minInt64Str) | ||
| pos += length | ||
| return string(buf[:pos]) | ||
| } | ||
|
|
||
| if scale >= length { | ||
| required := 2 + (scale - length) + length | ||
| if pos+required > len(buf) { | ||
| // Fallback. | ||
| result := "0." + strings.Repeat("0", scale-length) + minInt64Str | ||
| return "-" + result | ||
| } | ||
|
|
||
| buf[pos] = '0' | ||
| buf[pos+1] = '.' | ||
| pos += 2 | ||
|
|
||
| zeros := scale - length | ||
| for i := 0; i < zeros; i++ { | ||
| buf[pos] = '0' | ||
| pos++ | ||
| } | ||
|
|
||
| copy(buf[pos:], minInt64Str) | ||
| pos += length | ||
| } else { | ||
| integerLen := length - scale | ||
| required := integerLen + 1 + scale | ||
| if pos+required > len(buf) { | ||
| return d.Decimal.String() // Fallback | ||
| } | ||
|
|
||
| copy(buf[pos:], minInt64Str[:integerLen]) | ||
| pos += integerLen | ||
|
|
||
| buf[pos] = '.' | ||
| pos++ | ||
|
|
||
| copy(buf[pos:], minInt64Str[integerLen:]) | ||
| pos += scale | ||
| } | ||
|
|
||
| return string(buf[:pos]) | ||
| } | ||
|
|
There was a problem hiding this comment.
This code works faster than if you call the method d.Decimal.String() . Is it okay to leave it?
There was a problem hiding this comment.
Please, rename it to String and add benchmarks to compare with shopspring.Decimal if so.
There was a problem hiding this comment.
In general, I found out what the problem was with launching benchmarks, I just had to launch it without Tarantool.
There was a problem hiding this comment.
According to the results, my conversion function is twice as good if we have a lot of examples of numbers int64. After int64, it is slower by 100 nanoseconds. Thus, we have a compromise: for small numbers (which fit into an int64) there is a gain, for large numbers there is a small loss.
But in real-world scenarios, small numbers are probably more common. It is also worth noting that the difference in performance for large numbers is not very big (866.4 ns/op vs. 788.5 ns/op),
while for small numbers, the gain was several times.
There was a problem hiding this comment.
I note that for numbers that do not fit into Int 64, the Decimal.String() function is also used, but not immediately, but after checking the number for the value, due to this, the stringOptimized function, which I will soon rename to just String(), works a little slower.
And there are extra allocations in the case of large numbers, d.Decimal.Efficient() - creates a copy of big.Int
d.Decimal.Exponent() is a simple field, but it's still a method call.
coefficient.IsInt64() - big size check.Int
For large numbers, these operations create additional allocations and take time, even when we know that a fallback is inevitable.
There was a problem hiding this comment.
Therefore, I will try to make a simpler check for the value of the number.
There was a problem hiding this comment.
TestRealTarantoolUsage is broken. We need to decide whether the String() method for Decimal should return insignificant zeros after . or not. In general, in Tarantula, it is expected that 100.00 will be like 100, but the BCDCompability test expects the opposite. BCDCompability is not broken because of the String() method.
babyTsakhes
force-pushed
the
babyTsakhes/gh-322-add-string-convertion-for-decimal-datetime-interval
branch
2 times, most recently
from
a66d851 to
679282c
Compare
|
FAIL: ExampleInterval_Add (0.00s) What behavior should this test demonstrate now? After I added String(), it now gets a string, and expects a structure. Should I change the expectation of this test or not? |
babyTsakhes
force-pushed
the
babyTsakhes/gh-322-add-string-convertion-for-decimal-datetime-interval
branch
3 times, most recently
from
200537c to
7bd96f5
Compare
|
go-tarantool/decimal/decimal_test.go Line 782 in 7bd96f5 go-tarantool/decimal/decimal_test.go Line 814 in 7bd96f5 |
|
This TestRealTarantoolUsage test fails because the string conversion code retains insignificant zeros after the dot. Keep insignificant zeros after the dot? The TestTarantoolBCDCompatibility test fails due to Unmarshal, and String() does not apply to operation. In principle, we have already discussed that it is possible to leave zeros after the dot. |
babyTsakhes
force-pushed
the
babyTsakhes/gh-322-add-string-convertion-for-decimal-datetime-interval
branch
from
7bd96f5 to
6688ae7
Compare
Added a benchmark, which shows that the code for decimal is optimized two or more times for string conversion than the code from the library. Added a datetime, Interval type conversion function to a string, added tests for this function. Added #322
babyTsakhes
force-pushed
the
babyTsakhes/gh-322-add-string-convertion-for-decimal-datetime-interval
branch
from
338c283 to
47fc723
Compare
|
Decimal now removes insignificant zeros after the decimal point, as it works in Tarantula. The tests are passing. The updated String function for decimal numbers is still 2 times faster than the library function. Please review my code. |
msgpack_ext: add string() for decimal
Added a decimal type conversion function to a string, added tests for this function. Fixed name of function. Add test for decimal convertion. I tried to write an optimized version, but there is a problem with values when working with BCD, insignificant zeros after the decimal point are lost.
Added #322