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] Clean up NewDecimalWithPrecision #778

Merged
merged 3 commits into from
Jun 27, 2024
Merged

[typing] Clean up NewDecimalWithPrecision #778

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65ed630
[typing] Clean up `NewDecimalWithPrecision`
nathan-artie Jun 27, 2024
91733cb
Merge branch 'master' into nv/clean-up-decimal
nathan-artie Jun 27, 2024
401517e
Kill precision and scale
nathan-artie Jun 27, 2024
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
21 changes: 3 additions & 18 deletions lib/typing/decimal/decimal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,30 +4,15 @@ import (
"github.com/cockroachdb/apd/v3"
)

const PrecisionNotSpecified int32 = -1

// Decimal is Artie's wrapper around [*apd.Decimal] which can store large numbers w/ no precision loss.
type Decimal struct {
precision int32
value *apd.Decimal
}

const (
DefaultScale int32 = 5
PrecisionNotSpecified int32 = -1
// MaxPrecisionBeforeString - if the precision is greater than 38, we'll cast it as a string.
// This is because Snowflake and BigQuery both do not have NUMERIC data types that go beyond 38.
MaxPrecisionBeforeString int32 = 38
)

func NewDecimalWithPrecision(value *apd.Decimal, precision int32) *Decimal {
scale := -value.Exponent
if scale > precision && precision != PrecisionNotSpecified {
// Note: -1 precision means it's not specified.

// This is typically not possible, but Postgres has a design flaw that allows you to do things like: NUMERIC(5, 6) which actually equates to NUMERIC(7, 6)
// We are setting precision to be scale + 1 to account for the leading zero for decimal numbers.
precision = scale + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we don't need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we have this exact logic in NewDetails() and we don't read from Decimal.Precision() except for tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. We should refactor the precision function then

func (d *Decimal) Precision() int32 {
	return NewDetails(d.precision, d.Scale()).Precision()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd just as soon as kill Precision, we don't need it.

}

return &Decimal{
precision: precision,
value: value,
Expand Down Expand Up @@ -58,5 +43,5 @@ func (d *Decimal) String() string {
}

func (d *Decimal) Details() Details {
return Details{scale: d.Scale(), precision: d.precision}
return NewDetails(d.precision, d.Scale())
}
7 changes: 7 additions & 0 deletions lib/typing/decimal/details.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@ import (
"fmt"
)

const (
DefaultScale int32 = 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved these constants too since they are only used for decimal details.

// MaxPrecisionBeforeString - if the precision is greater than 38, we'll cast it as a string.
// This is because Snowflake and BigQuery both do not have NUMERIC data types that go beyond 38.
MaxPrecisionBeforeString int32 = 38
)

type Details struct {
scale int32
precision int32
Expand Down