[WIP][FEA] Support ANSI SQL operators in Transform - #22224
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. |
Co-authored-by: Copilot <copilot@github.com>
- Mark `expression` struct with [[nodiscard]] to prevent unintended discards. - Update `operation` constructors and methods for improved clarity and consistency. - Introduce new JIT expressions: `nullify_if`, `coalesce`, `predicate`, and ANSI-compliant arithmetic operations (add, sub, mul, div) with error handling. - Implement tests for JIT expressions including `nullify_if`, `coalesce`, and ANSI arithmetic operations for both integer and decimal types. - Ensure proper handling of overflow scenarios in arithmetic operations. - Refactor null awareness and validity checks in the row IR to improve robustness. - Add additional casting and rescaling expressions with corresponding tests.
revans2
left a comment
There was a problem hiding this comment.
From a quick look through the public APIs it looks good to me, as something we could start using. It will not really be able to support decimal for us. There are a few issues here.
-
Spark knows about precision and scale, but this just knows about scale and bit-width. Precision and bit-width are related, but overflow checks in Spark require precision to work properly. Not width.
-
Spark also rounds the results to the desired output scale that it wants using half even rounding. When I look at rescaled it appears that it is not rounding at all. It is just adjusting the scale at a logical level. If that is true, then we don't have a way to represent the rounding to get to the scale that we need/want.
-
256-bit intermediate values. In order to support all decimal operations we need to be able to do the math at a wider width/precision that 128 bits supports. So with what we have we cannot do decimal operations for all 128 bit values and would have to fall back to our custom kernels for a lot of them.
None of these are blockers fro us to start using the code/testing it out. But it does mean that we will likely only have limited decimal support/testing.
The current functions are intended to preserve the existing CUDF operator semantic.
I believe this is solely to detect overflows? We were able to perform overflow checks without using 256-bit intermediates. We currently don't support 256-bit decimals and I would defer implementing it unless absolutely necessary. |
Yes and no. For add and subtract it is mostly for overflow detection, but not in all cases. DECIMAL(38, 5) + DECIMAL(38, 4). In spark we have a custom kernel to be able to do this because we have to normalize the two to add them, and to do that I would need DECIMAL(39, 5) + DECIMAL(39, 5) as the wider type. Then I would add them to a DECIMAL(39, 5) intermediate and finally cast it back to a DECIMAL(38, 5) with an overflow check. For multiply and divide it is similar the intermediate value could grow up to a 256 bit value internally before it is cast back down to the desired scale and precision. I suppose with knowing the output scale and precision ahead of time you might be able to make the actual intermediate size smaller, but I don't know how you can do half even rounding at the end without at least having a precision of 39 so you can know that other digit to round with. This is not a blocker like I said before. It just limits the range of expressions we can port over to this. We can keep our custom kernels for cases where we need them |
igorpeshansky
left a comment
There was a problem hiding this comment.
Agree with @revans2 — most of these should be usable. Noticed a few more discrepancies besides the ones he pointed out.
Split from #22224 This Pull request: - refactors the ROW-IR codegen setup to prepare for adding ANSI SQL operator support. - begins splitting out the opcode from the AST operator - unifies the `node` type to use opcodes instead of dynamic dispatch - removes ambiguous AST `input` resolution logic - removes the redundant `join_column_accessor` and instead uses a table index attached to each `column_accessor` instead Authors: - Basit Ayantunde (https://github.com/lamarrr) Approvers: - Bradley Dice (https://github.com/bdice) URL: #22511
…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
thirtiseven
left a comment
There was a problem hiding this comment.
Thank you for doing this! I'm working on a POC to integrate it into spark-rapids, and here are some findings from doing so (mostly from AI)
| template <bool has_user_data, typename Args> | ||
| __device__ void execute_transform_op(void* user_data, size_type element_idx, Args args) | ||
| template <ops::error_mode mode, bool has_user_data, typename Args> | ||
| __device__ void execute_transform_op(error_sink* __restrict__ error_sink, |
There was a problem hiding this comment.
In ANY_ROW mode, after a row reports an error, the transform kernel should not continue assigning that row's output. Could execute_transform_op return whether the UDF succeeded, and use that to skip output assignment and validity updates for failed rows?
There was a problem hiding this comment.
that would not be okay; some value types still need to be assigned, as the output column is assumed to be uninitialized. For example, we can't leave a string_view or list_view column uninitialized.
|
|
||
| auto finalized = finalize_outputs(is_null_aware, row_size, std::move(output_columns), stream, mr); | ||
|
|
||
| switch (error_handling_mode) { |
There was a problem hiding this comment.
Could we check the transform error sink immediately after jit_transform::run(...) and before finalize_outputs(...)? Otherwise failed transforms can still finalize outputs before throwing the reported error.
| } | ||
|
|
||
| template <typename T> | ||
| __device__ inline errc if_else(T* out, T const* true_value, T const* false_value, bool const* pred) |
There was a problem hiding this comment.
The nullable if_else overload appears to pass arguments to the non-nullable overload in the wrong order. The callee expects (out, true_value, false_value, pred), but the current call passes (out, pred, true_value, false_value)
| optional<T> const* false_value, | ||
| optional<bool> const* pred) | ||
| { | ||
| if (pred->has_value() && true_value->has_value() && false_value->has_value()) { |
There was a problem hiding this comment.
The nullable if_else overload writes through out->value() before the optional output is engaged, which looks like an implementation bug. Also, should nullable IF_ELSE follow selected-branch null semantics? Requiring both branch inputs to be valid makes IF(true, valid, null) and IF(false, null, valid) null. If strict null propagation is intended here, can we document that so consumers like Spark can avoid this operator for nullable branches?
| .instantiate(is_null_aware, has_user_data, ins, outs); | ||
| .instantiate(error_handling_mode, is_null_aware, has_user_data, ins, outs); | ||
|
|
||
| return jit::get_udf_kernel( |
There was a problem hiding this comment.
We found a stale-but-loadable Jitify file-cache entry that caused cudaErrorLaunchFailure for a transform JIT kernel. Clearing/disabling the cache or using a fresh LIBCUDF_KERNEL_CACHE_PATH fixed it, and copying the stale file into a clean cache reproduced it. This is not recoverable through the current deserialization retry because get_kernel() succeeds and the failure happens at launch. Could the file cache key or directory include a stronger libcudf/JIT-header build identity, or otherwise avoid reusing stale linked-program files across local rebuilds with the same CUDF_VERSION?
Operational avoidance:
- Prefer a build- or deployment-specific
LIBCUDF_KERNEL_CACHE_PATH. - Clear the cache when swapping libcudf builds in place.
- Use
LIBCUDF_KERNEL_CACHE_DISABLED=1only as a debugging or conservative workaround; it avoids stale files but gives up disk-cache benefits.
There was a problem hiding this comment.
The new RTCX library we introduced fixed this headache. It uses the build hash of the headers as a subdirectory in the LIBCUDF_KERNEL_CACHE_PATH directory.
|
Closed. |
Description
This is a POC PR to implement support for ANSI SQL semantics for CUDF JIT Transforms.
New Operators
Checklist