Add in support for NULL_LOGICAL_AND and NULL_LOGICAL_OR binops #10016

revans2 · 2022-01-11T15:29:04Z

These already exist as a part of the AST. Spark's AND/OR implementations follow these requirements and to be able to re-implement it using existing CUDF functionality ended up being very expensive. We found that this one change could cut almost 13% off the total run time on TPC-DS query 28. AND/OR are common enough in all queries we expect this to have a major performance impact generally.

We tried to use the AST version instead, but depending on the hardware used the overhead of AST does not pay for itself when the input/intermediate outputs are boolean columns. It appears to be because the amount of memory transfers saved is relatively small in most boolean cases and on large GPUs like the a100 the intermediate results might even fit entirely in the L2 cache.

cpp/include/cudf/binaryop.hpp

java/src/test/java/ai/rapids/cudf/BinaryOpTest.java

jbrennan333

one minor question

java/src/main/java/ai/rapids/cudf/BinaryOp.java

jbrennan333 · 2022-01-11T16:24:09Z

I am not seeing src/binaryop/compiled/NullLogicalOr.cu nor src/binaryop/compiled/NullLogicalAnd.cu in the PR?

jlowe

Should we add convenience functions in BinaryOperable to make this a bit cleaner to call from the Java API?

vyasr · 2022-01-11T19:17:43Z

Will this PR finish the remainder of #8980?

revans2 · 2022-01-11T20:02:17Z

Will this PR finish the remainder of #8980?

yes I just linked the two

codecov · 2022-01-11T20:51:06Z

Codecov Report

Merging #10016 (b3ff013) into branch-22.02 (967a333) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.02   #10016      +/-   ##
================================================
- Coverage         10.49%   10.40%   -0.09%     
================================================
  Files               119      119              
  Lines             20305    20557     +252     
================================================
+ Hits               2130     2139       +9     
- Misses            18175    18418     +243

Impacted Files	Coverage Δ
python/custreamz/custreamz/kafka.py	`29.16% <0.00%> (-0.63%)`	⬇️
python/dask_cudf/dask_cudf/sorting.py	`92.66% <0.00%> (-0.25%)`	⬇️
python/dask_cudf/dask_cudf/core.py	`70.85% <0.00%> (-0.17%)`	⬇️
python/cudf/cudf/__init__.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/api/types.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/frame.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/index.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/io/parquet.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/dtypes.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/scalar.py	`0.00% <0.00%> (ø)`
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fd7dd2...b3ff013. Read the comment docs.

cpp/include/cudf/binaryop.hpp

cpp/tests/binaryop/binop-compiled-test.cpp

cpp/src/binaryop/compiled/operation.cuh

revans2 · 2022-01-13T19:27:12Z

I posted a small performance improvement that replaced the conditionals with boolean operations. This showed an 11% performance boost on operations that do not have nulls in them. I have no idea why it didn't help the operations that do have nulls in them. It might be nice to look at the AST at some point to see if we can make an improvement there too, but that is separate.

revans2 · 2022-01-14T19:03:58Z

rerun tests

revans2 · 2022-01-18T14:40:32Z

rerun tests

jbrennan333

+1 this looks good to me.

revans2 · 2022-01-19T16:49:24Z

rerun tests

revans2 · 2022-01-19T18:58:21Z

Something is going on with the build where the python tests are failing and I have no idea what it could be. Some of them have errors like

11:53:37 ________________ ERROR collecting cudf/tests/test_api_types.py _________________
11:53:37 ImportError while importing test module '/workspace/python/cudf/cudf/tests/test_api_types.py'.
11:53:37 Hint: make sure your test modules/packages have valid Python names.
11:53:37 Traceback:
11:53:37 /opt/conda/envs/rapids/lib/python3.8/importlib/__init__.py:127: in import_module
11:53:37     return _bootstrap._gcd_import(name[level:], package, level)
11:53:37 cudf/tests/test_api_types.py:8: in <module>
11:53:37     import cudf
11:53:37 /opt/conda/envs/rapids/lib/python3.8/site-packages/cudf/__init__.py:5: in <module>
11:53:37     validate_setup()
11:53:37 /opt/conda/envs/rapids/lib/python3.8/site-packages/cudf/utils/gpu_utils.py:18: in validate_setup
11:53:37     from rmm._cuda.gpu import (
11:53:37 E   ImportError: cannot import name 'cudaDeviceAttr' from 'rmm._cuda.gpu' (/opt/conda/envs/rapids/lib/python3.8/site-packages/rmm/_cuda/gpu.py)
11:53:37 ________________ ERROR collecting cudf/tests/test_apply_rows.py ________________

They all appear to indicate that I am having problems importing 'cudaDeviceAttr', but that is something cuda specific and I didn't touch anything python, or RMM, etc. Any help debugging this would really be appreciated.

bdice · 2022-01-19T19:18:48Z

@revans2 CI will fail until #10008 is merged.

vyasr · 2022-01-20T00:54:12Z

rerun tests

revans2 · 2022-01-20T12:20:16Z

@gpucibot merge

Add in support for NULL_LOGICAL_AND and NULL_LOGICAL_OR binops

c18ba01

revans2 added feature request New feature or request 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. Java Affects Java cuDF API. Spark Functionality that helps Spark RAPIDS non-breaking Non-breaking change labels Jan 11, 2022

revans2 self-assigned this Jan 11, 2022

revans2 requested review from a team as code owners January 11, 2022 15:29

revans2 requested review from cwharris and codereport January 11, 2022 15:29

github-actions bot added the CMake CMake build issue label Jan 11, 2022

abellina reviewed Jan 11, 2022

View reviewed changes

cpp/include/cudf/binaryop.hpp Outdated Show resolved Hide resolved

revans2 mentioned this pull request Jan 11, 2022

Switch and/or to use new cudf binops to improve performance NVIDIA/spark-rapids#4501

Merged

jlowe reviewed Jan 11, 2022

View reviewed changes

java/src/test/java/ai/rapids/cudf/BinaryOpTest.java Show resolved Hide resolved

Addressed review comments and style checks

f4fdbd3

jlowe approved these changes Jan 11, 2022

View reviewed changes

jbrennan333 reviewed Jan 11, 2022

View reviewed changes

java/src/main/java/ai/rapids/cudf/BinaryOp.java Show resolved Hide resolved

Missed some files

7ec2e64

jlowe reviewed Jan 11, 2022

View reviewed changes

revans2 added 2 commits January 11, 2022 12:25

Merge branch 'branch-22.02' into null_aware_and_or

7713078

Fix merge conflict

6a63125

revans2 linked an issue Jan 11, 2022 that may be closed by this pull request

[FEA] Null-tolerant logical AND and OR operations #8980

Closed

codereport suggested changes Jan 11, 2022

View reviewed changes

cpp/include/cudf/binaryop.hpp Outdated Show resolved Hide resolved

cpp/tests/binaryop/binop-compiled-test.cpp Outdated Show resolved Hide resolved

cpp/src/binaryop/compiled/operation.cuh Outdated Show resolved Hide resolved

revans2 added 3 commits January 12, 2022 11:12

Addressed review comments

a273f00

Performance boost

b4c7487

fix style again

e1c6d68

Merge branch 'branch-22.02' into null_aware_and_or

c89e7bc

codereport approved these changes Jan 14, 2022

View reviewed changes

Merge branch 'branch-22.02' into null_aware_and_or

b3ff013

jrhemstad approved these changes Jan 19, 2022

View reviewed changes

jbrennan333 approved these changes Jan 19, 2022

View reviewed changes

jbrennan333 mentioned this pull request Jan 19, 2022

Implement GpuAnd/GpuOr using AST expressions NVIDIA/spark-rapids#4473

Closed

rapids-bot bot merged commit d5f1aed into rapidsai:branch-22.02 Jan 20, 2022

revans2 deleted the null_aware_and_or branch January 20, 2022 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add in support for NULL_LOGICAL_AND and NULL_LOGICAL_OR binops #10016

Add in support for NULL_LOGICAL_AND and NULL_LOGICAL_OR binops #10016

revans2 commented Jan 11, 2022

jbrennan333 left a comment

jbrennan333 commented Jan 11, 2022

jlowe left a comment

vyasr commented Jan 11, 2022

revans2 commented Jan 11, 2022

codecov bot commented Jan 11, 2022 •

edited

Loading

revans2 commented Jan 13, 2022

revans2 commented Jan 14, 2022

revans2 commented Jan 18, 2022

jbrennan333 left a comment

revans2 commented Jan 19, 2022

revans2 commented Jan 19, 2022 •

edited

Loading

bdice commented Jan 19, 2022

vyasr commented Jan 20, 2022

revans2 commented Jan 20, 2022

Add in support for NULL_LOGICAL_AND and NULL_LOGICAL_OR binops #10016

Add in support for NULL_LOGICAL_AND and NULL_LOGICAL_OR binops #10016

Conversation

revans2 commented Jan 11, 2022

jbrennan333 left a comment

Choose a reason for hiding this comment

jbrennan333 commented Jan 11, 2022

jlowe left a comment

Choose a reason for hiding this comment

vyasr commented Jan 11, 2022

revans2 commented Jan 11, 2022

codecov bot commented Jan 11, 2022 • edited Loading

Codecov Report

revans2 commented Jan 13, 2022

revans2 commented Jan 14, 2022

revans2 commented Jan 18, 2022

jbrennan333 left a comment

Choose a reason for hiding this comment

revans2 commented Jan 19, 2022

revans2 commented Jan 19, 2022 • edited Loading

bdice commented Jan 19, 2022

vyasr commented Jan 20, 2022

revans2 commented Jan 20, 2022

codecov bot commented Jan 11, 2022 •

edited

Loading

revans2 commented Jan 19, 2022 •

edited

Loading