Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
15 changes: 15 additions & 0 deletions cpp/src/io/parquet/experimental/variant_extract.cu
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,20 @@ void validate_variant_child(column_view const& child)
std::invalid_argument);
}

struct validate_variant_type_fn {
template <typename T>
void operator()()
requires(is_variant_castable<T>)
{
}
template <typename T>
void operator()()
requires(not is_variant_castable<T>)
{
CUDF_FAIL("unsupported type for variant cast", std::invalid_argument);
}
};

struct cast_variant_fn {
cudf::lists_column_device_view values;
size_type num_rows;
Expand Down Expand Up @@ -886,6 +900,7 @@ std::unique_ptr<column> cast_variant(column_view const& values,
rmm::device_async_resource_ref mr)
{
validate_variant_child(values);
cudf::type_dispatcher(desired_type, validate_variant_type_fn{});

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.

Is there a way we can get rid of the type dispatcher here. It's generally quite expensive and the functor being dispatch seems trivial enough.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, I have refactored it

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.

I think the comment only referred to validate_variant_type_fn, not the entire cast. I'm not sure that we want all casting in a single function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have re-refactored it to address this comment!

size_type const num_rows = values.size();
if (num_rows == 0) { return make_empty_column(desired_type); }

Expand Down
26 changes: 19 additions & 7 deletions cpp/tests/io/experimental/variant_extract_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <cudf/utilities/span.hpp>

#include <array>
#include <bit>
#include <cstdio>
#include <cstring>
#include <memory>
Expand Down Expand Up @@ -1032,11 +1033,9 @@ TEST_F(CastVariantTest, EmptyInput)
}
}

TEST_F(CastVariantTest, CastToUnsupportedTargetThrows)
TEST_F(CastVariantTest, UnsupportedTypeThrows)
{
// cast_variant only supports INT8/16/32/64 and STRING targets. Every other target is rejected at
// compile-time dispatch on the requested output type, independent of the input bytes, so a single
// well-formed placeholder row triggers the same throw for all of them.
// Unsupported target types must throw regardless of whether the input is empty or non-empty.
auto stream = cudf::test::get_default_stream();
std::vector<uint8_t> const val{make_variant_primitive(variant_primitive_type::NULLVAL)};
cudf::test::lists_column_wrapper<uint8_t> values(val.begin(), val.end());
Comment thread
abigalekim marked this conversation as resolved.
Resolved
Expand All @@ -1053,11 +1052,24 @@ TEST_F(CastVariantTest, CastToUnsupportedTargetThrows)
cudf::type_id::DECIMAL64,
cudf::type_id::DECIMAL128};

// Empty input: the early-return path must still validate the type.
auto const empty_values =
cudf::empty_like(cudf::structs_column_view{make_xyz_three_row_variant()}.child(1));
for (auto const& id : ids) {
Comment thread
abigalekim marked this conversation as resolved.
Resolved
EXPECT_THROW(static_cast<void>(cudf::io::parquet::experimental::cast_variant(
*empty_values, cudf::data_type{id}, stream)),
std::invalid_argument)
<< "expected throw for type_id " << static_cast<int>(id) << " on empty input";
Comment thread
abigalekim marked this conversation as resolved.
Resolved
}

// Non-empty input: the dispatch path must also throw for unsupported types.
auto col = make_apache_variant(avf::primitive_int32);

@mhaseeb123 mhaseeb123 Aug 4, 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.

🚁🚁🚁 (iykyk 😄)

auto const value = cudf::structs_column_view{col}.get_sliced_child(1, stream);
for (auto const id : ids) {
SCOPED_TRACE(std::string{"target type_id: "} + std::to_string(static_cast<int32_t>(id)));
EXPECT_THROW(static_cast<void>(cudf::io::parquet::experimental::cast_variant(
values, cudf::data_type{id}, stream)),
std::invalid_argument);
value, cudf::data_type{id}, stream)),
std::invalid_argument)
<< "expected throw for type_id " << static_cast<int>(id) << " on non-empty input";
Comment thread
abigalekim marked this conversation as resolved.
Resolved
}
}

Expand Down
Loading