parquet_column_view should use type_dispatcher

[jrhemstad]

Describe the bug

parquet_column_view has an explicit switch over type_ids:

cudf/cpp/src/io/parquet/writer_impl.cu

Line 118 in a537380

switch (col.type().id()) {

This kind of code is extremely problematic and is precisely why the type_dispatcher was developed.

This should be replaced with a type dispatched function object.

[devavret]

This exact thought crossed my mind when I was working on it, so I tried. But the code for each type is so different that it needed a separate template specialization for each type. There's no code reusability between any two types. The primary goal this achieves is to provide a translation for cud::type_id to parquet::Type, which needs to be defined case by case somewhere a. la. type_to_id

As a negative, this reduced the readability of the existing code.

[jrhemstad]

This exact thought crossed my mind when I was working on it, so I tried. But the code for each type is so different that it needed a separate template specialization for each type. There's no code reusability between any two types. The primary goal this achieves is to provide a translation for cud::type_id to parquet::Type, which needs to be defined case by case somewhere a. la. type_to_id

As a negative, this reduced the readability of the existing code.

Using the type_dispatcher ensures that any new types added are automatically picked up instead of easy to forget need to manually update the switch statement.

[devavret]

Using the type_dispatcher ensures that any new types added are automatically picked up instead of easy to forget need to manually update the switch statement.

What good would that be if parquet needs special handling for every type anyway?

[jrhemstad]

Using the type_dispatcher ensures that any new types added are automatically picked up instead of easy to forget need to manually update the switch statement.

What good would that be if parquet needs special handling for every type anyway?

Because the type_dispatcher is the single source of truth for mapping runtime types to compile time types. There should not be exceptions or deviations.

[cwharris]

The design of type_dispatcher encompasses a few things, some of which are not accounted for in the example code. For one, the type_dispatcher pattern encourages a pure-function approach, so setting instance-specific state is discouraged, or it at least encourages a single assignment of a custom type. In that way, type_dispatcher's design has a cascade effect. Using it incorrectly or in a clunky way reveals "anti-patterns" in the calling code.

Here, for example, we are assigning three separate highly coupled values, and this would require, at the very least, a tie at the call site to accomplish using type_dispatcher, but given that we would have already created a tuple to return from type dispatcher, it's likely we would have noticed the correlation and created a custom type, or simply used the tuple as the source of truth, requiring us to change each value at once, reducing the likelihood of erroneous state changes down the line.

cudf/cpp/src/io/parquet/writer_impl.cu

Lines 118 to 123 in a537380

	switch (col.type().id()) {
	case cudf::type_id::INT8:
	_physical_type = Type::INT32;
	_converted_type = ConvertedType::INT_8;
	_stats_dtype = statistics_dtype::dtype_int8;
	break;

Although less of an issue, type_dispatcher also eliminates the possibility of omitting a break and falling through to the next switch, which could save some debugging time.

[cwharris]

That being said, this code works as is and probably shouldn't be changes just for the sake of removing the switch. Replacing it and refactoring the class in to a pure function at the same time would probably be advantageous, but as it stands there's not much to be gained now that it's implemented and tested. The type_dispatcher pattern should still be used for new code, avoiding type-based switches. It's verbose, but encourages better code upstream.

[cwharris]

One more thought on the subject...

The primary goal this achieves is to provide a translation for cud::type_id to parquet::Type, which needs to be defined case by case somewhere a. la. type_to_id

This is actually an argument in favor of type_dispatcher, as it imposes a compile-time restraint on handling all cases, even if one of those cases is a catch all al a default that calls CUDF_FAIL. If there truly is a mapping per type, there won't CUDF_FAIL case, and any time a new type is added, it will need to be accounted for explicitly, reducing the likelihood of future bugs.

[github-actions]

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

[kaatish]

Fixed by #7461