Skip to content

Make is_supported_cast fail for variable-width inputs #23613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JohnZed wants to merge 3 commits into
base: main
Choose a base branch
from
+9 −3
Open

Make is_supported_cast fail for variable-width inputs #23613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1006f91
Make is_supported_cast fail for variable-width inputs
JohnZed Aug 10, 2026
715e98c
[pre-commit.ci] auto code formatting
pre-commit-ci[bot] Aug 10, 2026
e8ca74d
Fix cast tests header comment
JohnZed Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cpp/src/unary/cast_ops.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* SPDX-FileCopyrightText: Copyright (c) 2019-2026, NVIDIA CORPORATION.
* SPDX-FileCopyrightText: Copyright (c) 2019-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

Expand Down Expand Up @@ -118,7 +118,7 @@ struct fixed_point_unary_cast {
template <typename From, typename To>
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>() &&

@igorpeshansky igorpeshansky Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)…

// Disallow fixed_point here (requires different specialization)
!(cudf::is_fixed_point<From>() || cudf::is_fixed_point<To>()) &&
// Disallow conversions between timestamps and numeric
Expand Down
8 changes: 7 additions & 1 deletion cpp/tests/unary/cast_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION.
* SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

Expand Down Expand Up @@ -201,6 +201,12 @@ inline auto make_data_type()
return cudf::data_type{cudf::type_to_id<T>()};
}

TEST(IsSupportedCast, StringToInt32IsUnsupported)
{
EXPECT_FALSE(cudf::is_supported_cast(cudf::data_type{cudf::type_id::STRING},
cudf::data_type{cudf::type_id::INT32}));
}
Comment on lines +204 to +208

@igorpeshansky igorpeshansky Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

  1. But not EMPTY — this one trips a CUDF_FAIL in type_dispatcher, and since is_supported_cast is noexcept, it'll terminate the entire test binary. Out of scope for this PR, but might be worth a follow-up issue… ↩

Comment on lines +204 to +208

@coderabbitai coderabbitai Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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' cpp

Repository: 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.md

Repository: 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/tests

Repository: 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/benchmarks

Repository: 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/include

Repository: NVIDIA/cudf

Length of output: 6462

Use a BaseFixture and add direct benchmark coverage.

  • Change TEST(IsSupportedCast, ...) to a TEST_F using cudf::test::BaseFixture.
  • Add an NVBench benchmark for cudf::is_supported_cast; the existing intcast benchmark 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


struct CastTimestampsSimple : public cudf::test::BaseFixture {};

TEST_F(CastTimestampsSimple, IsIdempotent)
Expand Down
Loading