[FEA] ANSI SQL Operator JIT Support (3) : Refactor operator library - #22836
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
919caa4 to
c738c00
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors operator helpers (bitwise, casts, ANSI arithmetic, logic, null handling), migrates internal error enum to ChangesOperator Library and Transform Kernel Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/transform/jit/kernel.cu`:
- Around line 49-50: The loop body in the operation lambda calls __ballot_sync
with a hard-coded full mask while threads that meet row >= row_size may have
exited the loop, causing undefined behavior; replace the hard-coded mask usage
in the null-aware path (where __ballot_sync is invoked) with an active-thread
mask (e.g., use __activemask() or compute a mask from the per-lane predicate) so
only participating lanes are included, and ensure any subsequent warp-wide sync
uses the same mask; update references around the operation lambda and the
null-aware ballot call to use that computed mask instead of 0xFFFF'FFFFU.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 266f14c1-7ea0-4d98-8c68-ab2ca7f21bf0
📒 Files selected for processing (10)
cpp/include/cudf/ast/detail/operator_functor.cuhcpp/include/cudf/detail/operators/ansi_arithmetic.cuhcpp/include/cudf/detail/operators/bitwise.cuhcpp/include/cudf/detail/operators/casts.cuhcpp/include/cudf/detail/operators/error.hppcpp/include/cudf/detail/operators/logic.cuhcpp/include/cudf/detail/operators/null_handling.cuhcpp/include/cudf/detail/operators/operators.cuhcpp/include/cudf/errc.hppcpp/src/transform/jit/kernel.cu
💤 Files with no reviewable changes (1)
- cpp/include/cudf/detail/operators/error.hpp
We require a minimum of CUDA 12.9 for builds, is 12.9 supported? If so, this shouldn't be an issue. |
For JIT code, the C++ version & standard library headers are tied to the runtime CUDA version, not the build version |
Ouch. Was this something that passed CI previously? Do we have adequate CI test coverage of this area? |
The existing operators are used and tested by AST; the new operators are used and tested in #22602. |
Co-authored-by: Igor Peshansky <7594381+igorpeshansky@users.noreply.github.com>
….com/lamarrr/cudf into ansi-jit-2--1--operators-refactoring
…nsure proper casting
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cudf/detail/operators/casts.cuh`:
- Around line 234-237: The rescale template currently accepts any signed_integer
Scale but unconditionally narrows new_scale to int32_t when constructing
numeric::scale_type, risking silent truncation; change the template to accept an
int32_t scale (e.g., template <typename R, signed_integer int32_t Scale> or
simply require Scale be int32_t) or, if you must support wider types, add an
explicit range check in rescale (check new_scale fits within int32_t bounds and
handle out-of-range cases by asserting/throwing/logging) before the static_cast
to int32_t when constructing numeric::scale_type for
numeric::decimal<R>::rescaled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df584d1a-c591-4628-9e87-fff92a4ff64d
📒 Files selected for processing (1)
cpp/include/cudf/detail/operators/casts.cuh
igorpeshansky
left a comment
There was a problem hiding this comment.
One remaining question/optional comment, otherwise LGTM
.
….com/lamarrr/cudf into ansi-jit-2--1--operators-refactoring
PointKernel
left a comment
There was a problem hiding this comment.
Looks good. Docs are outdated but not blocking
Fixed ballot sync in transform kernel
|
I believe all concerns have been addressed @PointKernel @igorpeshansky @mhaseeb123 |
|
/merge |
…VIDIA#22836) Follows up NVIDIA#22514. Implements refactoring and bug fixes for the operator library: - renames the `bit_*` operators to `bitwise_*` for consistency with AST operator names - renames `cast_to_b8` to `cast_to_bool8` and other `cast_to_*` functions for consistency with existing AST operator names - renames `ansi_*` arithmetic to use `_overflow` and `ansi_precision_check` to `precision_check` for consistency with CUDF's operators - updates binary operators to use double template types (to make it usable with AST's double dispatch mechanism) - removes invalid `.overflow` checks in the ANSI operators - fixes error returns in ANSI operators (return `std::unexpected`) - removes `cuda::std::convertible_to` concept checks, which are not available in CUDA 12.2 - properly constrains `if_else` operator to avoid ambiguous argument deduction - moves the error code enum to a public header - adds `to_string` function for the error code enum --- to be used in exceptions - refactors the transform kernel - removes `nullify_if` as it is not necessary for a first PoC for ANSI support - Fixes call to `detail::decimal_cast` in `cast_to_decimal_*` functions. Authors: - Basit Ayantunde (https://github.com/lamarrr) Approvers: - Igor Peshansky (https://github.com/igorpeshansky) - Yunsong Wang (https://github.com/PointKernel) URL: NVIDIA#22836
Description
Follows up #22514.
Implements refactoring and bug fixes for the operator library:
bit_*operators tobitwise_*for consistency with AST operator namescast_to_b8tocast_to_bool8and othercast_to_*functions for consistency with existing AST operator namesansi_*arithmetic to use_overflowandansi_precision_checktoprecision_checkfor consistency with CUDF's operators.overflowchecks in the ANSI operatorsstd::unexpected)cuda::std::convertible_toconcept checks, which are not available in CUDA 12.2if_elseoperator to avoid ambiguous argument deductionto_stringfunction for the error code enum --- to be used in exceptionsnullify_ifas it is not necessary for a first PoC for ANSI supportdetail::decimal_castincast_to_decimal_*functions.Checklist