-
Notifications
You must be signed in to change notification settings - Fork 240
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
[WIP] decimal meta adjustment for GPU runtime #976
Conversation
Can one of the admins verify this patch? |
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.
Until we support casting to decimal I don't see any value of adding this in.
@@ -532,6 +532,20 @@ object GpuOverrides { | |||
// There are so many of these that we don't need to print them out. | |||
override def print(append: StringBuilder, depth: Int, all: Boolean): Unit = {} | |||
}), | |||
// TODO: maybe we should apply the conversion between decimal32 and decimal64 to promote precision? | |||
expr[PromotePrecision]( |
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.
This is the same as KnownFloatingPointNormalized
or KnownNotNull
. In those cases we don't throw it away, we replace it with s GPU version that is essentially a noop. It has little performance impact and lets us keep the same structure that spark has.
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.
Yes! And this PR has been refactored.
override def convertToGpu(child: Expression): GpuExpression = { | ||
child match { | ||
case c: GpuCast => c.child.asInstanceOf[GpuExpression] | ||
case c => throw new IllegalStateException( |
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.
Is this guaranteed 100% of the time that the child will be a cast? If not then we need to figure out better way to handle this?
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.
So far, it is 100% guaranteed since case class PrecisionPromotion
will only be created inside private method DecimalPrecision.promotePrecision
with Cast
Op.
Yes. Because initial work for decimal is W.I.P. in terms of cuDF java library, I am trying to work on something which is independent on |
I am fine with you wanting to work ahead. I just wasn't aware that this patch is still a work in progress. Sorry for the confusion. |
Signed-off-by: sperlingxx <[email protected]>
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.
This should be retargeted to branch-0.4.
…IDIA#976) Signed-off-by: spark-rapids automation <[email protected]>
Catalyst analyzer will apply
promotePrecision
andcheckOverflow
on binary arithmetic expressions if data in both sides of expressions are decimal types (refer to link).Considering GPU_MAX_PRECISION of decimal type can only be up to 19 (half of MAX_PRECISION in spark), we need to reapply precision and scale adjustment with GPU constraints. To achieve this, we introduce two new expression-rules
PromotePrecisionExprMeta
andCheckOverflowExprMeta
into GpuOverrides.In order to keep align with decimal overflow controlling behavior of spark, we follow the same strategy as catalyst with GPU constraints replacement. But in spark catalyst runtime, overflow decimal value will be replaced by null. (Otherwise, overflow exception will be thrown.) In cuDF runtime, overflow check for decimal operations is still missing.