Make is_supported_cast fail for variable-width inputs - #23613
Conversation
|
pre-commit.ci autofix |
|
/ok to test 52419b5 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe cast support check now requires fixed-width source and target types. The cast documentation describes this condition. Tests verify unsupported casts from STRING, LIST, STRUCT, and DICTIONARY32 to INT32. SPDX notices were updated. ChangesCast support validation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🟡 Moderate · up to The PR narrows cast support for variable-width inputs, but the added regression test still does not use the repository-required BaseFixture pattern. That test issue should be fixed or explicitly accepted before merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tests/unary/cast_tests.cpp (1)
204-208: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover variable-width targets too.
The implementation checks both
FromandTo, but this test only checks a variable-width source. Add anINT32toSTRINGassertion so a regression in the target check cannot pass unnoticed.Proposed test extension
+ EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::INT32}, + cudf::data_type{cudf::type_id::STRING}));🤖 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/unary/cast_tests.cpp` around lines 204 - 208, Extend the StringToInt32IsUnsupported test to also assert that an INT32 source to STRING target is unsupported, covering the variable-width target path while preserving the existing assertion.
🤖 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 `@cpp/tests/unary/cast_tests.cpp`:
- Around line 204-208: Extend the StringToInt32IsUnsupported test to also assert
that an INT32 source to STRING target is unsupported, covering the
variable-width target path while preserving the existing assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d85c5012-f405-490c-8203-2bd7700e60d6
📒 Files selected for processing (2)
cpp/src/unary/cast_ops.cucpp/tests/unary/cast_tests.cpp
|
/ok to test 34b1762 |
|
/ok to test 2b79f91 |
2b79f91 to
e8ca74d
Compare
|
/ok to test e8ca74d |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tests/unary/cast_tests.cpp`:
- Around line 204-208: Update the IsSupportedCast.StringToInt32IsUnsupported
test to use TEST_F with a fixture derived from cudf::test::BaseFixture, adding
or reusing an appropriate fixture while preserving the existing assertion.
- Around line 204-208: Update the StringToInt32IsUnsupported test to use TEST_F
with cudf::test::BaseFixture, and add direct NVBench coverage invoking
cudf::is_supported_cast rather than relying on the existing intcast benchmark,
which exercises different APIs.
🪄 Autofix
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: Pro Plus
Run ID: 36b11fdf-974b-4a49-a66e-904f6f76faa7
📒 Files selected for processing (2)
cpp/src/unary/cast_ops.cucpp/tests/unary/cast_tests.cpp
| TEST(IsSupportedCast, StringToInt32IsUnsupported) | ||
| { | ||
| EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::STRING}, | ||
| cudf::data_type{cudf::type_id::INT32})); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a BaseFixture-derived test.
Replace TEST with TEST_F and attach the test to a fixture that inherits from cudf::test::BaseFixture.
Based on learnings: “All libcudf unit tests should make use of a GTest Test Fixture” and “Even if the fixture is empty, it should inherit from the base fixture cudf::test::BaseFixture.”
Proposed fix
-TEST(IsSupportedCast, StringToInt32IsUnsupported)
+class IsSupportedCastTest : public cudf::test::BaseFixture {};
+
+TEST_F(IsSupportedCastTest, StringToInt32IsUnsupported)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST(IsSupportedCast, StringToInt32IsUnsupported) | |
| { | |
| EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::STRING}, | |
| cudf::data_type{cudf::type_id::INT32})); | |
| } | |
| class IsSupportedCastTest : public cudf::test::BaseFixture {}; | |
| TEST_F(IsSupportedCastTest, StringToInt32IsUnsupported) | |
| { | |
| EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::STRING}, | |
| cudf::data_type{cudf::type_id::INT32})); | |
| } |
🤖 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/unary/cast_tests.cpp` around lines 204 - 208, Update the
IsSupportedCast.StringToInt32IsUnsupported test to use TEST_F with a fixture
derived from cudf::test::BaseFixture, adding or reusing an appropriate fixture
while preserving the existing assertion.
Source: Learnings
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'is_supported_cast|BENCHMARK' cppRepository: NVIDIA/cudf
Length of output: 19219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cast test structure and nearby cases ---'
sed -n '1,235p' cpp/tests/unary/cast_tests.cpp
printf '%s\n' '--- cast implementation and declarations ---'
sed -n '120,160p' cpp/include/cudf/unary.hpp
sed -n '420,465p' cpp/src/unary/cast_ops.cu
printf '%s\n' '--- benchmark files with cast-related names ---'
git ls-files cpp/benchmarks | rg -i 'cast|unary'
printf '%s\n' '--- test fixture and benchmark guidance ---'
rg -n -C 3 'BaseFixture|fixture|unit benchmark|benchmark' cpp/tests/unary/cast_tests.cpp cpp/doxygen/developer_guide/TESTING.md cpp/doxygen/developer_guide/BENCHMARKING.md cpp/REVIEW_GUIDELINES.md CONTRIBUTING.mdRepository: NVIDIA/cudf
Length of output: 30777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- existing intcast benchmark ---'
wc -l cpp/benchmarks/string/intcast.cpp
sed -n '1,240p' cpp/benchmarks/string/intcast.cpp
printf '%s\n' '--- support predicate implementation ---'
sed -n '125,175p' cpp/src/unary/cast_ops.cu
printf '%s\n' '--- all is_supported_cast tests and nearby test declarations ---'
rg -n -C 5 'IsSupportedCast|is_supported_cast|TEST(_F)?\(' cpp/testsRepository: NVIDIA/cudf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- existing intcast benchmark ---'
sed -n '1,220p' cpp/benchmarks/string/intcast.cpp
printf '%s\n' '--- support predicate body ---'
rg -n -A 45 -B 8 'constexpr inline auto is_supported_cast' cpp/src/unary/cast_ops.cu
printf '%s\n' '--- exact API references in tests and benchmarks ---'
rg -n 'cudf::is_supported_cast|is_supported_cast<' cpp/tests cpp/benchmarksRepository: NVIDIA/cudf
Length of output: 5337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- non-fixed-point support predicate ---'
sed -n '90,145p' cpp/src/unary/cast_ops.cu
printf '%s\n' '--- test target configuration ---'
rg -n -C 4 'cast_tests|unary' cpp/tests/CMakeLists.txt cpp/tests/unary/CMakeLists.txt 2>/dev/null || true
printf '%s\n' '--- concise support-test references ---'
rg -n 'IsSupportedCast|is_supported_cast' cpp/tests cpp/benchmarks cpp/src cpp/includeRepository: NVIDIA/cudf
Length of output: 6462
Use a BaseFixture and add direct benchmark coverage.
- Change
TEST(IsSupportedCast, ...)to aTEST_Fusingcudf::test::BaseFixture. - Add an NVBench benchmark for
cudf::is_supported_cast; the existingintcastbenchmark measures different APIs.
🤖 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/unary/cast_tests.cpp` around lines 204 - 208, Update the
StringToInt32IsUnsupported test to use TEST_F with cudf::test::BaseFixture, and
add direct NVBench coverage invoking cudf::is_supported_cast rather than relying
on the existing intcast benchmark, which exercises different APIs.
Source: Coding guidelines
igorpeshansky
left a comment
igorpeshansky
left a comment
There was a problem hiding this comment.
A couple of notes on the PR description and suggested tests.
| constexpr inline auto is_supported_non_fixed_point_cast() | ||
| { | ||
| return cudf::is_fixed_width<To>() && | ||
| return cudf::is_fixed_width<From>() && cudf::is_fixed_width<To>() && |
There was a problem hiding this comment.
Note
Your PR description says that you're matching the "cudf cast documentation", but the cast will, in fact, fail for those non-fixed-width types, so you're in fact matching the cast implementation. Might be worth a tweak to the PR description…
[Optional] Might also be worth it to update the is_supported_cast function doc in unary.hpp (lines 129-136)…
There was a problem hiding this comment.
@igorpeshansky - agreed about expanding the test above and took your suggestion there. Changed the PR description too. Thanks!
Not sure I see a change to the header docs for is_supported_cast as necessary here though. I feel like the current description of "Check if a cast between two datatypes is supported." is a good one - nice and concise, and I don't know that it needs to spell out all the possible cast failure paths.
The documentation of cast in unary.hpp was a little funny though - it mentioned failing on variable width output_type, but not variable-width inputs. So I updated that one.
Co-authored-by: Igor Peshansky <7594381+igorpeshansky@users.noreply.github.com>
ce63ee8 to
8914db2
Compare
|
/ok to test 8914db2 |
4 participants
Description
cudf is_supported_cast returns true for string inputs. But cudf cast operator does not support variable-width types.
This leads to an issue in velox (facebookincubator/velox#18450) where the engine thinks it can
use cudf cast to convert varchars to ints, though that will ultimately need an exception. Velox will eventually need to use
a different strings API for this conversion, but at least changing is_supported_cast will allow it to no longer route conversions
through this path.
Checklist