Add partial and final only hash aggregate tests and fix nulls corner case for Average #157
Conversation
kuhushukla
added
bug
kuhushukla
changed the title
| } | ||
|
|
||
| override def doColumnar(lhs: Scalar, rhs: GpuColumnVector): GpuColumnVector = { | ||
| throw new IllegalStateException("Should not be used with lhs as scalar") |
There was a problem hiding this comment.
I would like to explore this and see if we need to add this to GpuOverrides as a follow on since it is even more outside the scope of this PR.
There was a problem hiding this comment.
Nvl is a Coalesce that only takes 2 args. the nvl function gets turned into a Coalesce, which we turn into a GpuCoalesce. We should probably document that her to make it clear what is happening, or we should just sue GpuCoalesce directly instead.
There was a problem hiding this comment.
Thanks for that pointer as my Nvl knowledge is limited. I will try out GpuCoalesce.
|
build |
| } | ||
|
|
||
| override def doColumnar(lhs: Scalar, rhs: GpuColumnVector): GpuColumnVector = { | ||
| throw new IllegalStateException("Should not be used with lhs as scalar") |
There was a problem hiding this comment.
Nvl is a Coalesce that only takes 2 args. the nvl function gets turned into a Coalesce, which we turn into a GpuCoalesce. We should probably document that her to make it clear what is happening, or we should just sue GpuCoalesce directly instead.
|
The tests are getting really complicated with the parameters, etc, and at this point I would almost rather see more tests, that look like near duplicates of each other, just so it is more readable. |
That makes sense. I will try and make it a bit more easy to grok. |
|
@revans2 , I have done the following to address:
I removed To address:
I replaced GpuNvl with this and it works. Thanks for the reviews and inputs. Appreciate it. |
|
build |
…case for Average (NVIDIA#157) * make config passing configurable in hash aggregate tests Co-authored-by: Kuhu Shukla <kuhus@nvidia.com>
…case for Average (NVIDIA#157) * make config passing configurable in hash aggregate tests Co-authored-by: Kuhu Shukla <kuhus@nvidia.com>
Signed-off-by: spark-rapids automation <70000568+nvauto@users.noreply.github.com>
Please write a description in this text box of the changes that are being made.
This PR fixes #110 and #154
and adds tests for
finalandpartialmodes for hash aggregate tests. It highlights a couple of failures/bugs caught during testing (marked xfail with comments)uses GpuNvl/GpuCoalesce to stop averages to pass down nulls to the CPU. Additionally contains minor nits to the python test file for hash aggregates.