[REVIEW] Support building decimal columns with Table.TestBuilder [skip ci] #6770
Conversation
|
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
sperlingxx
changed the title
Codecov Report
@@ Coverage Diff @@
## branch-0.17 #6770 +/- ##
===============================================
+ Coverage 81.73% 81.77% +0.04%
===============================================
Files 96 96
Lines 15981 15885 -96
===============================================
- Hits 13062 12990 -72
+ Misses 2919 2895 -24
Continue to review full report at Codecov.
|
|
I went through this PR and it looks good to me. It would be great if we could get it reviewed from @revans2 as he had reviewed and had comments on this part ( |
| int scale = type.getScale(); | ||
| if (dataArray instanceof Integer[]) { | ||
| BigDecimal[] data = Arrays.stream(((Integer[]) dataArray)) | ||
| .map((i) -> i == null ? null : BigDecimal.valueOf(i, -scale)) |
There was a problem hiding this comment.
With unscaled values provided, why use BigDecimal instead of using the much cheaper unscaled value decimal constructors?
There was a problem hiding this comment.
Because unscaled value constructors can not support null values, since it is for primitive values.
There was a problem hiding this comment.
unscaled value constructors can not support null values
That seems like a problem with the value constructors that needs a followup. We support value constructors for Integer values that can accept null values, so why can't we do the same for unscaled decimal values? Having to go through BigDecimal to workaround a lack of functionality in the value constructor for decimals is not ideal.
This PR is about to support building decimal columns with Table.TestBuilder, which is widely used in automatic tests of spark-rapids plugin.