[REVIEW] Remove bounds check for cudf::gather

CHANGELOG.md

@@ -1,3 +1,11 @@
+# cuDF 0.18.0 (Date TBD)
+
+## New Features
+
+## Improvements
+
+## Bug Fixes
+
 # cuDF 0.17.0 (Date TBD)
 
 ## New Features
@@ -35,6 +43,7 @@
 - PR #6765 Cupy fallback for __array_function__ and __array_ufunc__ for cudf.Series
 - PR #6817 Add support for scatter() on lists-of-struct columns
 - PR #6805 Implement `cudf::detail::copy_if` for `decimal32` and `decimal64`
+- PR #6847 Add a cmake find module for cuFile in JNI code
 
 ## Improvements
 
@@ -52,6 +61,7 @@
 - PR #6471 Replace index type-dispatch call with indexalator in cudf::strings::substring
 - PR #6485 Add File IO to cuIO benchmarks
 - PR #6504 Update Java bindings version to 0.17-SNAPSHOT
+- PR #6523 Remove bounds check for `cudf::gather`
 - PR #6489 Add `AVRO` fuzz tests with varying function parameters
 - PR #6540 Add dictionary support to `cudf::unary_operation`
 - PR #6537 Refactor ORC timezone
@@ -108,6 +118,7 @@
 - PR #6780 Move `cudf::cast` tests to separate test file
 - PR #6789 Rename `unary_op` to `unary_operator`
 - PR #6770 Support building decimal columns with Table.TestBuilder
+- PR #6800 Push DeviceScalar to cython-only
 - PR #6822 Split out `cudf::distinct_count` from `drop_duplicates.cu`
 - PR #6813 Enable `expand=False` in `.str.split` and `.str.rsplit`
 - PR #6829 Enable workaround to write categorical columns in csv
@@ -116,6 +127,7 @@
 - PR #6835 Move template param to member var to improve compile of hash/groupby.cu
 - PR #6837 Avoid gather when copying strings view from start of strings column
 - PR #6859 Move align_ptr_for_type() from cuda.cuh to alignment.hpp
+- PR #6523 Remove bound check for `cudf::gather`
 
 ## Bug Fixes
 

conda/environments/cudf_dev_cuda10.1.yml

@@ -9,7 +9,7 @@ dependencies:
   - clang=8.0.1
   - clang-tools=8.0.1
   - cupy>7.1.0,<9.0.0a0
-  - rmm=0.17.*
+  - rmm=0.18.*
   - cmake>=3.14
   - cmake_setuptools>=0.1.3
   - python>=3.6,<3.8

conda/environments/cudf_dev_cuda10.2.yml

@@ -9,7 +9,7 @@ dependencies:
   - clang=8.0.1
   - clang-tools=8.0.1
   - cupy>7.1.0,<9.0.0a0
-  - rmm=0.17.*
+  - rmm=0.18.*
   - cmake>=3.14
   - cmake_setuptools>=0.1.3
   - python>=3.6,<3.8

conda/environments/cudf_dev_cuda11.0.yml

@@ -9,7 +9,7 @@ dependencies:
   - clang=8.0.1
   - clang-tools=8.0.1
   - cupy>7.1.0,<9.0.0a0
-  - rmm=0.17.*
+  - rmm=0.18.*
   - cmake>=3.14
   - cmake_setuptools>=0.1.3
   - python>=3.6,<3.8

cpp/CMakeLists.txt

@@ -16,7 +16,7 @@
 
 cmake_minimum_required(VERSION 3.14...3.17 FATAL_ERROR)
 
-project(CUDA_DATAFRAME VERSION 0.17.0 LANGUAGES C CXX CUDA)
+project(CUDA_DATAFRAME VERSION 0.18.0 LANGUAGES C CXX CUDA)
 
 if(NOT CMAKE_CUDA_COMPILER)
   message(SEND_ERROR "CMake cannot locate a CUDA compiler")

cpp/cmake/Modules/FindcuFile.cmake

@@ -0,0 +1,121 @@
+#=============================================================================
+# Copyright (c) 2020, NVIDIA CORPORATION.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#[=======================================================================[.rst:
+FindcuFile
+----------
+
+Find cuFile headers and libraries.
+
+Imported Targets
+^^^^^^^^^^^^^^^^
+
+``cuFile::cuFile``
+  The cuFile library, if found.
+``cuFile::cuFileRDMA``
+  The cuFile RDMA library, if found.
+
+Result Variables
+^^^^^^^^^^^^^^^^
+
+This will define the following variables in your project:
+
+``cuFile_FOUND``
+  true if (the requested version of) cuFile is available.
+``cuFile_VERSION``
+  the version of cuFile.
+``cuFile_LIBRARIES``
+  the libraries to link against to use cuFile.
+``cuFileRDMA_LIBRARIES``
+  the libraries to link against to use cuFile RDMA.
+``cuFile_INCLUDE_DIRS``
+  where to find the cuFile headers.
+``cuFile_COMPILE_OPTIONS``
+  this should be passed to target_compile_options(), if the
+  target is not used for linking
+
+#]=======================================================================]
+
+
+# use pkg-config to get the directories and then use these values
+# in the FIND_PATH() and FIND_LIBRARY() calls
+find_package(PkgConfig QUIET)
+pkg_check_modules(PKG_cuFile QUIET cuFile)
+
+set(cuFile_COMPILE_OPTIONS ${PKG_cuFile_CFLAGS_OTHER})
+set(cuFile_VERSION ${PKG_cuFile_VERSION})
+
+find_path(cuFile_INCLUDE_DIR
+  NAMES
+    cufile.h
+  HINTS
+    ${PKG_cuFile_INCLUDE_DIRS}
+    /usr/local/gds/lib
+)
+
+find_library(cuFile_LIBRARY
+  NAMES
+    cufile
+  HINTS
+    ${PKG_cuFile_LIBRARY_DIRS}
+    /usr/local/gds/lib
+)
+
+find_library(cuFileRDMA_LIBRARY
+  NAMES
+    cufile_rdma
+  HINTS
+    ${PKG_cuFile_LIBRARY_DIRS}
+    /usr/local/gds/lib
+)
+
+include(FindPackageHandleStandardArgs)
+find_package_handle_standard_args(cuFile
+  FOUND_VAR
+    cuFile_FOUND
+  REQUIRED_VARS
+    cuFile_LIBRARY
+    cuFileRDMA_LIBRARY
+    cuFile_INCLUDE_DIR
+  VERSION_VAR
+    cuFile_VERSION
+)
+
+
+if (cuFile_FOUND AND NOT TARGET cuFile::cuFile)
+  add_library(cuFile::cuFile UNKNOWN IMPORTED)
+  set_target_properties(cuFile::cuFile PROPERTIES
+    IMPORTED_LOCATION "${cuFile_LIBRARY}"
+    INTERFACE_COMPILE_OPTIONS "${cuFile_COMPILE_OPTIONS}"
+    INTERFACE_INCLUDE_DIRECTORIES "${cuFile_INCLUDE_DIR}"
+  )
+endif ()
+
+if (cuFile_FOUND AND NOT TARGET cuFile::cuFileRDMA)
+  add_library(cuFile::cuFileRDMA UNKNOWN IMPORTED)
+  set_target_properties(cuFile::cuFileRDMA PROPERTIES
+    IMPORTED_LOCATION "${cuFileRDMA_LIBRARY}"
+    INTERFACE_COMPILE_OPTIONS "${cuFile_COMPILE_OPTIONS}"
+    INTERFACE_INCLUDE_DIRECTORIES "${cuFile_INCLUDE_DIR}"
+  )
+endif ()
+
+mark_as_advanced(cuFile_LIBRARY cuFileRDMA_LIBRARY cuFile_INCLUDE_DIR)
+
+if (cuFile_FOUND)
+  set(cuFile_LIBRARIES ${cuFile_LIBRARY})
+  set(cuFileRDMA_LIBRARIES ${cuFileRDMA_LIBRARY})
+  set(cuFile_INCLUDE_DIRS ${cuFile_INCLUDE_DIR})
+endif ()

cpp/include/cudf/copying.hpp

@@ -33,6 +33,18 @@ namespace cudf {
  * @brief Column APIs for gather, scatter, split, slice, etc.
  */
 
+/**
+ * @brief Policy to account for possible out-of-bounds indices
+ *
+ * `NULLIFY` means to nullify output values corresponding to out-of-bounds gather_map values.
+ * `DONT_CHECK` means do not check whether the indices are out-of-bounds, for better performance.
+ */
+
+enum class out_of_bounds_policy : int8_t {
+  NULLIFY,    /// Output values corresponding to out-of-bounds indices are null
+  DONT_CHECK  /// No bounds checking is performed, better performance
+};
+
 /**
  * @brief Gathers the specified rows (including null values) of a set of columns.
  *
@@ -49,22 +61,24 @@ namespace cudf {
  * For dictionary columns, the keys column component is copied and not trimmed
  * if the gather results in abandoned key elements.
  *
- * @throws cudf::logic_error if `check_bounds == true` and an index exists in
- * `gather_map` outside the range `[-n, n)`, where `n` is the number of rows in
- * the source table. If `check_bounds == false`, the behavior is undefined.
+ * @throws cudf::logic_error if gather_map contains null values.
  *
  * @param[in] source_table The input columns whose rows will be gathered
  * @param[in] gather_map View into a non-nullable column of integral indices that maps the
  * rows in the source columns to rows in the destination columns.
- * @param[in] check_bounds Optionally perform bounds checking on the values
- * of `gather_map` and throw an error if any of its values are out of bounds.
+ * @param[in] bounds_policy Policy to apply to account for possible out-of-bounds indices
+ * `DONT_CHECK` skips all bounds checking for gather map values. `NULLIFY` coerces rows that
+ * corresponds to out-of-bounds indices in the gather map to be null elements. Callers should
+ * use `DONT_CHECK` when they are certain that the gather_map contains only valid indices for
+ * better performance. If `policy` is set to `DONT_CHECK` and there are out-of-bounds indices
+ * in the gather map, the behavior is undefined. Defaults to `DONT_CHECK`.
  * @param[in] mr Device memory resource used to allocate the returned table's device memory
  * @return std::unique_ptr<table> Result of the gather
  */
 std::unique_ptr<table> gather(
   table_view const& source_table,
   column_view const& gather_map,
-  bool check_bounds                   = false,
+  out_of_bounds_policy bounds_policy  = out_of_bounds_policy::DONT_CHECK,
   rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());
 
 /**

cpp/include/cudf/detail/copy_if.cuh

@@ -38,6 +38,7 @@
 #include <rmm/device_uvector.hpp>
 
 #include <cub/cub.cuh>
+#include "cudf/copying.hpp"
 
 #include <algorithm>
 
@@ -268,8 +269,12 @@ struct scatter_gather_functor {
                     indices.begin(),
                     filter);
 
-    auto output_table = cudf::detail::gather(
-      cudf::table_view{{input}}, indices.begin(), indices.end(), false, stream, mr);
+    auto output_table = cudf::detail::gather(cudf::table_view{{input}},
+                                             indices.begin(),
+                                             indices.end(),
+                                             cudf::out_of_bounds_policy::DONT_CHECK,
+                                             mr,
+                                             stream);
 
     // There will be only one column
     return std::make_unique<cudf::column>(std::move(output_table->get_column(0)));

cpp/include/cudf/detail/gather.cuh

@@ -610,7 +610,12 @@ struct column_gatherer_impl<struct_view, MapItRoot> {
  * the source columns to rows in the destination columns
  * @param[in] gather_map_end End of iterator range of integer indices that map the rows in the
  * source columns to rows in the destination columns
- * @param[in] nullify_out_of_bounds Nullify values in `gather_map` that are out of bounds.
+ * @param[in] bounds_policy Policy to apply to account for possible out-of-bound indices
+ * `DONT_CHECK` skips all bound checking for gather map values. `NULLIFY` coerces rows that
+ * corresponds to out-of-bound indices in the gather map to be null elements. Callers should
+ * use `DONT_CHECK` when they are certain that the gather_map contains only valid indices for
+ * better performance. In case there are out-of-bound indices in the gather map, the behavior
+ * is undefined. Defaults to `NULLIFY`.
  * @param[in] mr Device memory resource used to allocate the returned table's device memory
  * @param[in] stream CUDA stream used for device memory operations and kernel launches.
  * @return cudf::table Result of the gather
@@ -620,29 +625,32 @@ std::unique_ptr<table> gather(
   table_view const& source_table,
   MapIterator gather_map_begin,
   MapIterator gather_map_end,
-  bool nullify_out_of_bounds          = false,
-  rmm::cuda_stream_view stream        = rmm::cuda_stream_default,
-  rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource())
+  out_of_bounds_policy bounds_policy  = out_of_bounds_policy::DONT_CHECK,
+  rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource(),
+  cudaStream_t stream                 = 0)
 {
   std::vector<std::unique_ptr<column>> destination_columns;
 
   // TODO: Could be beneficial to use streams internally here
 
   for (auto const& source_column : source_table) {
     // The data gather for n columns will be put on the first n streams
-    destination_columns.push_back(cudf::type_dispatcher(source_column.type(),
-                                                        column_gatherer{},
-                                                        source_column,
-                                                        gather_map_begin,
-                                                        gather_map_end,
-                                                        nullify_out_of_bounds,
-                                                        stream,
-                                                        mr));
+    destination_columns.push_back(
+      cudf::type_dispatcher(source_column.type(),
+                            column_gatherer{},
+                            source_column,
+                            gather_map_begin,
+                            gather_map_end,
+                            bounds_policy == out_of_bounds_policy::NULLIFY,
+                            stream,
+                            mr));
   }
 
-  auto const op =
-    nullify_out_of_bounds ? gather_bitmask_op::NULLIFY : gather_bitmask_op::DONT_CHECK;
-  gather_bitmask(source_table, gather_map_begin, destination_columns, op, stream, mr);
+  gather_bitmask_op const op = bounds_policy == out_of_bounds_policy::NULLIFY
+                                 ? gather_bitmask_op::NULLIFY
+                                 : gather_bitmask_op::DONT_CHECK;
+
+  gather_bitmask(source_table, gather_map_begin, destination_columns, op, mr, stream);
 
   return std::make_unique<table>(std::move(destination_columns));
 }

cpp/include/cudf/detail/gather.hpp

@@ -18,16 +18,16 @@
 #include <cudf/column/column_view.hpp>
 #include <cudf/table/table_view.hpp>
 
+#include <cudf/copying.hpp>
 #include <cudf/table/table.hpp>
 
 #include <rmm/cuda_stream_view.hpp>
 
 #include <memory>
 
 namespace cudf {
-namespace detail {
 
-enum class out_of_bounds_policy : int8_t { FAIL, NULLIFY, IGNORE };
+namespace detail {
 
 enum class negative_index_policy : bool { ALLOWED, NOT_ALLOWED };
 
@@ -49,11 +49,11 @@ enum class negative_index_policy : bool { ALLOWED, NOT_ALLOWED };
  * @param[in] source_table The input columns whose rows will be gathered
  * @param[in] gather_map View into a non-nullable column of integral indices that maps the
  * rows in the source columns to rows in the destination columns.
- * @param[in] out_of_bounds_policy How to treat out of bounds indices. FAIL: check `gather_map`
- * values and throw an exception if any are out of bounds. `NULLIFY` means to nullify output values
- * corresponding to out-of-bounds gather_map values. `IGNORE` means to ignore values in
- * `gather_map` that are out of bounds. `IGNORE` is incompatible with `negative_index_policy ==
- * ALLOW`.
+ * @param[in] bounds_policy How to treat out-of-bounds indices. `NULLIFY` coerces rows that
+ * correspond to out-of-bounds indices in the gather map to be null elements. For better
+ * performance, use `DONT_CHECK` when the `gather_map` is known to contain only valid
+ * indices. If `policy` is set to `DONT_CHECK` and there are out-of-bounds indices in `gather_map`,
+ * the behavior is undefined.
  * @param[in] negative_index_policy Interpret each negative index `i` in the
  * gathermap as the positive index `i+num_source_rows`.
  * @param[in] mr Device memory resource used to allocate the returned table's device memory
@@ -63,7 +63,7 @@ enum class negative_index_policy : bool { ALLOWED, NOT_ALLOWED };
 std::unique_ptr<table> gather(
   table_view const& source_table,
   column_view const& gather_map,
-  out_of_bounds_policy bounds,
+  out_of_bounds_policy bounds_policy,
   negative_index_policy neg_indices,
   rmm::cuda_stream_view stream        = rmm::cuda_stream_default,
   rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());