HostColumnVectoreCore#isNull should return true for out-of-range rows #10779
Conversation
Signed-off-by: Gera Shegalov <gera@apache.org>
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #10779 +/- ##
================================================
+ Coverage 86.40% 86.43% +0.03%
================================================
Files 143 143
Lines 22448 22448
================================================
+ Hits 19396 19403 +7
+ Misses 3052 3045 -7
Continue to review full report at Codecov.
|
ttnghia
changed the title
| return BitVectorHelper.isNull(offHeap.valid, rowIndex); | ||
| } | ||
| return false; | ||
| return rowIndex < 0 || rowIndex >= rows // unknown, hence NULL |
There was a problem hiding this comment.
I don't think this is logically correct. Accessing out of bounds is different from having null.
There was a problem hiding this comment.
We get to decide what is logically correct because we define the logic. E.g., we have different out-of-bound policies in other use cases. We could make it configurable here as well once there is a different use case necessitating it.
I need to instrument codegen in Spark to get a better grasp of how we end up in this lookup. However, in terms of tests it appears to be a fix that strictly improves the plugin because it does not break existing tests.
gerashegalov
added
the
5 - Ready to Merge
|
@gpucibot merge |
This PR adds tests verifying the fix for correctly dealing with all-null data partitions in rapidsai/cudf#10779. Signed-off-by: Gera Shegalov <gera@apache.org>
This PR suggests a 3VL way of interpreting
isNullfor arowIdout of bounds. Such a value is unknown and therefore isNull should betrue.NVIDIA/spark-rapids#5140 shows that
SpecificUnsafeProjectionmay probe child columns for NULL even though the parent column row is also NULL.However there are no rows in the child CV when the parent row is NULL leading to an assert violation if asserts are enabled or an NPE if disabled.
Signed-off-by: Gera Shegalov gera@apache.org