Decimal32 support #1717

razajafri · 2021-02-12T17:38:37Z

This PR handles Decimal32 support.

Tests for division are ~~failing~~ all passing
Support for nested types for reading legacy Decimals is ~~lacking~~ done
~~While copying vector back on the Host in HostColumnarToGpu I am getting the BigDecimal value, it could be optimized by getting the unscaledLong value like we were before this PR~~ This is now handled based on the precision
There might be issues with casting from D32 to D64 that I was seeing before but I don't see them now. It could be attributed to the scale value flipped in cudf, so I might have made a mistake while evaluating them this was happening because I was confused between the scale values being opposite in cudf

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala

sql-plugin/src/main/scala/com/nvidia/spark/rapids/decimalExpressions.scala

sql-plugin/src/main/scala/com/nvidia/spark/rapids/literals.scala

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala

revans2

I didn't have time to finish all of the review, but I thought I would get some of my comments in for today.

sql-plugin/src/main/java/org/apache/spark/sql/catalyst/CudfUnsafeRow.java

sql-plugin/src/main/scala/com/nvidia/spark/rapids/DecimalUtil.scala

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri · 2021-03-30T23:41:12Z

build

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri · 2021-03-31T17:19:25Z

build

revans2

Overall it is looking okay. I am a bit concerned about all of the extra code in the binary operators for decimal. It just feels like there should be a simpler way to do this. Especially with type erasure in cudf. It mostly feels like we just need to check if the output needs to be 64-bit and if it does then we don't need to check any of the children, because a no-op cast is cheap.

integration_tests/src/main/python/arithmetic_ops_test.py

...311/src/main/scala/com/nvidia/spark/rapids/shims/spark311/ParquetCachedBatchSerializer.scala

...311/src/main/scala/org/apache/spark/sql/rapids/shims/spark311/GpuInMemoryTableScanExec.scala

sql-plugin/src/main/scala/com/nvidia/spark/rapids/DecimalUtil.scala

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuParquetScan.scala

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuRowToColumnarExec.scala

revans2 · 2021-03-31T18:12:01Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/arithmetic.scala

-        withResource(Scalar.fromDecimal(-scale, 0L)) { scalar =>
-          scalar.sub(input.getBase)
+        if (DecimalType.is32BitDecimalType(dt)) {
+          withResource(Scalar.fromDecimal(-scale, 0)) { scalar =>


nit: It would be nice, perhaps as a follow on issue, to Make creating a Scalar decimal value simpler. That way we could combine the code together a bit better.

razajafri · 2021-03-31T19:32:15Z

Overall it is looking okay. I am a bit concerned about all of the extra code in the binary operators for decimal. It just feels like there should be a simpler way to do this. Especially with type erasure in cudf. It mostly feels like we just need to check if the output needs to be 64-bit and if it does then we don't need to check any of the children, because a no-op cast is cheap.

I am not sure I understand, if the output is 64-bit but the operands are 32-bit, it will result in an overflow so we have to cast the inputs to 64-bit decimal so there is no overflow.

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri · 2021-04-03T00:23:31Z

build

razajafri · 2021-04-03T00:38:32Z

@revans2 I have filed a couple of follow-on issues. One for refactoring the binaryoperations here and the other one for making the creation of scalar simpler here. PTAL and let me know if you have any more concerns

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri · 2021-04-13T15:00:02Z

build

revans2

Just a nit left, and I am OK with this going in without it

revans2 · 2021-04-13T15:28:54Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/DecimalUtil.scala

+
+object DecimalUtil {
+
+  def createCudfDecimal(precision: Int, scale: Int): DType = {


nit: can we have some comments here? I would like it clean that the input precision and scale should be what Spark expects and this will convert it into whatever CUDF expects.

I will fix this in one of my PRs

* Add support for Decimal32 Signed-off-by: Raza Jafri <rjafri@nvidia.com> * fixed unary_minus Signed-off-by: Raza Jafri <rjafri@nvidia.com> * unscaledLong fix Signed-off-by: Raza Jafri <rjafri@nvidia.com> * More support for Decimal32 Signed-off-by: Raza Jafri <rjafri@nvidia.com> * implicit for casting dec32todec64 Signed-off-by: Raza Jafri <rjafri@nvidia.com> * cleanup Signed-off-by: Raza Jafri <rjafri@nvidia.com> * refactored castDecimalToDecimal Signed-off-by: Raza Jafri <rjafri@nvidia.com> * fixed legacy decimal read for non-nested Signed-off-by: Raza Jafri <rjafri@nvidia.com> * fixed casting and added more tests Signed-off-by: Raza Jafri <rjafri@nvidia.com> * added nested tests for reading legacy decimals Signed-off-by: Raza Jafri <rjafri@nvidia.com> * removed implicit Signed-off-by: Raza Jafri <rjafri@nvidia.com> * struct working Signed-off-by: Raza Jafri <rjafri@nvidia.com> * Lists working Signed-off-by: Raza Jafri <rjafri@nvidia.com> * divide not working Signed-off-by: Raza Jafri <rjafri@nvidia.com> * cleanup Signed-off-by: Raza Jafri <rjafri@nvidia.com> * division working but problem with casting Signed-off-by: Raza Jafri <rjafri@nvidia.com> * moved div code to GpuModLike Signed-off-by: Raza Jafri <rjafri@nvidia.com> * code cleanup Signed-off-by: Raza Jafri <rjafri@nvidia.com> * some more fixes Signed-off-by: Raza Jafri <rjafri@nvidia.com> * some more fixes Signed-off-by: Raza Jafri <rjafri@nvidia.com> * addressed review comments Signed-off-by: Raza Jafri <rjafri@nvidia.com> * added more comments Signed-off-by: Raza Jafri <rjafri@nvidia.com> * park Signed-off-by: Raza Jafri <rjafri@nvidia.com> * properly cast scalar Signed-off-by: Raza Jafri <rjafri@nvidia.com> * fixed gpu metric Signed-off-by: Raza Jafri <rjafri@nvidia.com> * upmerged Signed-off-by: Raza Jafri <rjafri@nvidia.com> * fixed castFloatsToDecimals to pick the right precision Signed-off-by: Raza Jafri <rjafri@nvidia.com> * addressed review comments Signed-off-by: Raza Jafri <rjafri@nvidia.com> * Fixed memory leak Signed-off-by: Raza Jafri <rjafri@nvidia.com> * removed length restriction Signed-off-by: Raza Jafri <rjafri@nvidia.com> * fixed test failure due to upmerge Signed-off-by: Raza Jafri <rjafri@nvidia.com> Co-authored-by: Raza Jafri <rjafri@nvidia.com>

razajafri changed the title ~~Decimal32 support~~ [WIP] Decimal32 support Feb 12, 2021

razajafri marked this pull request as draft February 12, 2021 17:46

razajafri changed the title ~~[WIP] Decimal32 support~~ Decimal32 support Feb 12, 2021

razajafri mentioned this pull request Feb 12, 2021

[FEA] Explore support for DECIMAL32 #1317

Closed

sameerz added the feature request New feature or request label Feb 13, 2021

razajafri changed the base branch from branch-0.4 to branch-0.5 February 22, 2021 20:27

razajafri commented Feb 26, 2021

View reviewed changes

razajafri added 18 commits February 26, 2021 09:36

Add support for Decimal32

3b67957

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

fixed unary_minus

a9388e8

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

unscaledLong fix

1c31d30

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

More support for Decimal32

8832ffa

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

implicit for casting dec32todec64

7db8137

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

cleanup

b23754c

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

refactored castDecimalToDecimal

2c88a75

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

fixed legacy decimal read for non-nested

15b8ff5

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

fixed casting and added more tests

6060aea

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

added nested tests for reading legacy decimals

e486bd1

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

removed implicit

cc82c29

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

struct working

8389019

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

Lists working

e573a40

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

divide not working

b538ee6

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

cleanup

a8b3b0a

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

division working but problem with casting

26fa64c

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

moved div code to GpuModLike

38da4e3

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

code cleanup

8702bef

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri force-pushed the decimal32_support branch from 3a0a098 to 8702bef Compare February 26, 2021 21:06

some more fixes

6e8310e

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri commented Mar 1, 2021

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala Show resolved Hide resolved

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala Show resolved Hide resolved

razajafri marked this pull request as ready for review March 1, 2021 20:39

revans2 reviewed Mar 1, 2021

View reviewed changes

razajafri added 3 commits March 30, 2021 10:37

fixed gpu metric

a6d42c5

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

Merge remote-tracking branch 'origin/branch-0.5' into decimal32

390016d

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

upmerged

900c919

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

fixed castFloatsToDecimals to pick the right precision

6fd98c9

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

revans2 reviewed Mar 31, 2021

View reviewed changes

sameerz modified the milestones: Mar 15 - March 26, Mar 30 - Apr 9 Mar 31, 2021

razajafri added 4 commits March 31, 2021 14:34

addressed review comments

e14b3a7

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

Fixed memory leak

95448c3

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

Merge remote-tracking branch 'origin/branch-0.5' into decimal32

4101856

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

removed length restriction

1676c87

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri mentioned this pull request Apr 3, 2021

Make creation of a Scalar decimal value simpler #2076

Closed

razajafri mentioned this pull request Apr 6, 2021

add nightly cache tests #2083

Merged

sameerz modified the milestones: Mar 29 - Apr 9, Apr 12 - Apr 23 Apr 9, 2021

razajafri added 2 commits April 12, 2021 14:56

Merge remote-tracking branch 'origin/branch-0.5' into decimal32

55b32e9

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

fixed test failure due to upmerge

0dfc529

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

revans2 approved these changes Apr 13, 2021

View reviewed changes

razajafri merged commit cc47a25 into NVIDIA:branch-0.5 Apr 13, 2021

razajafri deleted the decimal32_support branch April 13, 2021 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decimal32 support #1717

Decimal32 support #1717

razajafri commented Feb 12, 2021 •

edited

Loading

revans2 left a comment

razajafri commented Mar 30, 2021

razajafri commented Mar 31, 2021

revans2 left a comment

revans2 Mar 31, 2021

razajafri commented Mar 31, 2021

razajafri commented Apr 3, 2021

razajafri commented Apr 3, 2021

razajafri commented Apr 13, 2021

revans2 left a comment

revans2 Apr 13, 2021

razajafri Apr 13, 2021


		object DecimalUtil {

		def createCudfDecimal(precision: Int, scale: Int): DType = {

Decimal32 support #1717

Decimal32 support #1717

Conversation

razajafri commented Feb 12, 2021 • edited Loading

revans2 left a comment

Choose a reason for hiding this comment

razajafri commented Mar 30, 2021

razajafri commented Mar 31, 2021

revans2 left a comment

Choose a reason for hiding this comment

revans2 Mar 31, 2021

Choose a reason for hiding this comment

razajafri commented Mar 31, 2021

razajafri commented Apr 3, 2021

razajafri commented Apr 3, 2021

razajafri commented Apr 13, 2021

revans2 left a comment

Choose a reason for hiding this comment

revans2 Apr 13, 2021

Choose a reason for hiding this comment

razajafri Apr 13, 2021

Choose a reason for hiding this comment

razajafri commented Feb 12, 2021 •

edited

Loading