Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[typing] Kill Decimal.Value method #773

Merged
merged 1 commit into from
Jun 27, 2024
Merged

[typing] Kill Decimal.Value method #773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions clients/shared/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ func DefaultValue(column columns.Column, dialect sql.Dialect, additionalDateFmts
return nil, fmt.Errorf("colVal is not type *decimal.Decimal")
}

// TODO: Call [String] instead.
return val.Value(), nil
return val.String(), nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only place it was used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this function is only called when backfilling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tang8330 I feel like since we were sometimes emitting a string before, always emitting a string should be fine?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's fine, I just tested it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet thanks!

case typing.String.Kind:
return sql.QuoteLiteral(fmt.Sprint(column.DefaultValue())), nil
}
Expand Down
31 changes: 9 additions & 22 deletions lib/cdc/util/relational_event_decimal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package util
import (
"encoding/json"
"io"
"math/big"
"os"
"testing"

Expand Down Expand Up @@ -47,25 +46,21 @@ func TestSchemaEventPayload_Numeric_GetData(t *testing.T) {
retMap, err := schemaEventPayload.GetData(nil, &kafkalib.TopicConfig{})
assert.NoError(t, err)

assert.Equal(t, "123456.789", retMap["numeric_test"].(*decimal.Decimal).Value())
assert.Equal(t, 0, big.NewFloat(1234).Cmp(retMap["numeric_5"].(*decimal.Decimal).Value().(*big.Float)))
assert.Equal(t, "123456.789", retMap["numeric_test"].(*decimal.Decimal).String())
assert.Equal(t, "1234", retMap["numeric_5"].(*decimal.Decimal).String())
numericWithScaleMap := map[string]string{
"numeric_5_2": "568.01",
"numeric_5_6": "0.023456",
"numeric_5_0": "5",
}

for key, expectedValue := range numericWithScaleMap {
// Numeric data types that actually have scale fails when comparing *big.Float using `.Cmp`, so we are using STRING() instead.
_, isOk := retMap[key].(*decimal.Decimal).Value().(*big.Float)
assert.True(t, isOk)
// Now, we know the data type is *big.Float, let's check the .String() value.
assert.Equal(t, expectedValue, retMap[key].(*decimal.Decimal).String())
}

assert.Equal(t, "58569102859845154622791691858438258688", retMap["numeric_39_0"].(*decimal.Decimal).Value())
assert.Equal(t, "5856910285984515462279169185843825868.22", retMap["numeric_39_2"].(*decimal.Decimal).Value())
assert.Equal(t, "585691028598451546227958438258688.123456", retMap["numeric_39_6"].(*decimal.Decimal).Value())
assert.Equal(t, "58569102859845154622791691858438258688", retMap["numeric_39_0"].(*decimal.Decimal).String())
assert.Equal(t, "5856910285984515462279169185843825868.22", retMap["numeric_39_2"].(*decimal.Decimal).String())
assert.Equal(t, "585691028598451546227958438258688.123456", retMap["numeric_39_6"].(*decimal.Decimal).String())
}

func TestSchemaEventPayload_Decimal_GetData(t *testing.T) {
Expand All @@ -79,23 +74,19 @@ func TestSchemaEventPayload_Decimal_GetData(t *testing.T) {
assert.NoError(t, err)
retMap, err := schemaEventPayload.GetData(nil, &kafkalib.TopicConfig{})
assert.NoError(t, err)
assert.Equal(t, "123.45", retMap["decimal_test"].(*decimal.Decimal).Value())
assert.Equal(t, "123.45", retMap["decimal_test"].(*decimal.Decimal).String())
decimalWithScaleMap := map[string]string{
"decimal_test_5": "123",
"decimal_test_5_2": "123.45",
}

for key, expectedValue := range decimalWithScaleMap {
// Numeric data types that actually have scale fails when comparing *big.Float using `.Cmp`, so we are using STRING() instead.
_, isOk := retMap[key].(*decimal.Decimal).Value().(*big.Float)
assert.True(t, isOk)
// Now, we know the data type is *big.Float, let's check the .String() value.
assert.Equal(t, expectedValue, retMap[key].(*decimal.Decimal).String(), key)
}

assert.Equal(t, "58569102859845154622791691858438258688", retMap["decimal_test_39"].(*decimal.Decimal).Value(), "decimal_test_39")
assert.Equal(t, "585691028598451546227916918584382586.22", retMap["decimal_test_39_2"].(*decimal.Decimal).Value(), "decimal_test_39_2")
assert.Equal(t, "585691028598451546227916918584388.123456", retMap["decimal_test_39_6"].(*decimal.Decimal).Value(), "decimal_test_39_6")
assert.Equal(t, "58569102859845154622791691858438258688", retMap["decimal_test_39"].(*decimal.Decimal).String(), "decimal_test_39")
assert.Equal(t, "585691028598451546227916918584382586.22", retMap["decimal_test_39_2"].(*decimal.Decimal).String(), "decimal_test_39_2")
assert.Equal(t, "585691028598451546227916918584388.123456", retMap["decimal_test_39_6"].(*decimal.Decimal).String(), "decimal_test_39_6")
}

func TestSchemaEventPayload_Money_GetData(t *testing.T) {
Expand All @@ -115,10 +106,6 @@ func TestSchemaEventPayload_Money_GetData(t *testing.T) {
}

for key, expectedValue := range decimalWithScaleMap {
// Numeric data types that actually have scale fails when comparing *big.Float using `.Cmp`, so we are using STRING() instead.
_, isOk := retMap[key].(*decimal.Decimal).Value().(*big.Float)
assert.True(t, isOk)
// Now, we know the data type is *big.Float, let's check the .String() value.
assert.Equal(t, expectedValue, retMap[key].(*decimal.Decimal).String(), key)
}
}
23 changes: 1 addition & 22 deletions lib/debezium/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package debezium

import (
"math/big"
"testing"
"time"

Expand Down Expand Up @@ -450,7 +449,6 @@ func TestField_DecodeDecimal(t *testing.T) {
expectedPrecision int32
expectNilPtrPrecision bool
expectedScale int32
expectBigFloat bool
expectedErr string
}{
{
Expand Down Expand Up @@ -482,7 +480,6 @@ func TestField_DecodeDecimal(t *testing.T) {
expectedValue: "5",
expectedPrecision: 5,
expectedScale: 0,
expectBigFloat: true,
},
{
name: "NUMERIC(5,2)",
Expand All @@ -494,7 +491,6 @@ func TestField_DecodeDecimal(t *testing.T) {
expectedValue: "578.01",
expectedPrecision: 5,
expectedScale: 2,
expectBigFloat: true,
},
{
name: "NUMERIC(38, 0) - small #",
Expand All @@ -503,7 +499,6 @@ func TestField_DecodeDecimal(t *testing.T) {
"scale": "0",
"connect.decimal.precision": "38",
},
expectBigFloat: true,
expectedValue: "567",
expectedPrecision: 38,
expectedScale: 0,
Expand All @@ -515,7 +510,6 @@ func TestField_DecodeDecimal(t *testing.T) {
"scale": "0",
"connect.decimal.precision": "38",
},
expectBigFloat: true,
expectedValue: "99999999999999999999999999999999999999",
expectedPrecision: 38,
expectedScale: 0,
Expand All @@ -527,7 +521,6 @@ func TestField_DecodeDecimal(t *testing.T) {
"scale": "2",
"connect.decimal.precision": "38",
},
expectBigFloat: true,
expectedValue: "33.21",
expectedPrecision: 38,
expectedScale: 2,
Expand All @@ -539,7 +532,6 @@ func TestField_DecodeDecimal(t *testing.T) {
"scale": "2",
"connect.decimal.precision": "38",
},
expectBigFloat: true,
expectedValue: "9999999999999999999999999999999999.99",
expectedPrecision: 38,
expectedScale: 2,
Expand All @@ -551,7 +543,6 @@ func TestField_DecodeDecimal(t *testing.T) {
"scale": "4",
"connect.decimal.precision": "38",
},
expectBigFloat: true,
expectedValue: "484.4419",
expectedPrecision: 38,
expectedScale: 4,
Expand All @@ -563,7 +554,6 @@ func TestField_DecodeDecimal(t *testing.T) {
"scale": "4",
"connect.decimal.precision": "38",
},
expectBigFloat: true,
expectedValue: "999999999999999999999999999999.9999",
expectedPrecision: 38,
expectedScale: 4,
Expand Down Expand Up @@ -596,7 +586,6 @@ func TestField_DecodeDecimal(t *testing.T) {
params: map[string]any{
"scale": "2",
},
expectBigFloat: true,
expectedValue: "123456.98",
expectNilPtrPrecision: true,
expectedScale: 2,
Expand All @@ -618,9 +607,6 @@ func TestField_DecodeDecimal(t *testing.T) {
}

assert.NoError(t, err)
decVal := dec.Value()
_, isOk := decVal.(*big.Float)
assert.Equal(t, testCase.expectBigFloat, isOk, testCase.name)
assert.Equal(t, testCase.expectedValue, dec.String(), testCase.name)

if testCase.expectNilPtrPrecision {
Expand Down Expand Up @@ -713,16 +699,9 @@ func TestField_DecodeDebeziumVariableDecimal(t *testing.T) {
continue
}

// It should never be a *big.Float
_, isOk := dec.Value().(*big.Float)
assert.False(t, isOk, testCase.name)

// It should be a string instead.
_, isOk = dec.Value().(string)
assert.True(t, isOk, testCase.name)
assert.Equal(t, int32(-1), *dec.Precision(), testCase.name)
assert.Equal(t, testCase.expectedScale, dec.Scale(), testCase.name)
assert.Equal(t, testCase.expectedValue, dec.Value(), testCase.name)
assert.Equal(t, testCase.expectedValue, dec.String(), testCase.name)
}

}
19 changes: 0 additions & 19 deletions lib/typing/decimal/decimal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
package decimal

import (
"log/slog"
"math/big"

"github.com/artie-labs/transfer/lib/ptr"
"github.com/cockroachdb/apd/v3"
)
Expand Down Expand Up @@ -55,22 +52,6 @@ func (d *Decimal) String() string {
return d.value.Text('f')
}

func (d *Decimal) Value() any {
// -1 precision is used for variable scaled decimal
// We are opting to emit this as a STRING because the value is technically unbounded (can get to ~1 GB).
if d.precision != nil && (*d.precision > MaxPrecisionBeforeString || *d.precision == PrecisionNotSpecified) {
return d.String()
}

// Depending on the precision, we will want to convert value to STRING or keep as a FLOAT.
// TODO: [Value] is only called in one place, look into calling [String] instead.
if out, ok := new(big.Float).SetString(d.String()); ok {
return out
}
slog.Error("Failed to convert apd.Decimal to big.Float", slog.String("value", d.String()))
return d.String()
}

func (d *Decimal) Details() DecimalDetails {
precision := PrecisionNotSpecified
if d.precision != nil {
Expand Down