Repository: amzn/ion-go
Pull request #51 (2020-06-04 01:58:46)

FIX Decimal 'D' format scale 0 being omitted in binary
Issue 36:
Description of Changes:

Decimal values being represented in the 'D' format that have a scale of 0
were being omitted in the binary slice representation. This matches the
behaviour in the other Ion libraries. The issue was being caused because
with the omission of the scale 0, the dot binary representation in the slice was
being mistaken as the scale value.




codecov-commenter: # [Codecov](https://codecov.io/gh/amzn/ion-go/pull/51?src=pr&el=h1) Report
> Merging [#51](https://codecov.io/gh/amzn/ion-go/pull/51?src=pr&el=desc) into [master](https://codecov.io/gh/amzn/ion-go/commit/c6d91a2c33946dec146c4946c614503a9f411482&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/amzn/ion-go/pull/51/graphs/tree.svg?width=650&height=150&src=pr&token=OBDT1TNoNE)](https://codecov.io/gh/amzn/ion-go/pull/51?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   75.83%   75.82%   -0.02%     
==========================================
  Files          22       22              
  Lines        4573     4570       -3     
==========================================
- Hits         3468     3465       -3     
  Misses        681      681              
  Partials      424      424              
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-go/pull/51?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [ion/binarywriter.go](https://codecov.io/gh/amzn/ion-go/pull/51/diff?src=pr&el=tree#diff-aW9uL2JpbmFyeXdyaXRlci5nbw==) | `77.66% <100.00%> (-0.23%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-go/pull/51?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-go/pull/51?src=pr&el=footer). Last update [c6d91a2...e6597af](https://codecov.io/gh/amzn/ion-go/pull/51?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

matthewh-BQ: @fernomac : If possible, could we please get some historical context to the intention with the conditional check for ensuring that exp != 0?

We've seen some issues with the round trip testing specifically with decimals following the 'D' format with a scale of 0. e.g., 123456d0

When the decimal is being written, the buffer does not factor it in.

The unit test was changed to match what we think ought to be happening.
fernomac: Ah, I think this was a misread of the spec on my part:

“ Decimal representations have two components: exponent (a VarInt) and coefficient (an Int). The decimal’s value is coefficient * 10 ^ exponent.

The length of the coefficient subfield is the total length of the representation minus the length of exponent. The subfield should not be present (that is, it has zero length) when the coefficient’s value is (positive) zero.

If the value is 0. (aka 0d0) then L is zero, there are no length or representation fields, and the entire value is encoded as the single byte 0x50.”

So a zero exponent should be omitted if and only if the coefficient is also zero?
matthewh-BQ: > Ah, I think this was a misread of the spec on my part:
> 
> “ Decimal representations have two components: exponent (a VarInt) and coefficient (an Int). The decimal’s value is coefficient * 10 ^ exponent.
> 
> The length of the coefficient subfield is the total length of the representation minus the length of exponent. The subfield should not be present (that is, it has zero length) when the coefficient’s value is (positive) zero.
> 
> If the value is 0. (aka 0d0) then L is zero, there are no length or representation fields, and the entire value is encoded as the single byte 0x50.”
> 
> So a zero exponent should be omitted if and only if the coefficient is also zero?

Yes, that makes sense. I've added a conditional for this special case.

Patch:
diff --git a/ion/binarywriter.go b/ion/binarywriter.go
index 0115faf..5c61358 100644
--- a/ion/binarywriter.go
+++ b/ion/binarywriter.go
@@ -172,21 +172,24 @@ func (w *binaryWriter) WriteFloat(val float64) error {
 func (w *binaryWriter) WriteDecimal(val *Decimal) error {
 	coef, exp := val.CoEx()
 
-	vlen := uint64(0)
-	if exp != 0 {
-		vlen += varIntLen(int64(exp))
-	}
-	if coef.Sign() != 0 {
-		vlen += bigIntLen(coef)
+	// If the value is 0. (aka 0d0) then L is zero, there are no length or
+	// representation fields, and the entire value is encoded as the single byte 0x50.
+	if coef.Sign() == 0 && int64(exp) == 0 {
+		buf := make([]byte, 0, 0)
+		buf = appendTag(buf, 0x50, 0)
+
+		return w.writeValue("Writer.WriteDecimal", buf)
 	}
 
+	// Otherwise, length or representation fields are present and must be considered.
+	vlen := varIntLen(int64(exp))
+	vlen += bigIntLen(coef)
+
 	buflen := vlen + tagLen(vlen)
 	buf := make([]byte, 0, buflen)
 
 	buf = appendTag(buf, 0x50, vlen)
-	if exp != 0 {
-		buf = appendVarInt(buf, int64(exp))
-	}
+	buf = appendVarInt(buf, int64(exp))
 	buf = appendBigInt(buf, coef)
 
 	return w.writeValue("Writer.WriteDecimal", buf)


Test patch:
diff --git a/ion/binarywriter_test.go b/ion/binarywriter_test.go
index c1cbaaa..c36c293 100644
--- a/ion/binarywriter_test.go
+++ b/ion/binarywriter_test.go
@@ -176,6 +176,7 @@ func TestWriteBinaryTimestamp(t *testing.T) {
 func TestWriteBinaryDecimal(t *testing.T) {
 	eval := []byte{
 		0x50,       // 0.
+		0x51, 0xC1, // 0.0, aka 0 x 10^-1
 		0x51, 0xC3, // 0.000, aka 0 x 10^-3
 		0x53, 0xC3, 0x03, 0xE8, // 1.000, aka 1000 x 10^-3
 		0x53, 0xC3, 0x83, 0xE8, // -1.000, aka -1000 x 10^-3
@@ -185,6 +186,7 @@ func TestWriteBinaryDecimal(t *testing.T) {
 
 	testBinaryWriter(t, eval, func(w Writer) {
 		w.WriteDecimal(MustParseDecimal("0."))
+		w.WriteDecimal(MustParseDecimal("0.0"))
 		w.WriteDecimal(MustParseDecimal("0.000"))
 		w.WriteDecimal(MustParseDecimal("1.000"))
 		w.WriteDecimal(MustParseDecimal("-1.000"))
diff --git a/ion/integration_test.go b/ion/integration_test.go
index acc2542..607cc91 100644
--- a/ion/integration_test.go
+++ b/ion/integration_test.go
@@ -56,9 +56,6 @@ var binaryRoundTripSkipList = []string{
 	"clobNewlines.ion",
 	"clobWithNonAsciiCharacter.10n",
 	"clobs.ion",
-	"clobs.ion",
-	"decimal64BitBoundary.ion",
-	"decimals.ion",
 	"float32.10n",
 	"floatSpecials.ion",
 	"floats.ion",
@@ -104,11 +101,6 @@ var textRoundTripSkipList = []string{
 	"annotations.ion",
 	"clobNewlines.ion",
 	"clobs.ion",
-	"clobs.ion",
-	"decimal64BitBoundary.ion",
-	"decimal_values.ion",
-	"decimals.ion",
-	"decimalsWithUnderscores.ion",
 	"float32.10n",
 	"floatSpecials.ion",
 	"floats.ion",