Fix the precision when converting a decimal128 column to an arrow array

[jihoonson]

Description

This PR fixes #13749. As discussed in the issue linked, the precision is unnecessarily being limited to 18 when converting decimal128 to arrow.

Implementation-wise, I wasn't sure where is the best place to define the max precision for decimal types. Given that the decimal types don't store the precision in libcudf, I thought it would be better to not expose the max precision to the outside of to-arrow conversion. However, this led to replicating the definition of max precision across multiple places. Appreciate any suggestion.

Finally, it was suggested in #13749 (comment) to add some tests for round tripping. The existing tests look sufficient to me for round trip tests, so I just modified them instead of adding new tests. Please let me know if we need new tests in addition to them.

I'm also not sure whether any documentation should be fixed. Please let me know.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[jihoonson]

Uh, should I create this PR against an other branch than the main?

[davidwendt]

Uh, should I create this PR against an other branch than the main?

The PR should go against branch-23.12

[jihoonson]

Thanks @davidwendt. Working on the rebase and conflict.

[vyasr]

/ok to test

[vyasr]

/ok to test

[vyasr]

@jihoonson thanks for the PR! Style checks are currently failing. Could you please run our pre-commit hooks locally on your system to fix those? If you don't have those set up, the steps are (I'm making no assumptions about your environment, you may want to change how you install pre-commit if you use conda for instance):

cd /path/to/repo
pip install pre-commit
python -m pre-commit install
pre-commit run --all-files

Then commit and push those changes.

Implementation-wise, I wasn't sure where is the best place to define the max precision for decimal types. Given that the decimal types don't store the precision in libcudf, I thought it would be better to not expose the max precision to the outside of to-arrow conversion. However, this led to replicating the definition of max precision across multiple places. Appreciate any suggestion.

That's fine. Since as you say libcudf doesn't have a concept of the precision the only real place it makes sense to include is in the conversion function. I wouldn't sweat the mild duplication.

Finally, it was suggested in #13749 (comment) to add some tests for round tripping. The existing tests look sufficient to me for round trip tests, so I just modified them instead of adding new tests. Please let me know if we need new tests in addition to them.

I agree with your approach, modifying is fine.

I'm also not sure whether any documentation should be fixed. Please let me know.

At the moment this behavior is not specifically documented. What might be nice is to add a note to the to_arrow C++ documentation indicating that decimal types will be converted to the widest precision supported by that type. Perhaps an @note section would be good.

[jihoonson]

Thanks @vyasr and @ttnghia for the review! I addressed your comments in the last 3 commits.

[ttnghia]

/ok to test

[jihoonson]

Thanks @hyperbolic2346 and @ttnghia! What will be the next step? Please let me know if there is anything I need to do 🙂

[ttnghia]

/ok to test

[ttnghia]

/merge

[jihoonson]

Hmm the codecov failed for python/cudf/cudf/io/orc.py, which seems unrelated to my change in this PR.

[ttnghia]

/merge