Add CAST_TO_DECIMAL32/64/128 support to AST expressions (#22507) - #22519
Open
rpathade wants to merge 4 commits into
Open
Add CAST_TO_DECIMAL32/64/128 support to AST expressions (#22507)#22519rpathade wants to merge 4 commits into
rpathade wants to merge 4 commits into
Conversation
rpathade
force-pushed
the
feat/ast-cast-to-decimal
branch
from
May 15, 2026 06:18
0ab71cb to
6845ed0
Compare
rpathade
marked this pull request as ready for review
May 15, 2026 06:23
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds AST cast support to decimal targets with a new ChangesCast-to-decimal support for AST expressions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cpp/tests/ast/transform_tests.cpp (1)
1394-1410: ⚡ Quick winAdd DECIMAL128 coverage for the new cast node contract.
The PR scope includes DECIMAL128 support and intermediate constraints, but these additions only exercise DECIMAL32/64. Please add:
- a positive test for
cast(int -> DECIMAL128)as output, and- a negative test asserting failure when DECIMAL128 is used where intermediate width is disallowed.
Also applies to: 1412-1431, 1433-1448
🤖 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/tests/ast/transform_tests.cpp` around lines 1394 - 1410, Add two tests mirroring the existing CastIntToDecimal64 pattern but targeting DECIMAL128: create an int32 column (column_wrapper<int32_t>), build a cudf::ast::cast to cudf::data_type{cudf::type_id::DECIMAL128, -2}, call Executor::compute_column and assert the result equals a cudf::test::fixed_point_column_wrapper<int128_t> with values multiplied by 10^2 using CUDF_TEST_EXPECT_COLUMNS_EQUAL; and add a negative test that attempts a cast path which would require a disallowed intermediate width (same pattern referenced in the comment ranges) and assert the call to Executor::compute_column throws (use EXPECT_THROW or ASSERT_THROW) to validate failure for DECIMAL128 intermediate constraints.cpp/src/jit/row_ir.hpp (1)
469-476: 💤 Low valueMissing
@copydocdocumentation on method overrides.Other node types in this file (e.g.,
get_input,set_output,operation,filter_predicate) include@copydoc node::get_id,@copydoc node::get_type, etc. for their method declarations. Consider adding these for consistency.🤖 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.hpp` around lines 469 - 476, Add missing `@copydoc` tags to the override declarations in the class (get_id, get_type, is_null_aware, is_always_valid, instantiate, generate_code) to match other node types; for each method declaration replace or augment its comment with the corresponding `@copydoc` references (e.g., `@copydoc` node::get_id, `@copydoc` node::get_type, `@copydoc` node::is_null_aware, `@copydoc` node::is_always_valid, `@copydoc` node::instantiate, `@copydoc` node::generate_code) so the documentation is consistent with get_input/set_output/operation/filter_predicate.
🤖 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/ast/detail/expression_evaluator.cuh`:
- Around line 242-253: convert_to_rep currently ignores decimal/fixed-point
inputs and assumes the target scale sign; update it to rescale fixed-point
InputT values using both the source and target scales: detect when InputT is a
decimal/fixed-point type, obtain its source scale (e.g., InputT::scale() or the
appropriate accessor), compute an integer scale_delta =
static_cast<int32_t>(source_scale) - static_cast<int32_t>(target_scale), then
multiply the rep by numeric::detail::exp10<RepT>(scale_delta) so the returned
RepT is properly rescaled; keep the existing paths for floating/integral using
numeric::detail::exp10 but replace the decimal fall-through
(static_cast<RepT>(val)) with this rescaling logic and ensure you use
numeric::scale_type for scale variables.
In `@cpp/src/jit/row_ir.cpp`:
- Around line 445-452: The else-branch generating "static_cast<...>(...)" fails
when ctx.has_nulls() makes target_type_name and operand_ types
cuda::std::optional<T>/U because cross-type static_cast isn't supported; fix by
detecting nullable non-decimal casts and rejecting them with CUDF_FAIL (or an
equivalent error) instead of emitting a static_cast. Concretely, in the branch
handling non-decimal casts (around target_type_name, operand_, id_), add a check
for ctx.has_nulls() and call CUDF_FAIL with a clear message rejecting
non-decimal nullable casts; alternatively, if you want to support them,
implement explicit nullable handling mirroring the decimal path: create an
optional<T> result, set it present only when operand_.has_value(), cast
operand_.value() to T, and assign into id_.
In `@cpp/tests/ast/transform_tests.cpp`:
- Around line 1394-1410: The CastIntToDecimal64 test (TYPED_TEST
TransformTest::CastIntToDecimal64) only checks a dense happy path; add edge-case
variants that exercise null propagation, empty/sliced inputs, and multi-block
boundary behavior for both executors by creating additional test cases that
reuse column_wrapper<int32_t>, cudf::ast::cast and Executor::compute_column: (1)
a column with nulls and corresponding expected
fixed_point_column_wrapper<int64_t> with matching null mask to verify nulls
propagate; (2) an empty/sliced input (use a sliced column view of c_0) to verify
empty/slice handling; and (3) a large multi-block-sized column (length > GPU
block threshold) to validate block-boundary correctness—duplicate these checks
for the other cast tests referenced (lines ~1412-1448) so both executors and all
cast scenarios cover nulls, empty/slice, and multi-block cases.
---
Nitpick comments:
In `@cpp/src/jit/row_ir.hpp`:
- Around line 469-476: Add missing `@copydoc` tags to the override declarations in
the class (get_id, get_type, is_null_aware, is_always_valid, instantiate,
generate_code) to match other node types; for each method declaration replace or
augment its comment with the corresponding `@copydoc` references (e.g., `@copydoc`
node::get_id, `@copydoc` node::get_type, `@copydoc` node::is_null_aware, `@copydoc`
node::is_always_valid, `@copydoc` node::instantiate, `@copydoc` node::generate_code)
so the documentation is consistent with
get_input/set_output/operation/filter_predicate.
In `@cpp/tests/ast/transform_tests.cpp`:
- Around line 1394-1410: Add two tests mirroring the existing CastIntToDecimal64
pattern but targeting DECIMAL128: create an int32 column
(column_wrapper<int32_t>), build a cudf::ast::cast to
cudf::data_type{cudf::type_id::DECIMAL128, -2}, call Executor::compute_column
and assert the result equals a cudf::test::fixed_point_column_wrapper<int128_t>
with values multiplied by 10^2 using CUDF_TEST_EXPECT_COLUMNS_EQUAL; and add a
negative test that attempts a cast path which would require a disallowed
intermediate width (same pattern referenced in the comment ranges) and assert
the call to Executor::compute_column throws (use EXPECT_THROW or ASSERT_THROW)
to validate failure for DECIMAL128 intermediate constraints.
🪄 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: 4b82774e-f18e-4586-9223-158dd5ef878c
📒 Files selected for processing (13)
cpp/include/cudf/ast/ast_operator.hppcpp/include/cudf/ast/detail/expression_evaluator.cuhcpp/include/cudf/ast/detail/expression_parser.hppcpp/include/cudf/ast/detail/expression_transformer.hppcpp/include/cudf/ast/detail/operator_functor.cuhcpp/include/cudf/ast/detail/operators.cuhcpp/include/cudf/ast/expressions.hppcpp/src/ast/expression_parser.cppcpp/src/ast/expressions.cppcpp/src/ast/operators.cppcpp/src/jit/row_ir.cppcpp/src/jit/row_ir.hppcpp/tests/ast/transform_tests.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cpp/tests/ast/transform_tests.cpp (1)
1394-1532:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd sliced and multi-block cast cases to complete edge coverage.
Nice expansion of cast tests, but cast-specific sliced-input and boundary/multi-block cases are still missing (Line 1394 onward). This leaves offset-handling and block-boundary behavior unverified for the new cast node across both executors.
As per coding guidelines,
cpp/**/*test*.{cu,cpp}: "Test functions must cover edge cases: empty input, null values, sliced columns, boundary sizes, multi-block sizes".🤖 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/tests/ast/transform_tests.cpp` around lines 1394 - 1532, Add tests that exercise sliced and multi-block behavior for the cast AST node: create new TYPED_TESTs (e.g., CastIntToDecimal64Sliced, CastIntToDecimal64MultiBlock, CastIntToDecimal64BoundarySlice) that follow the existing pattern (use Executor::compute_column, cudf::ast::cast with target decimal types) but construct inputs that exercise offsets and block boundaries—for sliced tests, create a base column (from column_wrapper), take a cudf::column_view slice via cudf::slice or table.column(0).slice(...) and build a table with that sliced view before casting; for multi-block/boundary tests, create larger columns (size > a typical block size or a size that crosses internal chunk boundaries, e.g., several thousand elements) and include null patterns at block edges, then assert expected fixed-point results similar to CastIntToDecimal64, CastIntToDecimal64WithNulls and CastFloatToDecimal64 to verify correct scaling, null propagation, and offset handling.
🤖 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/tests/ast/transform_tests.cpp`:
- Around line 1394-1532: Add tests that exercise sliced and multi-block behavior
for the cast AST node: create new TYPED_TESTs (e.g., CastIntToDecimal64Sliced,
CastIntToDecimal64MultiBlock, CastIntToDecimal64BoundarySlice) that follow the
existing pattern (use Executor::compute_column, cudf::ast::cast with target
decimal types) but construct inputs that exercise offsets and block
boundaries—for sliced tests, create a base column (from column_wrapper), take a
cudf::column_view slice via cudf::slice or table.column(0).slice(...) and build
a table with that sliced view before casting; for multi-block/boundary tests,
create larger columns (size > a typical block size or a size that crosses
internal chunk boundaries, e.g., several thousand elements) and include null
patterns at block edges, then assert expected fixed-point results similar to
CastIntToDecimal64, CastIntToDecimal64WithNulls and CastFloatToDecimal64 to
verify correct scaling, null propagation, and offset handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0a0977f8-3ec0-46aa-8c75-7c998d65673b
📒 Files selected for processing (2)
cpp/src/jit/row_ir.cppcpp/tests/ast/transform_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/jit/row_ir.cpp
- Resolve merge conflicts in operator_functor.cuh, row_ir.cpp, row_ir.hpp - Fix JIT decimal cast codegen: emit type-specific conversion code at host time instead of relying on if-constexpr (NVRTC doesn't dead-branch-eliminate) - Fix decimal-to-decimal scale formula: use (target_neg - source_neg) for correct rescaling direction in both interpreted and JIT paths - Fix interpreted decimal-to-decimal cast in expression_evaluator.cuh - Add using declarations for visit(cast) in parquet expression_transformer subclasses to fix partial-override warnings with nvcc - Add using declaration in stats_columns_collector for same reason
Contributor
|
/ok to test ceba82c |
Contributor
|
/ok to test 3f814cf |
davidwendt
reviewed
Jun 29, 2026
Comment on lines
+249
to
+259
| // Decimal-to-decimal: rescale from source scale to target scale | ||
| // val.value() gives the raw rep at source scale; we need rep at target scale | ||
| auto const source_neg_scale = static_cast<int32_t>(-val.scale()); | ||
| auto const combined_exp = neg_scale - source_neg_scale; | ||
| if (combined_exp >= 0) { | ||
| auto const multiplier = numeric::detail::exp10<RepT>(combined_exp); | ||
| return static_cast<RepT>(val.value()) * multiplier; | ||
| } else { | ||
| auto const divisor = numeric::detail::exp10<RepT>(-combined_exp); | ||
| return static_cast<RepT>(val.value()) / divisor; | ||
| } |
Contributor
There was a problem hiding this comment.
Probably could just use the builtin rescale function?
https://github.com/rapidsai/cudf/blob/main/cpp/include/cudf/fixed_point/fixed_point.hpp#L568
davidwendt
reviewed
Jun 29, 2026
| output_object.template set_value<RepT>(output_row_index, result); | ||
| } else { | ||
| IntermediateDataType<has_nulls> tmp{}; | ||
| memcpy(&tmp, &result, sizeof(result)); |
Contributor
There was a problem hiding this comment.
This may not work for decimal128. I believe the IntermediateDataType is not large enough to hold an int128.
You may want to test with some large decimal128 values.
I think fixing this is outside the scope of this PR so you may want to consider only supporting decimal128 for the jit code path.
lamarrr
requested changes
Jul 6, 2026
Comment on lines
+238
to
+274
| struct cast_to_decimal_dispatch { | ||
| cudf::data_type target_type; | ||
|
|
||
| template <typename InputT, typename RepT> | ||
| __device__ inline RepT convert_to_rep(InputT val, numeric::scale_type scale) const | ||
| { | ||
| auto const neg_scale = static_cast<int32_t>(-scale); | ||
| if constexpr (cuda::std::is_floating_point_v<InputT>) { | ||
| auto const multiplier = numeric::detail::exp10<double>(neg_scale); | ||
| return static_cast<RepT>(val * multiplier); | ||
| } else if constexpr (cudf::is_fixed_point<InputT>()) { | ||
| // Decimal-to-decimal: rescale from source scale to target scale | ||
| // val.value() gives the raw rep at source scale; we need rep at target scale | ||
| auto const source_neg_scale = static_cast<int32_t>(-val.scale()); | ||
| auto const combined_exp = neg_scale - source_neg_scale; | ||
| if (combined_exp >= 0) { | ||
| auto const multiplier = numeric::detail::exp10<RepT>(combined_exp); | ||
| return static_cast<RepT>(val.value()) * multiplier; | ||
| } else { | ||
| auto const divisor = numeric::detail::exp10<RepT>(-combined_exp); | ||
| return static_cast<RepT>(val.value()) / divisor; | ||
| } | ||
| } else if constexpr (cuda::std::is_integral_v<InputT>) { | ||
| if (neg_scale >= 0) { | ||
| auto const multiplier = numeric::detail::exp10<RepT>(neg_scale); | ||
| return static_cast<RepT>(val) * multiplier; | ||
| } else { | ||
| // Positive scale means large units; divide to get the rep | ||
| auto const divisor = numeric::detail::exp10<RepT>(-neg_scale); | ||
| return static_cast<RepT>(val) / divisor; | ||
| } | ||
| } else { | ||
| // Unsupported types (timestamps, durations, structs, etc.) - this branch is | ||
| // instantiated by type_dispatcher but never reached at runtime. | ||
| return RepT{}; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This should be part of the cast_to_decimal functions in the operators library: https://github.com/rapidsai/cudf/blob/f142f830ba5ce737dc307725047a3ae96b53a986/cpp/include/cudf/detail/operators/casts.cuh#L198, so they can be re-used as function calls by both the JIT and AST code paths
I think we want operators to have a single arity. If cast_to_decimal needs a separate scale, then it should be in the name, e.g., cast_to_decimal32_scaled.
Comment on lines
+515
to
575
| /** | ||
| * @brief A cast expression that converts the result of a sub-expression to a target data type. | ||
| * | ||
| * Unlike the enum-based cast operators (CAST_TO_INT64, etc.), this expression node can target | ||
| * types that require runtime parameters, such as fixed-point/decimal types where the target | ||
| * scale must be specified. It also subsumes the existing cast operators for convenience. | ||
| */ | ||
| class cast : public expression { | ||
| public: | ||
| /** | ||
| * @brief Construct a new cast expression. | ||
| * | ||
| * @param operand The expression whose result will be cast | ||
| * @param target_type The target data type (carries scale for decimal targets) | ||
| */ | ||
| cast(expression const& operand, cudf::data_type target_type); | ||
|
|
||
| cast(expression&& operand, cudf::data_type target_type) = delete; | ||
|
|
||
| /** | ||
| * @brief Get the target data type. | ||
| * @return The target data type of the cast | ||
| */ | ||
| [[nodiscard]] cudf::data_type get_target_type() const { return target_type_; } | ||
|
|
||
| /** | ||
| * @brief Get the operand expression. | ||
| * @return The operand expression | ||
| */ | ||
| [[nodiscard]] expression const& get_operand() const { return operand_.get(); } | ||
|
|
||
| /** | ||
| * @copydoc expression::accept | ||
| */ | ||
| cudf::size_type accept(detail::expression_parser& visitor) const override; | ||
|
|
||
| /** | ||
| * @copydoc expression::accept | ||
| */ | ||
| std::reference_wrapper<expression const> accept( | ||
| detail::expression_transformer& visitor) const override; | ||
|
|
||
| /** | ||
| * @copydoc expression::accept | ||
| */ | ||
| [[nodiscard]] std::unique_ptr<cudf::detail::row_ir::node> accept( | ||
| cudf::detail::row_ir::ast_converter& visitor) const override; | ||
|
|
||
| [[nodiscard]] bool may_evaluate_null(table_view const& left, | ||
| table_view const& right, | ||
| rmm::cuda_stream_view stream) const override | ||
| { | ||
| return operand_.get().may_evaluate_null(left, right, stream); | ||
| } | ||
|
|
||
| private: | ||
| std::reference_wrapper<expression const> operand_; | ||
| cudf::data_type target_type_; | ||
| }; | ||
|
|
||
| namespace detail { |
Contributor
There was a problem hiding this comment.
Do we need a separate cast expression type?
Comment on lines
+284
to
+335
| cudf::size_type expression_parser::visit(cast const& expr) | ||
| { | ||
| auto const expression_index = _expression_count++; | ||
| // Visit the operand | ||
| auto const operand_index = expr.get_operand().accept(*this); | ||
|
|
||
| // Give back intermediate storage consumed by the operand | ||
| auto const& operand_ref = _data_references[operand_index]; | ||
| if (operand_ref.reference_type == detail::device_data_reference_type::INTERMEDIATE) { | ||
| _intermediate_counter.give(operand_ref.data_index); | ||
| } | ||
|
|
||
| // Determine the operator based on target type | ||
| auto const target_type = expr.get_target_type(); | ||
| ast_operator op; | ||
| switch (target_type.id()) { | ||
| case type_id::DECIMAL32: op = ast_operator::CAST_TO_DECIMAL32; break; | ||
| case type_id::DECIMAL64: op = ast_operator::CAST_TO_DECIMAL64; break; | ||
| case type_id::DECIMAL128: op = ast_operator::CAST_TO_DECIMAL128; break; | ||
| case type_id::INT64: op = ast_operator::CAST_TO_INT64; break; | ||
| case type_id::UINT64: op = ast_operator::CAST_TO_UINT64; break; | ||
| case type_id::FLOAT64: op = ast_operator::CAST_TO_FLOAT64; break; | ||
| default: CUDF_FAIL("Unsupported cast target type."); break; | ||
| } | ||
|
|
||
| _operators.push_back(op); | ||
| _operator_arities.push_back(1); | ||
|
|
||
| // Use the target type directly (including scale for decimals) — bypass ast_operator_return_type | ||
| auto const output = [&]() { | ||
| if (expression_index == 0) { | ||
| return detail::device_data_reference( | ||
| detail::device_data_reference_type::COLUMN, target_type, 0, table_reference::OUTPUT); | ||
| } else { | ||
| if (!cudf::is_fixed_width(target_type)) { | ||
| CUDF_FAIL( | ||
| "The output data type is not a fixed-width type but must be stored in an intermediate."); | ||
| } else if (cudf::size_of(target_type) > (_has_nulls ? sizeof(IntermediateDataType<true>) | ||
| : sizeof(IntermediateDataType<false>))) { | ||
| CUDF_FAIL("The output data type is too large to be stored in an intermediate."); | ||
| } | ||
| return detail::device_data_reference(detail::device_data_reference_type::INTERMEDIATE, | ||
| target_type, | ||
| _intermediate_counter.take()); | ||
| } | ||
| }(); | ||
|
|
||
| auto const index = add_data_reference(output); | ||
| _operator_source_indices.push_back(operand_index); | ||
| _operator_source_indices.push_back(index); | ||
| return index; | ||
| } |
Contributor
There was a problem hiding this comment.
I really think we should adapt this function to the existing AST operator node, and if necessary, add a constructor overload that accepts the scale.
| type_ = data_type{type_id::BOOL8}; | ||
| } break; | ||
| case opcode::CAST_TO_DECIMAL32: { | ||
| type_ = data_type{type_id::DECIMAL32, target_scale_.value_or(0)}; |
Contributor
There was a problem hiding this comment.
the checks here are inconsistent, we should require that the target scale be set or default
Comment on lines
+409
to
+436
| " auto _src_neg_ = static_cast<int32_t>(-_raw_.scale());\n" | ||
| " auto _combined_ = {} - _src_neg_;\n" | ||
| " if (_combined_ >= 0) {{\n" | ||
| " _rep_ = static_cast<_RepT_>(_raw_.value()) * " | ||
| "numeric::detail::exp10<_RepT_>(_combined_);\n" | ||
| " }} else {{\n" | ||
| " _rep_ = static_cast<_RepT_>(_raw_.value()) / " | ||
| "numeric::detail::exp10<_RepT_>(-_combined_);\n" | ||
| " }}\n", | ||
| neg_scale); | ||
| } else if (src_type_id == type_id::FLOAT32 || src_type_id == type_id::FLOAT64) { | ||
| // Float-to-decimal: multiply by 10^neg_scale | ||
| conversion_code = std::format( | ||
| " auto _mult_ = numeric::detail::exp10<double>({});\n" | ||
| " _rep_ = static_cast<_RepT_>(_raw_ * _mult_);\n", | ||
| neg_scale); | ||
| } else { | ||
| // Integer-to-decimal: multiply or divide depending on scale sign | ||
| if (neg_scale >= 0) { | ||
| conversion_code = std::format( | ||
| " auto _mult_ = numeric::detail::exp10<_RepT_>({});\n" | ||
| " _rep_ = static_cast<_RepT_>(_raw_) * _mult_;\n", | ||
| neg_scale); | ||
| } else { | ||
| conversion_code = std::format( | ||
| " auto _div_ = numeric::detail::exp10<_RepT_>({});\n" | ||
| " _rep_ = static_cast<_RepT_>(_raw_) / _div_;\n", | ||
| -neg_scale); |
Contributor
There was a problem hiding this comment.
The design intent of Row IR isn't to inject code but to reuse existing cuDF operators to construct an expression.
The code here should dispatch to a function call and contain no C++/CUDA logic.
It is presently very difficult to read or understand.
Comment on lines
+441
to
+476
| sink.emit( | ||
| std::format("{} {} = [&]() {{\n" | ||
| " auto _val_ = {};\n" | ||
| " if (!_val_.has_value()) return {{}};\n" | ||
| " auto _raw_ = *_val_;\n" | ||
| " using _RepT_ = {};\n" | ||
| " _RepT_ _rep_;\n" | ||
| "{}" | ||
| " return {}{{{}{{numeric::scaled_integer<_RepT_>{{_rep_, " | ||
| "numeric::scale_type{{{}}}}}}}}};\n" | ||
| "}}();\n", | ||
| type, | ||
| id_, | ||
| operand_id, | ||
| rep_type_name, | ||
| conversion_code, | ||
| type, | ||
| decimal_type_name, | ||
| scale)); | ||
| } else { | ||
| sink.emit( | ||
| std::format("{} {} = [&]() {{\n" | ||
| " auto _raw_ = {};\n" | ||
| " using _RepT_ = {};\n" | ||
| " _RepT_ _rep_;\n" | ||
| "{}" | ||
| " return {}{{numeric::scaled_integer<_RepT_>{{_rep_, " | ||
| "numeric::scale_type{{{}}}}}}};\n" | ||
| "}}();\n", | ||
| type, | ||
| id_, | ||
| operand_id, | ||
| rep_type_name, | ||
| conversion_code, | ||
| decimal_type_name, | ||
| scale)); |
Contributor
|
@rpathade do you need help with this PR? |
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
6 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.
This PR introduces a new
cudf::ast::castexpression node (parallel tooperationandliteral) that carries a targetcudf::data_typeincluding scale:Changes
ast_operatorenum: AddedCAST_TO_DECIMAL32,CAST_TO_DECIMAL64,CAST_TO_DECIMAL128.cudf::ast::castclass: New expression node holding operand + targetdata_type(with scale).expression_parser/expression_evaluator): Handles decimal cast dispatch with scaled conversion on GPU.DECIMAL128intermediates are rejected since they exceed the 8-byteIntermediateDataTypelimit.row_ir): Newcast_to_typeIR node that generates CUDA code for scaled fixed-point construction.expression_transformer: Defaultvisit(cast)implementation to avoid breaking existing transformers (e.g. Parquet I/O).TYPED_TESTcases covering int-to-decimal cast and cast-then-compare for both interpreted and JIT paths.Closes [FEA] AST should support
CAST_TO_DECIMAL{32,64,128}(cast-to-fixed-point incompute_column/compute_column_jit) #22507Checklist