Added proper handling of int and float for math op #4257
Conversation
query/aggregator.go
Outdated
| a.Tid = types.FloatID | ||
| *res = *a | ||
| return nil | ||
| } else { |
There was a problem hiding this comment.
if block ends with a return statement, so drop this else and outdent its block (from golint)
harshil-goel
requested review from
manishrjain,
pawanrawal and
a team
as code owners
There was a problem hiding this comment.
Reviewable status: 0 of 8 files reviewed, 9 unresolved discussions (waiting on @harshil-goel, @mangalaman93, @manishrjain, and @pawanrawal)
gql/math.go, line 249 at r2 (raw file):
continue } // We will try to parse it as an Int first, if that fails we move to float
Merge these two comments.
gql/math.go, line 365 at r2 (raw file):
// Leaf node. if t.Const.Tid == types.FloatID { x.Check2(buf.WriteString(strconv.FormatFloat(t.Const.Value.(float64), 'E', -1, 64)))
You can assign in the if and then, call check2 just once.
query/aggregator.go, line 186 at r2 (raw file):
func ApplyFloatMod(a, b, c *types.Val) error { if b.Value.(float64) == 0 { return errors.Errorf("Division by zero")
Shouldn't this be module by zero error
query/aggregator.go, line 193 at r2 (raw file):
func ApplyOtherMod(a, b, c *types.Val) error { return errors.Errorf("Wrong type encountered for func %s", "%")
Errorf seems unnecessary
query/aggregator.go, line 264 at r2 (raw file):
func ApplyLnOther(a, res *types.Val) error { return errors.Errorf("Wrong type encountered for func ln")
These errors could be defined in one place for all functions and then reused.
query/aggregator.go, line 352 at r2 (raw file):
type MathBinaryFunction func(a, b, res *types.Val) error var supportedUnaryFunctions = map[string][]MathUnaryFunction{
can use a value type as array instead of slice
query/aggregator.go, line 352 at r2 (raw file):
type MathBinaryFunction func(a, b, res *types.Val) error var supportedUnaryFunctions = map[string][]MathUnaryFunction{
Could just call the variable unaryFuncs
query/aggregator.go, line 451 at r2 (raw file):
} if function, ok := supportedBinaryFunctions[ag.name]; ok {
Why are we checking the presence of function in the map here but not above?
There was a problem hiding this comment.
nice work
I have a comment about the structure of functions, please discuss with @mangalaman93 and see what he thinks about it.
Reviewable status: 0 of 8 files reviewed, 11 unresolved discussions (waiting on @harshil-goel, @mangalaman93, @manishrjain, and @pawanrawal)
query/aggregator.go, line 173 at r2 (raw file):
func ApplyOtherDiv(a, b, c *types.Val) error { return errors.Errorf("Wrong type encountered for func /")
Can you also please add the name of the type in the error message and the value to make it easier for the user to debug?
query/aggregator.go, line 362 at r2 (raw file):
} var supportedBinaryFunctions = map[string][]MathBinaryFunction{
Interesting choice of defining a global variable here and defining separate for functions for different types Int, Float, Other.
I would have considered collecting all functions related to an operation in one function, so ApplyIntAdd, ApplyFloatAdd and ApplyOtherAdd all in one function in a switch case based on the type, since it would have led to lesser functions and can help us get rid of global variables. Most of these functions are one liners anyway.
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 7 of 7 files at r2.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @harshil-goel)
query/aggregator.go, line 196 at r2 (raw file):
} func ApplyIntPow(a, b, c *types.Val) error {
Please make all of them start with small case.
query/aggregator.go, line 297 at r2 (raw file):
func ApplySqrtInt(a, res *types.Val) error { res.Value = math.Sqrt(float64(a.Value.(int64)))
ApplySqrt()
switch case {
float:
int:
default:
}

Then, your map can just be map of string to func, instead of a map of list. Pass the vbase to the func, which it uses to switch case.
query/aggregator.go, line 349 at r2 (raw file):
} type MathUnaryFunction func(a, res *types.Val) error
Internal as well?
query/aggregator.go, line 394 at r2 (raw file):
} func (ag *aggregator) MatchType(vBase, vaBase *valType, v, va *types.Val) error {
Can be an internal func.
query/aggregator.go, line 431 at r2 (raw file):
if isUnary(ag.name) { res.Tid = v.Tid err := supportedUnaryFunctions[ag.name][vBase](&v, &res)
What if ag.name is not present in the list. Should error out instead of panicking.
query/aggregator.go, line 447 at r2 (raw file):
vaBase := getValType(&va) if err := ag.MatchType(&vBase, &vaBase, &v, &va); err != nil {
vaBase can be derived internally. 4 args is kinda pushing it.
query/query4_test.go, line 26 at r2 (raw file):
) func TestBigMathValue(t *testing.T) {
I'd add a few more tests.
There was a problem hiding this comment.
Reviewed 1 of 7 files at r2, 7 of 7 files at r3.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @harshil-goel)
Fixes #4132
This change is