Fixed a few issues with out of core sort #2209
Conversation
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
|
build |
|
build |
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuSortExec.scala
Outdated
Show resolved
Hide resolved
|
build |
| @@ -284,7 +283,7 @@ case class GpuOutOfCoreSortIterator( | |||
| // Protect ourselves from large rows when there are small targetSizes | |||
| val targetRowCount = Math.max((targetBatchSize/averageRowSize).toInt, 1024) | |||
|
|
|||
| if (sortedOffset == rows - 1) { | |||
| if (sortedOffset == rows) { | |||
There was a problem hiding this comment.
I think the code would read easier if we renamed sortedOffset to sortedRows or numSortedRows
There was a problem hiding this comment.
I personally think of it in terms of offsets instead of number of rows. The fact that they end up being equal is just because the sorted values are at the first part of the batch.
There was a problem hiding this comment.
Not a blocking issue.
Just to explain my thingking: I think of an offset as the start position of an array, or a range the way it's used on L300 [sortedOffset, rows). Since it describes the unsorted range, if we wanted to use the term offset, I'd call it unsortedOffset. On the other hand, we can view it as the definition of the sorted area [0, sortedOffset) in which case sortedRows works better for me.
|
build |
|
@gerashegalov is it OK if I merge this? or do you want me to make changes because the memory model is not OK |
| @@ -284,7 +283,7 @@ case class GpuOutOfCoreSortIterator( | |||
| // Protect ourselves from large rows when there are small targetSizes | |||
| val targetRowCount = Math.max((targetBatchSize/averageRowSize).toInt, 1024) | |||
|
|
|||
| if (sortedOffset == rows - 1) { | |||
| if (sortedOffset == rows) { | |||
There was a problem hiding this comment.
Not a blocking issue.
Just to explain my thingking: I think of an offset as the start position of an array, or a range the way it's used on L300 [sortedOffset, rows). Since it describes the unsorted range, if we wanted to use the term offset, I'd call it unsortedOffset. On the other hand, we can view it as the definition of the sorted area [0, sortedOffset) in which case sortedRows works better for me.
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
This fixes an off by one error when and entire batch is already sorted. It fixes an issue when inserting batches into the pending queue, and it fixes an issue when sorting only rows, no columns. Any of what could lead to data corruption.