Skip to content

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

Merged
rohanprasad merged 6 commits intorelease/v21.03from
rohanprasad/fix_illegal_math_operations
Mar 31, 2021
Merged

fix(Query): [Breaking] Return error for illegal math operations.#7631
rohanprasad merged 6 commits intorelease/v21.03from
rohanprasad/fix_illegal_math_operations

Conversation

@rohanprasad
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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.

Comment thread query/math.go Outdated
Comment thread query/math.go Outdated
Comment thread query/math.go
@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
Comment thread query/aggregator.go
Comment thread query/math.go Outdated
Comment thread query/aggregator.go Outdated
Copy link
Copy Markdown
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.

Comment thread query/aggregator.go Outdated
@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