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.

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

fix(Query): [Breaking] Return error for illegal math operations. #7631

Merged
merged 6 commits into from
Mar 31, 2021

Conversation

rohanprasad
Copy link
Contributor

@rohanprasad rohanprasad commented Mar 22, 2021

If an operand is passed to math operations like log (ln, logbase), sqrt, pow which might result in illegal operation (resulting in NaN) we return 0 instead of an error. This PR fixes that.

Fixes DGRAPH-3150

Operation Data type Current behavior New behavior
Add Int64 Overflow/Underflow Return error
Sub Int64 Overflow/Underflow Return error
Multiply Int64 Overflow/Underflow Return error
Pow Float64 Fractional power of -ve number returns 0 Return error
Logbase Int64 -ve number returns 0 Return error
Logbase Int64 Log with base 1 returns large float Return error
Logbase Float64 -ve number return 0 Return error
Logbase Float64 Log with base 1 returns large float Return error
Ln Int64 -ve number returns 0 Return error
Ln Float64 -ve number returns 0 Return error
Negative Int64 -ve of MinInt64 remains same Return error
Sqrt Int64 -ve number return 0 Return error
Sqrt Float64 -ve number return 0 Return error

This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Mar 22, 2021

CLA assistant check
All committers have signed the CLA.

@rohanprasad rohanprasad force-pushed the rohanprasad/fix_illegal_math_operations branch from 09071e6 to d0711a7 Compare March 22, 2021 10:46
Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

Looks good to me. Defer to @ajeetdsouza for the final approval.

Copy link
Contributor

@ajeetdsouza ajeetdsouza left a comment

Choose a reason for hiding this comment

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

Create a table with the old and new behaviours of all the math operations we've changed, and notify the docs team.

query/math.go Outdated Show resolved Hide resolved
query/math.go Outdated Show resolved Hide resolved
query/math.go Show resolved Hide resolved
@rohanprasad rohanprasad requested a review from ajeetdsouza March 26, 2021 10:53
@rohanprasad rohanprasad changed the title fix(Query): Return error for illegal math operations. fix(Query): [Breaking] Return error for illegal math operations. Mar 26, 2021
query/aggregator.go Show resolved Hide resolved
query/math.go Outdated Show resolved Hide resolved
query/aggregator.go Outdated Show resolved Hide resolved
Copy link
Contributor

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

query/aggregator.go Outdated Show resolved Hide resolved
@rohanprasad rohanprasad merged commit 0f59807 into release/v21.03 Mar 31, 2021
aman-bansal pushed a commit that referenced this pull request Apr 8, 2021
* Return error if operand for math functions are invalid
@joshua-goldstein joshua-goldstein deleted the rohanprasad/fix_illegal_math_operations branch August 11, 2022 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants