[FEA] Add Java bindings for AST JIT operations - #23117
Conversation
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
|
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:
📝 WalkthroughWalkthroughThis PR adds Java and JNI support for libcudf JIT AST expressions, decimal literal serialization, native AST construction, separate default and JIT execution paths, and validation and execution tests. ChangesJIT AST Execution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
java/src/main/java/ai/rapids/cudf/ast/Literal.java (1)
130-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnverified doc claim: legacy
computeColumnsupport for DECIMAL32/64 literals.The javadoc states only
DECIMAL128literals requirecomputeColumnJit, implyingDECIMAL32/DECIMAL64decimal literals work with legacycomputeColumn. No test inCompiledExpressionTestexercises a decimal literal viacomputeColumn(alltestJitDecimalLiteralTransformcases usecomputeColumnJit), so this claim is currently untested here.🤖 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 `@java/src/main/java/ai/rapids/cudf/ast/Literal.java` around lines 130 - 132, The javadoc in Literal is making an unverified claim about decimal literal support with computeColumn versus computeColumnJit. Update the documentation to match the actual behavior for DECIMAL32/DECIMAL64/DECIMAL128 literals, and add or adjust coverage in CompiledExpressionTest (for example around testJitDecimalLiteralTransform and computeColumn) so the supported execution path is explicitly verified.
🤖 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 `@java/src/main/java/ai/rapids/cudf/ast/Literal.java`:
- Around line 130-132: The javadoc in Literal is making an unverified claim
about decimal literal support with computeColumn versus computeColumnJit. Update
the documentation to match the actual behavior for
DECIMAL32/DECIMAL64/DECIMAL128 literals, and add or adjust coverage in
CompiledExpressionTest (for example around testJitDecimalLiteralTransform and
computeColumn) so the supported execution path is explicitly verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61abb19d-7bb0-44a3-991b-d21254a99a70
📒 Files selected for processing (11)
java/src/main/java/ai/rapids/cudf/Cudf.javajava/src/main/java/ai/rapids/cudf/ast/AstExpression.javajava/src/main/java/ai/rapids/cudf/ast/CompiledExpression.javajava/src/main/java/ai/rapids/cudf/ast/JitErrorPolicy.javajava/src/main/java/ai/rapids/cudf/ast/JitOperation.javajava/src/main/java/ai/rapids/cudf/ast/JitOperator.javajava/src/main/java/ai/rapids/cudf/ast/Literal.javajava/src/main/native/src/CompiledExpression.cppjava/src/main/native/src/CudfJni.cppjava/src/main/native/src/jni_compiled_expr.hppjava/src/test/java/ai/rapids/cudf/ast/CompiledExpressionTest.java
…indings-main-upmerge
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
lamarrr
left a comment
There was a problem hiding this comment.
The C++ changes look good to me.
I took a look at the Java changes too, but I believe we'll need more eyes on it.
| auto const op_info = jni_to_jit_operator(jni_ast.read_byte()); | ||
| auto const error_policy = jni_to_jit_error_policy(jni_ast.read_byte()); | ||
| auto const arity = static_cast<int32_t>(jni_ast.read_byte()); | ||
| if (arity < 0) { throw std::invalid_argument("unexpected JNI AST JIT operator arity"); } |
There was a problem hiding this comment.
i think checking the arity comparison below is sufficient
There was a problem hiding this comment.
Agreed. I removed the separate negative-arity check. The single expected-arity comparison rejects both negative and otherwise mismatched values.
|
/ok to test 30cceaf |
Signed-off-by: Haoyang Li <haoyangl@nvidia.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 `@java/src/main/native/src/CompiledExpression.cpp`:
- Around line 560-562: Add benchmark coverage for the JIT branch in the result
computation around compute_column_jit, using representative GPU expressions that
exercise standard AST execution, decimal handling, and overflow-policy behavior.
Follow existing benchmark conventions and retain equivalent coverage for the
non-JIT path where applicable.
🪄 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: fc60ee7f-d81f-4b70-a8af-4d97ba53b4cc
📒 Files selected for processing (13)
java/src/main/java/ai/rapids/cudf/Cudf.javajava/src/main/java/ai/rapids/cudf/ast/AstExpression.javajava/src/main/java/ai/rapids/cudf/ast/AstUtils.javajava/src/main/java/ai/rapids/cudf/ast/BinaryOperator.javajava/src/main/java/ai/rapids/cudf/ast/JitErrorPolicy.javajava/src/main/java/ai/rapids/cudf/ast/JitOperation.javajava/src/main/java/ai/rapids/cudf/ast/JitOperator.javajava/src/main/java/ai/rapids/cudf/ast/Literal.javajava/src/main/java/ai/rapids/cudf/ast/TableReference.javajava/src/main/java/ai/rapids/cudf/ast/UnaryOperator.javajava/src/main/native/src/CompiledExpression.cppjava/src/main/native/src/CudfJni.cppjava/src/test/java/ai/rapids/cudf/ast/CompiledExpressionTest.java
💤 Files with no reviewable changes (2)
- java/src/main/native/src/CudfJni.cpp
- java/src/main/java/ai/rapids/cudf/Cudf.java
🚧 Files skipped from review as they are similar to previous changes (5)
- java/src/main/java/ai/rapids/cudf/ast/JitErrorPolicy.java
- java/src/main/java/ai/rapids/cudf/ast/AstExpression.java
- java/src/main/java/ai/rapids/cudf/ast/Literal.java
- java/src/main/java/ai/rapids/cudf/ast/JitOperator.java
- java/src/main/java/ai/rapids/cudf/ast/JitOperation.java
| auto const& expression = compiled_expr_ptr->get_top_expression(); | ||
| if (backend == execution_backend::DEFAULT) { | ||
| auto const* literal = dynamic_cast<cudf::ast::literal const*>(&expression); | ||
| // The legacy evaluator silently corrupts decimal128 literal outputs in release builds. |
There was a problem hiding this comment.
Would a DECIMAL128 literal nested within an expression also be corrupted, or does that only apply to root literals?
There was a problem hiding this comment.
Good catch. The limitation is not specific to a bare root literal: the legacy evaluator cannot materialize any DECIMAL128 root result. I moved the validation into expression_parser so it uses the inferred root output type, while nested comparisons producing BOOL8 remain supported. This follows the fail-fast behavior introduced for unsupported DECIMAL128 AST operations in #22982.
@lamarrr @davidwendt, could you sanity-check whether the root-output routing is the right place for this validation? The alternative would be to keep this as a separate libcudf follow-up, but the JNI-only literal check was too narrow and allowed explicit IDENTITY expressions to reach the same silent-corruption path.
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
igorpeshansky
left a comment
There was a problem hiding this comment.
LGTM
modulo a tiny consistency nit.
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
|
/ok to test f869b70 |
|
/ok to test 3aeba52 |
|
/merge |
Adds cuDF Java/JNI bindings for the libcudf AST JIT operations introduced by NVIDIA#22602. ### Why cudf-spark needs to lower fused ANSI and TRY expressions to libcudf row IR while preserving row-level overflow and error semantics. JIT-only AST operations also need an explicit Java execution path: routing them through the existing `CompiledExpression.computeColumn` makes execution depend on the process-level `LIBCUDF_JIT_ENABLED` backend selection. ### What changed - Add `JitOperation`, `JitOperator`, and `JitErrorPolicy`, with Java and JNI validation for operator arity, error-policy use, and decimal target scales. - Add `CompiledExpression.computeColumnJit(Table)` to call `cudf::compute_column_jit` directly. - Add `Cudf.initializeJitRuntime()` as an idempotent JIT runtime and cache preflight. It does not change the backend selected by `computeColumn`. - Add decimal AST literals for `DECIMAL32`, `DECIMAL64`, and `DECIMAL128`, including null and range validation. - Cover regular and overflow-checking arithmetic, `PROPAGATE`/`NULLIFY`, shifts, casts, decimal precision and rescaling, conditionals, and nested legacy/JIT expression trees. ### API impact This adds the public `JitOperation`, `JitOperator`, and `JitErrorPolicy` types, `CompiledExpression.computeColumnJit(Table)`, `Cudf.initializeJitRuntime()`, and `Literal.ofDecimal(DType, BigInteger)`. Existing APIs remain source-compatible, and `CompiledExpression.computeColumn` retains its current backend-selection behavior. Expressions containing `JitOperation` must be evaluated with `computeColumnJit` unless the process-level JIT backend is enabled. ### Validation - All 134 `CompiledExpressionTest` cases pass on a local GPU against matching locally built shared libcudf and JNI libraries with `LIBCUDF_JIT_ENABLED=0`. - A clean cudf-spark Spark 3.5.2 `sql-plugin` compile passes against the resulting JNI jar. - Relevant clang-format, codespell, whitespace, and EOF pre-commit checks pass. Authors: - Haoyang Li (https://github.com/thirtiseven) Approvers: - Basit Ayantunde (https://github.com/lamarrr) - Igor Peshansky (https://github.com/igorpeshansky) URL: NVIDIA#23117
Description
Adds cuDF Java/JNI bindings for the libcudf AST JIT operations introduced by #22602.
Why
cudf-spark needs to lower fused ANSI and TRY expressions to libcudf row IR while preserving row-level overflow and error semantics. JIT-only AST operations also need an explicit Java execution path: routing them through the existing
CompiledExpression.computeColumnmakes execution depend on the process-levelLIBCUDF_JIT_ENABLEDbackend selection.What changed
JitOperation,JitOperator, andJitErrorPolicy, with Java and JNI validation for operator arity, error-policy use, and decimal target scales.CompiledExpression.computeColumnJit(Table)to callcudf::compute_column_jitdirectly.Cudf.initializeJitRuntime()as an idempotent JIT runtime and cache preflight. It does not change the backend selected bycomputeColumn.DECIMAL32,DECIMAL64, andDECIMAL128, including null and range validation.PROPAGATE/NULLIFY, shifts, casts, decimal precision and rescaling, conditionals, and nested legacy/JIT expression trees.API impact
This adds the public
JitOperation,JitOperator, andJitErrorPolicytypes,CompiledExpression.computeColumnJit(Table),Cudf.initializeJitRuntime(), andLiteral.ofDecimal(DType, BigInteger). Existing APIs remain source-compatible, andCompiledExpression.computeColumnretains its current backend-selection behavior. Expressions containingJitOperationmust be evaluated withcomputeColumnJitunless the process-level JIT backend is enabled.Validation
CompiledExpressionTestcases pass on a local GPU against matching locally built shared libcudf and JNI libraries withLIBCUDF_JIT_ENABLED=0.sql-plugincompile passes against the resulting JNI jar.Checklist