-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Feat/apd v3 upgrade #4
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe changes primarily involve the removal of trigonometric and mathematical function definitions from Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 4735815 and 74443d8c61254643e6af58c5d3a8d798ff74e0db.
Files ignored due to path filters (2)
go.mod
is excluded by!**/*.mod
go.sum
is excluded by!**/*.sum
,!**/*.sum
Files selected for processing (5)
- engine/atom.go (4 hunks)
- engine/float.go (1 hunks)
- engine/number.go (1 hunks)
- engine/number_test.go (1 hunks)
- interpreter_test.go (1 hunks)
Additional comments not posted (17)
engine/float.go (2)
5-5
: LGTM! Updated import path.The import path for the
apd
library has been correctly updated tov3
.
10-16
: LGTM! Improved documentation.The updated documentation for the
Float
struct provides better clarity on its purpose, usage, and precision.engine/atom.go (1)
Line range hint
1-1
:
Verify the removal of trigonometric and mathematical atoms.The atoms related to trigonometric functions (
acos
,asin
,atan
,atan2
,cos
,sin
,tan
) and mathematical operations (round
,sign
,e
) have been removed. Ensure that this removal is intentional and check if there are any replacements or alternatives provided.engine/number.go (1)
5-5
: LGTM! Updated import path.The import path for the
apd
library has been correctly updated tov3
.engine/number_test.go (12)
Line range hint
11-19
:
LGTM!The function
newFloatFromFloat64Must
correctly converts a float64 to aFloat
type using theapd.Decimal
type.
Line range hint
21-28
:
LGTM!The function
newFloatFromStringMust
correctly converts a string to aFloat
type using theNewFloatFromString
function.
Line range hint
30-38
:
LGTM!The function
mulMust
correctly multiplies twoFloat
types using themulF
function.
Line range hint
40-309
:
LGTM!The function
TestIs
contains comprehensive test cases for various mathematical operations and edge cases.
Line range hint
311-351
:
LGTM!The function
TestEqual
contains comprehensive test cases for various equality checks.
Line range hint
353-380
:
LGTM!The function
TestNotEqual
contains comprehensive test cases for various inequality checks.
Line range hint
382-408
:
LGTM!The function
TestLessThan
contains comprehensive test cases for various less than comparisons.
Line range hint
410-436
:
LGTM!The function
TestGreaterThan
contains comprehensive test cases for various greater than comparisons.
Line range hint
438-464
:
LGTM!The function
TestLessThanOrEqual
contains comprehensive test cases for various less than or equal to comparisons.
Line range hint
466-492
:
LGTM!The function
TestGreaterThanOrEqual
contains comprehensive test cases for various greater than or equal to comparisons.
Line range hint
494-520
:
LGTM!The
mockNumber
struct and its methods are correctly implemented for mocking number operations in tests.
5-5
: Verify compatibility with the newapd
version.The import path has been updated to
github.com/cockroachdb/apd/v3
. Ensure that the new version is compatible with the existing code and does not introduce any breaking changes.interpreter_test.go (1)
609-609
: Verify the precision of the modified output value.The output value in test case "172" has been changed from
1.0e-323
to1.000000000000000000000000000000000e-323
. Ensure that this change accurately reflects the expected precision and behavior of theapd
library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm.
Just a point, commit f28ae49b0d1253800774f80d0f0db4d1b9d3ff60 has a typo.
74443d8
to
f8e2298
Compare
Ship the latest
v3.2.1
version of https://github.com/cockroachdb/apd used in float decimal implementation.Also removes unused atoms & improve float documentation.