Fix null-bounds calculation for ranged window queries

[mythrocks]

This commit fixes an off-by-one error in grouped_time_range_rolling_window(), for cases where the order-by column (i.e. the timestamp) contains null values.

The error was reported when running Spark with the spark-rapids plugin enabled, manifesting in a process crash:

21/03/11 04:26:51 ERROR TaskSetManager: Task 3 in stage 4.0 failed 1 times; aborting job
org.apache.spark.SparkException: Job aborted due to stage failure: Task 3 in stage 4.0 failed 1 times, most recent failure: Los
t task 3.0 in stage 4.0 (TID 5, 10.0.0.23, executor driver): ai.rapids.cudf.CudfException: for_each: failed to synchronize: cud
aErrorLaunchFailure: unspecified launch failure
        at ai.rapids.cudf.Table.timeRangeRollingWindowAggregate(Native Method)
        at ai.rapids.cudf.Table.access$3100(Table.java:45)
        at ai.rapids.cudf.Table$AggregateOperation.aggregateWindowsOverTimeRanges(Table.java:2258)
        at com.nvidia.spark.rapids.GpuWindowExpression.$anonfun$evaluateRangeBasedWindowExpression$2(GpuWindowExpression.scala:
233)
        at com.nvidia.spark.rapids.Arm.withResource(Arm.scala:28)
        at com.nvidia.spark.rapids.Arm.withResource$(Arm.scala:26)
        at com.nvidia.spark.rapids.GpuWindowExpression.withResource(GpuWindowExpression.scala:132)
        at com.nvidia.spark.rapids.GpuWindowExpression.$anonfun$evaluateRangeBasedWindowExpression$1(GpuWindowExpression.scala:
224)

It is interesting that a fair number of the 870+ tests from 75 test cases in grouped_rolling_test.cpp exercise this code path. cuda-memcheck still does not seem to detect the corruption in CountMultiGroupTimestampASCNullsFirst, for instance:

✗ cuda-memcheck gtests/GROUPED_ROLLING_TEST --gtest_filter=TypedNullTimestampTestForRangeQ eries/0.CountMultiGroupTimestampASCNullsFirst
========= CUDA-MEMCHECK
Note: Google Test filter = TypedNullTimestampTestForRangeQueries/0.CountMultiGroupTimestampASCNullsFirst
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from TypedNullTimestampTestForRangeQueries/0, where TypeParam = signed char
[ RUN      ] TypedNullTimestampTestForRangeQueries/0.CountMultiGroupTimestampASCNullsFirst
[       OK ] TypedNullTimestampTestForRangeQueries/0.CountMultiGroupTimestampASCNullsFirst (663 ms)
[----------] 1 test from TypedNullTimestampTestForRangeQueries/0 (663 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (663 ms total)
[  PASSED  ] 1 test.
========= ERROR SUMMARY: 0 errors

[mythrocks]

I've raised this against branch-0.18 since this is a bug in a released version of libcudf. The user will experience application crashes or data corruption. I'm not sure of the best way to update a released CUDF version.

I'd be happy to retarget this PR to branch-0.19 if that's preferable.

[kkraus14]

See the hotfix process here: https://docs.rapids.ai/releases/hotfix/

cc @jrhemstad @harrism for visibility and to weigh in on if this is hotfix worthy

[kkraus14]

In addition to the fix we should add a test that captures this behavior.

[mythrocks]

Hello, @kkraus14. Thank you for your advice.

In addition to the fix we should add a test that captures this behavior

This is an off-by-1 error that should have been triggered by tests that currently exist in grouped_rolling_test.cpp. (CountMultiGroupTimestampASCNullsFirst, for instance.) I am as yet unable to reliably reproduce the crash from unit-tests, short of using a manually extracted Parquet file. The crash isn't a consistent one, outside of running cuda-memcheck as part of the test.

Please pardon my temerity, but I'm not sure I would have written a test for this case apart from the ones we already have. :/ (I'll keep trying, though.)

[mythrocks]

See the hotfix process here: https://docs.rapids.ai/releases/hotfix/

The process prescribes the following:

2. Create your branch from the branch-M.B branch

M.B is the next minor release i.e. 0.19. I will re-target this PR for branch-0.19, and raise a separate one for main.

[kkraus14]

Will let @harrism take a look before we move forward in starting the hotfix release process. Thanks all.

[mike-wendt]

Working on getting the tests running on this as we don't have testing enabled for this branch as it is not active

[raydouglass]

rerun tests

[kkraus14]

Python build failures are expected because of the Dask branch changes. Do we need to resolve these things on 0.18?

[mike-wendt]

Python build failures are expected because of the Dask branch changes. Do we need to resolve these things on 0.18?

@mythrocks can you patch the contents of #7535 onto your PR? This way we can get the Python builds and GPU tests to work now that Dask has changed their default branch?

[kkraus14]

Needs changes from #7532 as well. I'll add them.

[kkraus14]

@mike-wendt changes should be added

[mike-wendt]

Will merge when @raydouglass returns this afternoon

[mythrocks]

Python build failures are expected because of the Dask branch changes. Do we need to resolve these things on 0.18?

@mythrocks can you patch the contents of #7535 onto your PR? This way we can get the Python builds and GPU tests to work now that Dask has changed their default branch?

@mike-wendt: Acknowledging here with gratitude that @kkraus14 updated the PR before I even read this message. :] Thanks!

[mythrocks]

Hmm... Tests are failing for Dask:

10:13:33 Tests failed for dask-cudf-0.18.0a-py37_gb4b3c72479_258.tar.bz2 - moving package to /opt/conda/envs/rapids/conda-bld/broken
10:13:33 WARNING:conda_build.build:Tests failed for dask-cudf-0.18.0a-py37_gb4b3c72479_258.tar.bz2 - moving package to /opt/conda/envs/rapids/conda-bld/broken
10:13:33 WARNING conda_build.build:tests_failed(2890): Tests failed for dask-cudf-0.18.0a-py37_gb4b3c72479_258.tar.bz2 - moving package to /opt/conda/envs/rapids/conda-bld/broken
10:13:33 export PREFIX=/opt/conda/envs/rapids/conda-bld/dask-cudf_1615572558850/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh

Is this a failure to build the conda env?

[kkraus14]

13:15:12 ImportError: cannot import name 'stringify_path' from 'dask.bytes.core' (/opt/conda/envs/rapids/conda-bld/dask-cudf_1615572592450/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/lib/python3.7/site-packages/dask/bytes/core.py)

Needs the fix from #7580

[harrism]

I'm concerned that our guests failed to catch an error like this. @mythrocks if this code path is exercised by multiple tests as you say, why do none of them fail? This PR should add a test that fails without the fix and succeeds after the fix.

[kkraus14]

I'm concerned that our guests failed to catch an error like this. @mythrocks if this code path is exercised by multiple tests as you say, why do none of them fail? This PR should add a test that fails without the fix and succeeds after the fix.

Lets add the test in 0.19 so as to not delay the hotfix any further?

[mythrocks]

This PR should add a test that fails without the fix and succeeds after the fix.

It hasn't been from want of effort. Outside of exercising this code path from Spark through a handcrafted Parquet file, I have been unable to reproduce the crash, even by constructing the same input columns programmatically.

Lets add the test in 0.19 so as to not delay the hotfix any further?

I have filed #7590 to add a test to branch-0.19. I will try hit this ASAP.