Addition expression implementation #5072
Conversation
… error Changed tests TestAdd and TestCastFromNumeric_in arithmetic_test.go to return value output instead of error Signed-off-by: Rasika Kale <rasika@planetscale.com>
Signed-off-by: Rasika Kale <rasika@planetscale.com>
…lar to mySQL - Added possible tests for Addition function within arithmetic_test.go Signed-off-by: Rasika Kale <rasika@planetscale.com>
- In the process of implenting subtraction and multiplication function (marked within comments) Signed-off-by: Rasika Kale <rasika@planetscale.com>
rasikakale
changed the title
|
Need to fix the build. |
morgo
left a comment
morgo
left a comment
There was a problem hiding this comment.
Some initial feedback. I mainly came to look at the testcases, which make sense to me (but some are commented out). Please fix/remove commented out code and I'll take another look :-)
- Deleted unnecessary comments - Fixed whitespace Signed-off-by: Rasika Kale <rasika@planetscale.com>
- Made rk-add-expression only for addition function Signed-off-by: Rasika Kale <rasika@planetscale.com>
…e, rather than value or error Signed-off-by: Rasika Kale <rasika@planetscale.com>
sougou
left a comment
sougou
left a comment
There was a problem hiding this comment.
Nice work. I've nitpicked a few things.
Signed-off-by: Rasika Kale <rasika@planetscale.com>
- Fixed TestOrderedAggregateMergeFail in ordered_aggregate_test.go to return proper value for expressions such as "b + 1" Signed-off-by: Rasika Kale <rasika@planetscale.com>
… avoid runtime errors Signed-off-by: Rasika Kale <rasika@planetscale.com>
Signed-off-by: Rasika Kale <rasika@planetscale.com>
Signed-off-by: Rasika Kale <rasika@planetscale.com>
|
So, if I understand this correctly, this change set adds code for a few binary expressions to be used in the engine, but does not change the planner to actually produce these expressions, right? or am I missing something? |
Signed-off-by: Rasika Kale <rasika@planetscale.com>
rasikakale
changed the title
Signed-off-by: Rasika Kale <rasika@planetscale.com>
Signed-off-by: Rasika Kale <rasika@planetscale.com>
sougou
left a comment
sougou
left a comment
There was a problem hiding this comment.
Looking much better now. Few more comments.
Since you've added Subtract and Multiply to this PR, you'll need to write tests for those before we can merge this.
Also, we need to ensure 100% code coverage. I still see some code paths not being covered.
- Deleted Subtract() and Multiply() and other functions not related to Add() Signed-off-by: Rasika Kale <rasika@planetscale.com>
| return numeric{}, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "cannot add a negative number to an unsigned integer: %d, %d", v1, v2) | ||
| func intPlusIntWithError(v1, v2 int64) (numeric, error) { | ||
| result := v1 + v2 | ||
| if (result > v1) != (v2 > 0) { |
There was a problem hiding this comment.
as a general practice, we should prefer clarify over compactness. A clearer way to write this if would have been:
if (v1 > 0 && v2 > 0 && result < 0) || (v1 < 0 && v2 < 0 && result > 0)
5 participants
Changed
NullSafeAdd()to only return a value, instead of returning a value or an error. To achieve, the following functions were changed:addNumeric()- returns only a numeric, instead of returning a numeric or errorcastFromNumeric()- returns only a Value, instead of returning a Value or an erroruintPlusInt()- returns only a numeric, instead of returning a numeric or an errorImplemented
Addition()to return either Value or error. The output in this function presents the same output when two values are inputted into MySQL. The following functions were added to achieve this:addNumericWithError()- operates the same way asaddNumeric()except it returns a numeric or an errorintPlusIntWithError()- operates the same way asintPlusInt()except it returns a numeric or an error. A BIGINT error is returned if the sum returns a floatuintPlusIntWithError()- returns a numeric or an error.uintPlusUintWithError()- operates the same way asuintPlusUint(), except it returns a numeric or a BIGINT UNSIGNED error