NVIDIA / NVIDIA/cudf

[FEA] libcudf implementation of "intuitive" decimal division

Open
#22,157 2 comments 0 reactions 0 assignees View on GitHub
feature request libcudf Velox
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

The `libcudf` implementation of decimal division is really fixed-point division, and does not behave intuitively compared with the accepted conventions in (say) the SQL world.

For example, dividing `DECIMAL64(200,-2)` by `DECIMAL64(300,-2)` (which is equivalent to `2.00 / 3.00`) gives a result of `DECIMAL64(0,0)` due to the implementation of `fixed_point<>::operator/()` which simply divides the internal integer values and sets the output scale to the input scale difference.

This is not useful in a an SQL context, where the intuitive result would be something like `DECIMAL64(66,-2)` (i.e. `0.66`).

While the intuitively-correct behavior *is* achievable using standard `libcudf` operators, for example by pre-scaling the numerator to account for the denominator scale, the required operator graph translation is therefore not 1:1.

This has been reported before in this repo, most recently in cuDF issue #17448.

For the Velox (and Presto) GPU projects, it was decided to write custom kernels to perform decimal division, in order to achieve the desired behavior in a single operator, although the use of that custom operator is of course limited to "function mode" (i.e. as an alternative to `cudf::binary_operation(X, Y, cudf::binary_operator::DIV)`) and cannot be used by cudf AST mode. These kernels are in Velox PR 16750.

Additional custom kernels were implemented to support Presto GPU aggregations (most obviously `AVG`) which require a division of a sum value with specific handling of any overflow. These are currently contained in Velox PR 16751.

For various reasons, it is not ideal that custom CUDA kernels exist in the Velox source code repo, and so while they will be tolerated for the initial implementation of GPU Decimal, the long-term expectation is that they be moved to the cuDF repo, perhaps as the start of end-product-specific code. While specific to Velox/Presto at first, it is likely that other SQL (and probably non-SQL) implementations will require the same behavior, so ideally the implementation should be general enough, or able to be generalized later as additional applications arise.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.