-
Notifications
You must be signed in to change notification settings - Fork 244
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
Adds SQL function HYPOT using the GPU #4592
Conversation
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>
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/mathExpressions.scala
Outdated
Show resolved
Hide resolved
} | ||
|
||
override def doColumnar(lhs: GpuScalar, rhs: GpuColumnVector): ColumnVector = { | ||
doColumnar(GpuColumnVector.from(lhs, rhs.getRowCount.toInt, left.dataType), rhs) |
There was a problem hiding this comment.
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.
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/mathExpressions.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/mathExpressions.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/mathExpressions.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/mathExpressions.scala
Outdated
Show resolved
Hide resolved
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. |
Signed-off-by: Navin Kumar <navink@nvidia.com>
There was a problem hiding this 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 => |
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
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
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/mathExpressions.scala
Outdated
Show resolved
Hide resolved
build |
…d cleaner code Signed-off-by: Navin Kumar <navink@nvidia.com>
Implements #33.
This implements the SQL HYPOT function in class GpuHypot, and substitutes the original definition:
with this version:
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:NaN
and neither argument is infinite, then the result isNaN
.Also it implements another special case:
0.0
, then the result is0.0
.