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.

Sign up for GitHub

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

Fixes creation of invalid DecimalType in GpuDivide.tagExprForGpu #1991

Merged
merged 3 commits into from
Mar 23, 2021
Merged

Fixes creation of invalid DecimalType in GpuDivide.tagExprForGpu #1991

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
770131e
fixes #1984
razajafri Mar 22, 2021
73b5120
added unit tests
razajafri Mar 23, 2021
3901e6a
make sure the exception is not AnalysisException
razajafri Mar 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion integration_tests/src/main/python/arithmetic_ops_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ def test_multiplication_mixed(lhs, rhs):
f.col('a') * f.col('b')),
conf=allow_negative_scale_of_decimal_conf)

@pytest.mark.parametrize('data_gen', [double_gen, decimal_gen_neg_scale, DecimalGen(6, 3), DecimalGen(5, 5), DecimalGen(6, 0)], ids=idfn)
@pytest.mark.parametrize('data_gen', [double_gen, decimal_gen_neg_scale, DecimalGen(6, 3),
DecimalGen(5, 5), DecimalGen(6, 0),
pytest.param(DecimalGen(38, 21), marks=pytest.mark.xfail(reason="The precision is too large to be supported on the GPU")),
pytest.param(DecimalGen(21, 17), marks=pytest.mark.xfail(reason="The precision is too large to be supported on the GPU"))], ids=idfn)
Copy link
Collaborator

@revans2 revans2 Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If I remove your patch and keep the test there will be no real difference in the results, unless someone goes through the logs manually and looks that in one case we failed by falling back to the CPU, and in another case we failed by throwing an exception about going over the limit. In both cases the xfail ignored the exception that was thrown.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have updated the test to check if the test raises IllegalArgumentException. Let me know if you want me to tighten it down even more. Ideally we should be checking the message to make sure its failing because its not columnar but I am not sure how to accomplish that using the pytest xfail

def test_division(data_gen):
data_type = data_gen.data_type
assert_gpu_and_cpu_are_equal_collect(
Expand Down
50 changes: 30 additions & 20 deletions sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1714,29 +1714,39 @@ object GpuOverrides {
(childExprs.head.dataType, childExprs(1).dataType) match {
case (l: DecimalType, r: DecimalType) =>
val outputType = GpuDivideUtil.decimalDataType(l, r)
// We will never hit a case where outputType.precision < outputType.scale + r.scale.
// So there is no need to protect against that.
// The only two cases in which there is a possibility of the intermediary scale
// exceeding the intermediary precision is when l.precision < l.scale or l
// .precision < 0, both of which aren't possible.
// Proof:
// case 1:
// outputType.precision = p1 - s1 + s2 + s1 + p2 + 1 + 1
// outputType.scale = p1 + s2 + p2 + 1 + 1
// To find out if outputType.precision < outputType.scale simplifies to p1 < s1,
// which is never possible
//
// case 2:
// outputType.precision = p1 - s1 + s2 + 6 + 1
// outputType.scale = 6 + 1
// To find out if outputType.precision < outputType.scale simplifies to p1 < 0
// which is never possible
// Case 1: OutputType.precision doesn't get truncated
// We will never hit a case where outputType.precision < outputType.scale + r.scale.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good (and fairly easy) to add a unit test for this. I had a go and found some values that do hit this case but it is possible that I am using values that couldn't happen in real life. I am not sure.

Here is one example:

l = DecimalType(3,0)
r = DecimalType(21,17)
outputType = DecimalType(38,23)
38 < 23 + 17

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps I am hitting case 2 here where precision is getting truncated?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does look like I am hitting case 2 here so I think the logic in the comments is sound, but a unit test would make me more confident and would protect against future regressions if any of this code gets updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for the unit test to verify that this is working. The logic looks good to me. I am a little concerned about the coupling between this code and the code in GpuDivideUtil, but that is minor for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I wrote a unit test locally to verify this. I don't know why I didn't check it in. Let me do that

Copy link
Collaborator Author

@razajafri razajafri Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

l = DecimalType(3,0)
r = DecimalType(21,17)
outputType = DecimalType(38,23)
38 < 23 + 17

So, like you said this won't happen in reality as by the time we get the call both l and r Spark will be the same precision and scale, I think it will upcast the Decimal(3,0) to Decimal(21,17).

// So there is no need to protect against that.
// The only two cases in which there is a possibility of the intermediary scale
// exceeding the intermediary precision is when l.precision < l.scale or l
// .precision < 0, both of which aren't possible.
// Proof:
// case 1:
// outputType.precision = p1 - s1 + s2 + s1 + p2 + 1 + 1
// outputType.scale = p1 + s2 + p2 + 1 + 1
// To find out if outputType.precision < outputType.scale simplifies to p1 < s1,
// which is never possible
//
// case 2:
// outputType.precision = p1 - s1 + s2 + 6 + 1
// outputType.scale = 6 + 1
// To find out if outputType.precision < outputType.scale simplifies to p1 < 0
// which is never possible
// Case 2: OutputType.precision gets truncated to 38
// In this case we have to make sure the r.precision + l.scale + r.scale + 1 <= 38
// Otherwise the intermediate result will overflow
// TODO We should revisit the proof one more time after we support 128-bit decimals
val intermediateResult = DecimalType(outputType.precision, outputType.scale + r.scale)
if (intermediateResult.precision > DType.DECIMAL64_MAX_PRECISION) {
willNotWorkOnGpu("The actual output precision of the divide is too large" +
if (l.precision + l.scale + r.scale + 1 > 38) {
willNotWorkOnGpu("The intermediate output precision of the divide is too " +
s"large to be supported on the GPU i.e. Decimal(${outputType.precision}, " +
s"${outputType.scale + r.scale})")
} else {
val intermediateResult =
DecimalType(outputType.precision, outputType.scale + r.scale)
if (intermediateResult.precision > DType.DECIMAL64_MAX_PRECISION) {
willNotWorkOnGpu("The actual output precision of the divide is too large" +
s" to fit on the GPU $intermediateResult")
}
}
case _ => // NOOP
}
Expand Down