Make is_supported_cast fail for variable-width inputs - #23613
Conversation
|
pre-commit.ci autofix |
|
/ok to test 52419b5 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe cast support check now requires fixed-width source and target types. A unit test verifies that STRING-to-INT32 casting is unsupported. SPDX notices were updated. ChangesCast support validation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change narrows cast support for variable-width inputs and adds focused test coverage; no actionable merge-blocking risk remains after normal checks and review. 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.
| 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.
We might want to add more assertions here for other non-fixed-width inputs (and rename the test appropriately, e.g., UnsupportedTypes)… The code before your fix would have allowed LIST, STRUCT, and DICTIONARY32 as well1 — we can make sure there are no regressions. Note that this would be a good home for the mirror tests that CodeRabbit requested in #23613 (review) if you choose to add them.
| TEST(IsSupportedCast, StringToInt32IsUnsupported) | |
| { | |
| EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::STRING}, | |
| cudf::data_type{cudf::type_id::INT32})); | |
| } | |
| TEST(IsSupportedCast, UnsupportedTypes) | |
| { | |
| auto const to_int32 = cudf::data_type{cudf::type_id::INT32}; | |
| EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::STRING}, to_int32)); | |
| EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::LIST}, to_int32)); | |
| EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::STRUCT}, to_int32)); | |
| EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::DICTIONARY32}, to_int32)); | |
| } |
Footnotes
-
But not
EMPTY— this one trips aCUDF_FAILintype_dispatcher, and sinceis_supported_castisnoexcept, it'll terminate the entire test binary. Out of scope for this PR, but might be worth a follow-up issue… ↩
| 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)…
4 participants
Description
cudf is_supported_cast returns true for string inputs. But cudf cast documentation says that it 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