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

GpuAdd supports ANSI mode. #3537

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Conversation

firestarman
Copy link
Collaborator

@firestarman firestarman commented Sep 18, 2021

And let Multiply fall back to CPU when ANSI mode is enabled.

This closes #3472

Signed-off-by: Firestarman [email protected]

And let GPU Multiply fall back to CPU when ansi mode is enabled.

Signed-off-by: Firestarman <[email protected]>
@firestarman
Copy link
Collaborator Author

build

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 a few nits that can be handled by follow on issues.

ShortGen(min_val = 1, max_val = 100, special_cases=[]),
IntegerGen(min_val = 1, max_val = 1000, special_cases=[]),
LongGen(min_val = 1, max_val = 3000, special_cases=[]),
float_gen, double_gen,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think we can have a follow on issue to re-enable float, double, and decimal processing in ANSI mode. Each of them handle overflows already in different ways that should be okay.

@@ -1380,7 +1380,7 @@ object GpuOverrides extends Logging {
ExprChecks.mathUnary,
(a, conf, p, r) => new UnaryExprMeta[Log1p](a, conf, p, r) {
override def convertToGpu(child: Expression): GpuExpression =
GpuLog(GpuAdd(child, GpuLiteral(1d, DataTypes.DoubleType)))
GpuLog(GpuAdd(child, GpuLiteral(1d, DataTypes.DoubleType), SQLConf.get.ansiEnabled))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: This is a little misleading, because there is no fail on error overflow processing on doubles, and doubles are the only thing supported by Log1p


override def tagSelfForAst(): Unit = {
super.tagSelfForAst()
if (ansiEnabled) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: It would be good to check the data type, because double and float don't have special overflow processing and should be able to work in AST mode.

Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Small capitalization nits but otherwise lgtm.

override def tagSelfForAst(): Unit = {
super.tagSelfForAst()
if (ansiEnabled) {
willNotWorkInAst("AST Addition does not support ansi mode.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
willNotWorkInAst("AST Addition does not support ansi mode.")
willNotWorkInAst("AST Addition does not support ANSI mode.")

@@ -62,6 +63,26 @@ def test_multiplication(data_gen):
f.col('a') * f.col('b')),
conf=allow_negative_scale_of_decimal_conf)

# No overflow gens here because we just focus on verifying the fallback to CPU when
# enabling ansi mode. But overflows will fail the tests because CPU runs raise
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# enabling ansi mode. But overflows will fail the tests because CPU runs raise
# enabling ANSI mode. But overflows will fail the tests because CPU runs raise

@@ -1709,6 +1718,10 @@ object GpuOverrides extends Logging {
}
case _ => // NOOP
}

if (SQLConf.get.ansiEnabled) {
willNotWorkOnGpu("GPU Multiplication does not support ansi mode")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
willNotWorkOnGpu("GPU Multiplication does not support ansi mode")
willNotWorkOnGpu("GPU Multiplication does not support ANSI mode")

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll do this in a follow on issue as @firestarman is now out on vacation.

@revans2 revans2 merged commit d1800dd into NVIDIA:branch-21.10 Sep 20, 2021
@revans2 revans2 mentioned this pull request Sep 20, 2021
@firestarman firestarman deleted the ansi-add-mul branch September 22, 2021 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] GpuAdd and GpuMultiply do not include failOnError
4 participants