[C++] VisitType() requires the visitor to have in implementation for base DataType

[kszucs]

Describe the enhancement requested

I was trying to use VisitType with a closure using the constexpr type_id based type checkers from type_traits in the following fashion:

  auto handle_type = [&](auto&& type) {
    using Type = std::decay_t<decltype(type)>;
    if constexpr (::arrow::is_boolean(Type::type_id)) {
      ...
    }
    else if constexpr (::arrow::is_primitive(Type::type_id)) {
        ...
    }
    /* else etc. */
  };
  return VisitType(*values.type(), handle_type);

but got compile errors:

/Users/kszucs/Workspace/arrow/cpp/src/parquet/chunker_internal.cc:287:54: error: no member named 'type_id' in 'arrow::DataType'
  287 |       if constexpr (::arrow::is_primitive(ArrowType::type_id)) {
      |                                                      ^
/Users/kszucs/Workspace/arrow/cpp/src/arrow/visit_type_inline.h:88:10: note: in instantiation of function template specialization 'parquet::internal::ContentDefinedChunker::Impl::GetChunks(const int16_t *, const int16_t *, int64_t, const ::arrow::Array &)::(anonymous class)::operator()<const arrow::DataType &>' requested here
   88 |   return std::forward<VISITOR>(visitor)(type, std::forward<ARGS>(args)...);
      |          ^
/Users/kszucs/Workspace/arrow/cpp/src/parquet/chunker_internal.cc:299:21: note: in instantiation of function template specialization 'arrow::VisitType<(lambda at /Users/kszucs/Workspace/arrow/cpp/src/parquet/chunker_internal.cc:285:24) &>' requested here
  299 |     return ::arrow::VisitType(*values.type(), handle_type);
      |                     ^

because of the fallback branch (which is also mentioned in the docstrings):

arrow/cpp/src/arrow/visit_type_inline.h

Lines 69 to 89 in bc0b858

	/// \brief Call `visitor` with the corresponding concrete type class
	/// \tparam ARGS Additional arguments, if any, will be passed to the Visit function after
	/// the `type` argument
	///
	/// Unlike VisitTypeInline which calls `visitor.Visit`, here `visitor`
	/// itself is called.
	/// `visitor` must support a `const DataType&` argument as a fallback,
	/// in addition to concrete type classes.
	///
	/// The intent is for this to be called on a generic lambda
	/// that may internally use `if constexpr` or similar constructs.
	template <typename VISITOR, typename... ARGS>
	inline auto VisitType(const DataType& type, VISITOR&& visitor, ARGS&&... args)
	-> decltype(std::forward<VISITOR>(visitor)(type, args...)) {
	switch (type.id()) {
	ARROW_GENERATE_FOR_ALL_TYPES(TYPE_VISIT_INLINE);
	default:
	break;
	}
	return std::forward<VISITOR>(visitor)(type, std::forward<ARGS>(args)...);
	}

Since the base DataType doesn't define type_id we cannot use this dispatching pattern with the constexpr type checking functions.

Component(s)

C++

[kszucs]

I created a prototype implementation making the default path unreachable. We may consider this change as a breaking change, so maybe we should deprecate VisitType in favor of a newly named function.

On the other hand the requirements making the default path unreachable shouldn't break existing user code since it only depends on the arrow type system. It would only make the usage of VisitType less restrictive by not requiring an implementation for DataType&.