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

Adds SQL function HYPOT using the GPU #4592

Merged
merged 7 commits into from
Jan 24, 2022

Conversation

NVnavkumar
Copy link
Collaborator

Implements #33.

This implements the SQL HYPOT function in class GpuHypot, and substitutes the original definition:

HYPOT(lhs, rhs) = sqrt(lhs^2 + rhs^2)

with this version:

HYPOT(lhs, rhs) = r
WHERE:
r = x * sqrt(1 + (y / x)^2)
x = max(abs(lhs), abs(rhs)), y = min(abs(lhs), abs(rhs))

This is done to avoid overflow/underflow situations like the Spark implementation, which relies on java.lang.Math.hypot(double, double). It also implements the following corner cases that are defined in the java version documentation:

  • If either argument is infinite, then the result is positive infinity.
  • If either argument is NaN and neither argument is infinite, then the result is NaN.

Also it implements another special case:

  • If both arguments are 0.0, then the result is 0.0.

Signed-off-by: Navin Kumar <navink@nvidia.com>
squares

Signed-off-by: Navin Kumar <navink@nvidia.com>
specific edge cases referred by Java documentation

Signed-off-by: Navin Kumar <navink@nvidia.com>
maxNullAware

Signed-off-by: Navin Kumar <navink@nvidia.com>
@NVnavkumar NVnavkumar changed the title Adds SQL function HYPOT using the GPU [WIP] Adds SQL function HYPOT using the GPU Jan 21, 2022
@NVnavkumar NVnavkumar marked this pull request as draft January 21, 2022 01:16
@NVnavkumar NVnavkumar linked an issue Jan 21, 2022 that may be closed by this pull request
@sameerz sameerz added the feature request New feature or request label Jan 21, 2022
}

override def doColumnar(lhs: GpuScalar, rhs: GpuColumnVector): ColumnVector = {
doColumnar(GpuColumnVector.from(lhs, rhs.getRowCount.toInt, left.dataType), rhs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to make sure that the new ColumnVector being created is closed too.

@revans2
Copy link
Collaborator

revans2 commented Jan 21, 2022

Also I just want to be sure we don't forget to move this to be based off of the 22.04 branch instead of the 22.02 branch like it is now.

@NVnavkumar NVnavkumar changed the base branch from branch-22.02 to branch-22.04 January 21, 2022 17:37
@sameerz sameerz added this to the Jan 10 - Jan 28 milestone Jan 21, 2022
Signed-off-by: Navin Kumar <navink@nvidia.com>
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Just some nits to improve the worst case memory usage. Looking really good

}

def computeHypot(x: ColumnVector, y: ColumnVector): ColumnVector = {
withResource(y.div(x)) { yOverX =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: there are a lot of intermediate values that live longer than they need to. Could we do it a bit more like.

    val yOverXSquared = withResource(y.div(x)) { yOverX =>
        yOverX.mul(yOverX)
    }
    val onePlusTerm = withResource(yOverXSquared) { yOverXSquared =>
      withResource(Scalar.fromDouble(1)) { one =>
        one.add(yOverXSquared)
      }
    }
    val onePlusTermSqrt = withResource(onePlusTerm) { onePlusTerm =>
      onePlusTerm.sqrt
    }
    withResource(onePlusTermSqrt) { onePlusTermSqrt =>
      x.mul(onePlusTermSqrt)
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaned up in latest commit

@NVnavkumar
Copy link
Collaborator Author

build

…d cleaner code

Signed-off-by: Navin Kumar <navink@nvidia.com>
@NVnavkumar NVnavkumar marked this pull request as ready for review January 24, 2022 19:03
@NVnavkumar NVnavkumar changed the title [WIP] Adds SQL function HYPOT using the GPU Adds SQL function HYPOT using the GPU Jan 24, 2022
@NVnavkumar NVnavkumar merged commit 7506ae5 into NVIDIA:branch-22.04 Jan 24, 2022
@NVnavkumar NVnavkumar deleted the add-sql-function-hypot branch January 24, 2022 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] hypot SQL function
3 participants