[FEA] ANSI SQL Operator JIT Support (1) : Refactor ROW IR - #22511
Merged
rapids-bot[bot] merged 13 commits intoMay 20, 2026
Conversation
lamarrr
added
feature request
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactor row-expression IR to a single opcode-driven node model, rename internal predicate/filter entities, add table_index to column_accessor, change join filter kernel to pointer-based columns/indices with a single Accessors template, and propagate API/ABI changes through JIT launch, transform/filter plumbing, and tests. ChangesRow-IR, join/filter JIT refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cpp/src/join/filter_join_indices_jit.cu (1)
51-88: 💤 Low valuePotential null optional access for column inputs.
At line 69,
table_sources[i].value()is called without checking if it has a value. For column inputs from the join tables (constructed in lines 413-420),table_sourceswill have values (0 or 1). However, if this function is called with a column input that hasnulloptintable_sources, it will throwstd::bad_optional_access.This appears safe in the current usage since columns from left/right tables always have their table_source set, but consider adding validation or using
value_or()for robustness.🤖 Prompt for 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. In `@cpp/src/join/filter_join_indices_jit.cu` around lines 51 - 88, The code in build_join_filter_template_params uses table_sources[i].value() when instantiating cudf::jit::column_accessor for column_view inputs, which can throw if the optional is nullopt; change this to safely handle missing values by using table_sources[i].value_or(<placeholder>) (e.g., value_or(0)) or by adding an explicit check/assert before using value(), and ensure the same safe handling is applied to the scalar path comment/placeholder logic so no std::bad_optional_access can occur; update the column_accessor instantiation sites (the Template.instantiate calls) to consume the safe value.cpp/src/stream_compaction/filter/filter.cu (1)
69-85: ⚡ Quick winAdd
CUDF_FUNC_RANGE()to this public overload.
filter_extended(...)already does this, but this publiccudf::filter(...)overload now delegates straight intodetail::filter(...)without an NVTX range.
As per coding guidelines,cpp/src/**/*.{cu,cpp}: AddCUDF_FUNC_RANGE()in public functions before delegating to detail:: functions.🤖 Prompt for 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. In `@cpp/src/stream_compaction/filter/filter.cu` around lines 69 - 85, The public overload cudf::filter(...) currently delegates directly to detail::filter(...) without an NVTX range; add a CUDF_FUNC_RANGE() call at the start of the public cudf::filter function (just before constructing args and calling detail::filter) so it mirrors filter_extended(...)’s behavior; ensure CUDF_FUNC_RANGE() is the first statement inside the public function scope and do not change the existing arguments passed to detail::filter (args.udf, args.source_type, args.is_null_aware, args.user_data, args.inputs, filter_table, args.outputs[0].nullability, stream, mr).
🤖 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/jit/row_ir.cpp`:
- Around line 597-605: The code currently wraps expr into ast::detail::predicate
before type-checking, which causes compute_column(...) to always report BOOL8
and bypass validation; first inspect the original expr's type (without wrapping)
and assert it is boolean (compare expr.type().id() or equivalent to
type_id::BOOL8), throwing the same invalid_argument message if not; only after
that validation construct auto filter = ast::detail::predicate{expr} and call
compute_column(target_id, filter, left_table, right_table, function_name,
stream, mr) and keep the existing CUDF_EXPECTS on transform.outputs as a
secondary safeguard.
- Around line 443-457: The null-evaluation flag is computed too broadly: change
the may_evaluate_null logic so nullable inputs only force PRESERVE when there
exists an output that is not ALWAYS_VALID; specifically replace
may_evaluate_null = !output_is_always_valid || has_nullable_inputs with
may_evaluate_null = !output_is_always_valid && has_nullable_inputs so that
outputs where ir->is_always_valid() (e.g., IS_NULL, NULL_EQUAL, PREDICATE)
remain ALL_VALID regardless of instance_.inputs_ nullability, keeping the
subsequent null_policy selection correct.
In `@cpp/src/join/jit/filter_join_kernel.cuh`:
- Around line 21-35: The Doxygen for filter_join_kernel is stale: remove the old
`@param` entries for left_table, right_table, and scalars and replace them with
entries matching the current signature (num_rows, left_indices, right_indices,
columns, predicate_results, user_data); update the brief/description if needed
to reference column_device_view_core via the columns parameter and ensure each
`@param` name exactly matches the function parameters used in template<bool
has_user_data, null_aware is_null_aware, typename Accessors> CUDF_KERNEL void
filter_join_kernel(...).
In `@cpp/src/stream_compaction/filter/filter.cu`:
- Around line 47-58: The call to multi_transform is using
filter_table.num_rows() but the predicate inputs come from a separate table
(predicate_inputs), so ensure the transform uses the predicate side row count:
either pass the converter's row_size through to this call or compute/validate
the predicate row count from predicate_inputs and use that value instead of
filter_table.num_rows(); additionally add a pre-check that every column in
predicate_inputs has the same num_rows (or throw/log an error) before invoking
multi_transform to prevent mismatched extents.
---
Nitpick comments:
In `@cpp/src/join/filter_join_indices_jit.cu`:
- Around line 51-88: The code in build_join_filter_template_params uses
table_sources[i].value() when instantiating cudf::jit::column_accessor for
column_view inputs, which can throw if the optional is nullopt; change this to
safely handle missing values by using table_sources[i].value_or(<placeholder>)
(e.g., value_or(0)) or by adding an explicit check/assert before using value(),
and ensure the same safe handling is applied to the scalar path
comment/placeholder logic so no std::bad_optional_access can occur; update the
column_accessor instantiation sites (the Template.instantiate calls) to consume
the safe value.
In `@cpp/src/stream_compaction/filter/filter.cu`:
- Around line 69-85: The public overload cudf::filter(...) currently delegates
directly to detail::filter(...) without an NVTX range; add a CUDF_FUNC_RANGE()
call at the start of the public cudf::filter function (just before constructing
args and calling detail::filter) so it mirrors filter_extended(...)’s behavior;
ensure CUDF_FUNC_RANGE() is the first statement inside the public function scope
and do not change the existing arguments passed to detail::filter (args.udf,
args.source_type, args.is_null_aware, args.user_data, args.inputs, filter_table,
args.outputs[0].nullability, stream, mr).
🪄 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: 92319eb0-437a-4e3e-bd6c-7ebc620dc925
📒 Files selected for processing (13)
cpp/include/cudf/ast/detail/operator_functor.cuhcpp/include/cudf/ast/expressions.hppcpp/src/ast/expressions.cppcpp/src/jit/column_accessor.cuhcpp/src/jit/join_column_accessor.cuhcpp/src/jit/row_ir.cppcpp/src/jit/row_ir.hppcpp/src/join/filter_join_indices_jit.cucpp/src/join/jit/filter_join_kernel.cucpp/src/join/jit/filter_join_kernel.cuhcpp/src/stream_compaction/filter/filter.cucpp/src/transform/transform.cucpp/tests/jit/row_ir.cpp
💤 Files with no reviewable changes (1)
- cpp/src/jit/join_column_accessor.cuh
3 tasks
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/src/stream_compaction/filter/filter.cu (2)
122-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
CUDF_FUNC_RANGE()in legacy filter function.Same issue as the predicate_expr overload—this public function should have NVTX instrumentation for profiling consistency.
🛠️ Proposed fix
std::vector<std::unique_ptr<column>> filter(std::vector<column_view> const& predicate_columns, std::string const& predicate_udf, std::vector<column_view> const& filter_columns, bool is_ptx, std::optional<void*> user_data, null_aware is_null_aware, output_nullability predicate_nullability, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) { + CUDF_FUNC_RANGE(); // legacy behavior was to detect which column were scalars based on their sizesAs per coding guidelines: "Add CUDF_FUNC_RANGE() in public functions before delegating to detail:: functions".
🤖 Prompt for 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. In `@cpp/src/stream_compaction/filter/filter.cu` around lines 122 - 157, The public overload filter(...) is missing NVTX profiling instrumentation; add a CUDF_FUNC_RANGE() call as the first statement inside the public function filter (the one that builds inputs and then calls detail::filter) so the function is instrumented for profiling before delegating to detail::filter; ensure the call appears before any work (i.e., before building inputs/iterating predicate_columns) to mirror the predicate_expr overload's placement.
73-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
CUDF_FUNC_RANGE()in public function.This public function delegates to
detail::filterbut lacks the NVTX range instrumentation thatfilter_extendedhas at line 109.🛠️ Proposed fix
std::unique_ptr<table> filter(table_view const& predicate_table, ast::expression const& predicate_expr, table_view const& filter_table, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) { + CUDF_FUNC_RANGE(); auto args = cudf::detail::row_ir::ast_converter::filter(cudf::detail::row_ir::target::CUDA,As per coding guidelines: "Add CUDF_FUNC_RANGE() in public functions before delegating to detail:: functions".
🤖 Prompt for 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. In `@cpp/src/stream_compaction/filter/filter.cu` around lines 73 - 96, The public function filter is missing NVTX instrumentation — add a CUDF_FUNC_RANGE() call at the start of the filter(table_view const& predicate_table, ast::expression const& predicate_expr, table_view const& filter_table, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr) function before it builds args and delegates to detail::filter so it matches the instrumentation used by filter_extended; place the CUDF_FUNC_RANGE() as the first statement in that function to wrap the call to cudf::detail::row_ir::ast_converter::filter and subsequent detail::filter invocation.
🤖 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.
Outside diff comments:
In `@cpp/src/stream_compaction/filter/filter.cu`:
- Around line 122-157: The public overload filter(...) is missing NVTX profiling
instrumentation; add a CUDF_FUNC_RANGE() call as the first statement inside the
public function filter (the one that builds inputs and then calls
detail::filter) so the function is instrumented for profiling before delegating
to detail::filter; ensure the call appears before any work (i.e., before
building inputs/iterating predicate_columns) to mirror the predicate_expr
overload's placement.
- Around line 73-96: The public function filter is missing NVTX instrumentation
— add a CUDF_FUNC_RANGE() call at the start of the filter(table_view const&
predicate_table, ast::expression const& predicate_expr, table_view const&
filter_table, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr)
function before it builds args and delegates to detail::filter so it matches the
instrumentation used by filter_extended; place the CUDF_FUNC_RANGE() as the
first statement in that function to wrap the call to
cudf::detail::row_ir::ast_converter::filter and subsequent detail::filter
invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1decddf2-15ea-4cbe-a5b4-2a0299b6e318
📒 Files selected for processing (1)
cpp/src/stream_compaction/filter/filter.cu
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/jit/row_ir.cpp (1)
445-448: ⚡ Quick winUse the returned
output_idinstead of hard-coding output index 0.
instance_.add_output()already returns the index, butoutput_reference{0}ignores it. This is fragile ifgenerate_code()is reused or expanded beyond a single output.♻️ Proposed fix
- [[maybe_unused]] auto output_id = instance_.add_output(); + auto output_id = instance_.add_output(); - output_irs_.emplace_back(std::make_unique<row_ir::node>(output_reference{0}, expr.accept(*this))); + output_irs_.emplace_back( + std::make_unique<row_ir::node>(output_reference{output_id}, expr.accept(*this)));🤖 Prompt for 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. In `@cpp/src/jit/row_ir.cpp` around lines 445 - 448, The code calls instance_.add_output() but ignores its return value by constructing output_reference{0}; update the emplace_back call to use the actual output index returned by output_id so the created row_ir::node references output_reference{output_id} instead of hard-coded 0; locate the call to instance_.add_output(), the local variable output_id, and the output_irs_.emplace_back(std::make_unique<row_ir::node>(output_reference{0}, expr.accept(*this))) and replace the literal with the output_id to make the mapping robust if multiple outputs are added.
🤖 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.
Nitpick comments:
In `@cpp/src/jit/row_ir.cpp`:
- Around line 445-448: The code calls instance_.add_output() but ignores its
return value by constructing output_reference{0}; update the emplace_back call
to use the actual output index returned by output_id so the created row_ir::node
references output_reference{output_id} instead of hard-coded 0; locate the call
to instance_.add_output(), the local variable output_id, and the
output_irs_.emplace_back(std::make_unique<row_ir::node>(output_reference{0},
expr.accept(*this))) and replace the literal with the output_id to make the
mapping robust if multiple outputs are added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0d07a580-0293-4f4f-ad0d-625583cd90c4
📒 Files selected for processing (1)
cpp/src/jit/row_ir.cpp
…rrr/cudf into ansi-jit-1--refactor-row-ir
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cpp/src/join/jit/filter_join_kernel.cuh (1)
22-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
@paramtext for the raw index pointers.These two entries still describe
left_indicesandright_indicesas device spans, but the signature now takes rawcudf::size_type const*pointers. Please keep the docs aligned with the declaration.Suggested doc fix
- * `@param` left_indices Device span of left table indices - * `@param` right_indices Device span of right table indices + * `@param` left_indices Pointer to left table indices + * `@param` right_indices Pointer to right table indices🤖 Prompt for 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. In `@cpp/src/join/jit/filter_join_kernel.cuh` around lines 22 - 23, Update the documentation for the raw index pointer parameters in filter_join_kernel.cuh so they describe device pointers rather than device spans: change the `@param` entries for left_indices and right_indices to indicate they are raw device pointers (cudf::size_type const*), e.g., "Device pointer to left table indices" and "Device pointer to right table indices", and ensure any wording matches the function signature where these parameters are declared.
🤖 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.
Duplicate comments:
In `@cpp/src/join/jit/filter_join_kernel.cuh`:
- Around line 22-23: Update the documentation for the raw index pointer
parameters in filter_join_kernel.cuh so they describe device pointers rather
than device spans: change the `@param` entries for left_indices and right_indices
to indicate they are raw device pointers (cudf::size_type const*), e.g., "Device
pointer to left table indices" and "Device pointer to right table indices", and
ensure any wording matches the function signature where these parameters are
declared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 091aae27-5f3c-4aa5-a8c7-9e5bf4ddbcb9
📒 Files selected for processing (1)
cpp/src/join/jit/filter_join_kernel.cuh
bdice
approved these changes
May 18, 2026
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
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/join/filter_join_indices_jit.cu`:
- Around line 51-58: build_join_filter_template_params hardcodes the kernel
template argument has_user_data to false which can disagree with the PTX
signature built in build_join_filter_kernel when has_user_data is true; update
build_join_filter_template_params to accept/propagate the actual has_user_data
value (rather than hardcoding false) into the template_params (and do the same
for the other overload(s) around lines 91-123), ensuring the kernel template arg
and the PTX signature both use the same has_user_data value referenced by
build_join_filter_kernel.
🪄 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: eadf5c04-d1ea-4792-abdc-76f08101cf67
📒 Files selected for processing (1)
cpp/src/join/filter_join_indices_jit.cu
Contributor
Author
|
/merge |
Contributor
|
This got merged with only a single C++ review approval. |
28 tasks
Merged
3 tasks
3 tasks
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jun 2, 2026
…22514) Split from #22224 Preceded by #22511 Story: #22598 This Pull request: - Implements error codes for row operators - Ports AST's operators to re-usable functions that can be used with JIT codegen - Adds new ANSI-compliant operators: - ANSI_ADD - ANSI_SUB - ANSI_MUL - ANSI_DIV - ANSI_MOD - ANSI_ABS - ANSI_NEG - Adds the `coalesce` operator - Introduces an operator `result` type to allow error returns from operators - Transitioned AST `operator_functors` to use the operator library Authors: - Basit Ayantunde (https://github.com/lamarrr) Approvers: - Lawrence Mitchell (https://github.com/wence-) - Yunsong Wang (https://github.com/PointKernel) URL: #22514
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Split from #22224
This Pull request:
nodetype to use opcodes instead of dynamic dispatchinputresolution logicjoin_column_accessorand instead uses a table index attached to eachcolumn_accessorinsteadChecklist