-
Notifications
You must be signed in to change notification settings - Fork 232
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
Fix crash when casting decimals to long #6103
Changes from 6 commits
61151c9
37d4225
e0d7755
96c2afa
bc27e77
2abec0f
a267a7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,6 +177,8 @@ object GpuCast extends Arm { | |
private val TIMESTAMP_TRUNCATE_REGEX = "^([0-9]{4}-[0-9]{2}-[0-9]{2} " + | ||
"[0-9]{2}:[0-9]{2}:[0-9]{2})" + | ||
"(.[1-9]*(?:0)?[1-9]+)?(.0*[1-9]+)?(?:.0*)?$" | ||
private val bigDecimalLongMin = BigDecimal(Long.MinValue) | ||
private val bigDecimalLongMax = BigDecimal(Long.MaxValue) | ||
|
||
val INVALID_INPUT_MESSAGE: String = "Column contains at least one value that is not in the " + | ||
"required range" | ||
|
@@ -378,11 +380,19 @@ object GpuCast extends Arm { | |
// ansi cast from larger-than-long integral-like types, to long | ||
case (dt: DecimalType, LongType) if ansiMode => | ||
// This is a work around for https://github.com/rapidsai/cudf/issues/9282 | ||
val min = BigDecimal(Long.MinValue) | ||
.setScale(dt.scale, BigDecimal.RoundingMode.DOWN).bigDecimal | ||
val max = BigDecimal(Long.MaxValue) | ||
.setScale(dt.scale, BigDecimal.RoundingMode.DOWN).bigDecimal | ||
assertValuesInRange(input, Scalar.fromDecimal(min), Scalar.fromDecimal(max)) | ||
val min = bigDecimalLongMin.setScale(dt.scale, BigDecimal.RoundingMode.DOWN).bigDecimal | ||
val max = bigDecimalLongMax.setScale(dt.scale, BigDecimal.RoundingMode.DOWN).bigDecimal | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// We are going against our convention of calling assertValuesInRange() | ||
// because the min/max values are a different decimal type i.e. Decimal 128 as opposed to | ||
// the incoming input column type. | ||
withResource(input.min()) { minInput => | ||
withResource(input.max()) { maxInput => | ||
if (minInput.isValid && minInput.getBigDecimal().compareTo(min) == -1 || | ||
maxInput.isValid && maxInput.getBigDecimal().compareTo(max) == 1) { | ||
throw new ArithmeticException(GpuCast.INVALID_INPUT_MESSAGE) | ||
} | ||
} | ||
} | ||
if (dt.precision <= DType.DECIMAL32_MAX_PRECISION && dt.scale < 0) { | ||
// This is a work around for https://github.com/rapidsai/cudf/issues/9281 | ||
withResource(input.castTo(DType.create(DType.DTypeEnum.DECIMAL64, -dt.scale))) { tmp => | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -466,6 +466,11 @@ class AnsiCastOpSuite extends GpuExpressionTestSuite { | |
} | ||
} | ||
|
||
testSparkResultsAreEqual("ansi_cast decimals to long", | ||
generateValidValuesDecimalDF(Short.MinValue, Short.MaxValue, 18, 3), sparkConf) { | ||
frame => testCastTo(DataTypes.LongType)(frame) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to test that we actually throw the expected exception? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is only for the valid values. May be I need to add a test that tests just that an exception is thrown? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We won't need to have a new case as you are throwing the same exception because this will be covered in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, then that test is already testing the exception that is thrown. |
||
private def castToStringExpectedFun[T]: T => Option[String] = (d: T) => Some(String.valueOf(d)) | ||
|
||
private def testCastToString[T](dataType: DataType, ansiMode: Boolean, | ||
|
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.