GH-40282: [Python] Use C++ type traits

[llama90]

Rationale for this change

Use C++ type traits to avoid redefining in Python

What changes are included in this PR?

Changed all functions that check types in type_traits.h so that they can be used in Python.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

• GitHub Issue: [Python] Use C++ type traits for nested types in types.py #40282

[github-actions]

⚠️ GitHub issue #40282 has been automatically assigned in GitHub to PR creator.

[github-actions]

⚠️ GitHub issue #40282 has been automatically assigned in GitHub to PR creator.

[github-actions]

⚠️ GitHub issue #40282 has been automatically assigned in GitHub to PR creator.

[llama90]

As you can see, not all functions can be replaced by the functionalities present in type_traits.h.

1. There are cases where type_traits.h does not provide functions that check for a specific type only (e.g., int8, int16, int32, int64, etc.).
2. Even when check functions are available, they may cover different types from one another (e.g., temporal types, etc.).

Assuming a draft that makes the functions in type_traits.h accessible from Python has been created in this PR, it would be beneficial to address the mentioned types. Organizing these and handling them through new issues and PRs would be ideal.

I'd appreciate your thoughts on this matter. cc @AlenkaF @jorisvandenbossche @pitrou

[AlenkaF]

Thank you for working on this @llama90 , it will be very helpful!

[github-actions]

⚠️ GitHub issue #40282 has been automatically assigned in GitHub to PR creator.

[pitrou]

These docstrings are not useful if they merely repeat what the function name says. You should explain what a "_binary_like" type is, because the reader probably doesn't know.

[pitrou]

In other words, please use your own judgement when writing docstrings. Don't just write them mechanically.

[llama90]

I have enhanced the content.

[AlenkaF]

It is looking much better, thanks! I think you could remove the first sentence, and maybe the last ("Useful for ..."). The middle portions of the docstrings are quite clear already.

[pitrou]

Even I don't know what a "offset_bit_width type" is supposed to be...

[pitrou]

Hmm... why are we defining two sets of functions exactly? We should have either is_integer or is_integer_type, not both.

[llama90]

I wrote the wrapper for the C++ function to use the is_integer function, thinking someone might use it while working with pyarrow for compatibility. Should we not consider this aspect?

[llama90]

hello. I would appreciate it if you could review it when you have time. thank you! cc @pitrou

[llama90]

I've updated the docstrings based on the review and made adjustments to ensure that existing functions work using the functions in type_traits.h that are already available.

I understand that pyarrow has a wide user base, and to avoid breaking compatibility, I've made sure that functions in the form of is_${type} can still operate as intended. (If I'm mistaken, please let me know.)

Additionally, I plan to create a new issue to address types such as NA, BINARY_VIEW, STRING_VIEW, FIXED_SIZE_LIST, STRUCT, and RUN_END_ENCODED, which have the following considerations:

• NA, BINARY_VIEW, STRING_VIEW require C++ implementation in type_traits.h.
• While there are functions to check FIXED_SIZE_LIST, STRUCT, RUN_END_ENCODED types in type_traits.h, they might not be directly usable.

This requires further consideration, and I intend to proceed with a new issue for it.

[AlenkaF]

Thank you for the updates! Just few more suggestions from me.

Additionally, I plan to create a new issue to address types such as NA, BINARY_VIEW, STRING_VIEW, FIXED_SIZE_LIST, STRUCT, and RUN_END_ENCODED, ...

Sounds great!

[AlenkaF]

It is looking much better, thanks! I think you could remove the first sentence, and maybe the last ("Useful for ..."). The middle portions of the docstrings are quite clear already.

[AlenkaF]

Looking great! (Only added one small suggestion).
Thank you for working on this!

[AlenkaF]

@pitrou mind giving one more look before I merge?

[llama90]

@pitrou Hello. I think the changes match what we wanted. Could you review them when you have time? Thank you.

[danepitkin]

Hi @llama90 , I filed the original issue. Your PR is reimplementing too many things. This issue was originally filed due to this comment: https://github.com/apache/arrow/pull/40265/files#r1505826814

To help clarify, we don't want to reimplement all of the type checks. Instead, we want to investigate if we can replace these python-defined vars

arrow/python/pyarrow/types.py

Lines 29 to 46 in e1de9c5

	_SIGNED_INTEGER_TYPES = {lib.Type_INT8, lib.Type_INT16, lib.Type_INT32,
	lib.Type_INT64}
	_UNSIGNED_INTEGER_TYPES = {lib.Type_UINT8, lib.Type_UINT16, lib.Type_UINT32,
	lib.Type_UINT64}
	_INTEGER_TYPES = _SIGNED_INTEGER_TYPES | _UNSIGNED_INTEGER_TYPES
	_FLOATING_TYPES = {lib.Type_HALF_FLOAT, lib.Type_FLOAT, lib.Type_DOUBLE}
	_DECIMAL_TYPES = {lib.Type_DECIMAL128, lib.Type_DECIMAL256}
	_DATE_TYPES = {lib.Type_DATE32, lib.Type_DATE64}
	_TIME_TYPES = {lib.Type_TIME32, lib.Type_TIME64}
	_INTERVAL_TYPES = {lib.Type_INTERVAL_MONTH_DAY_NANO}
	_TEMPORAL_TYPES = ({lib.Type_TIMESTAMP,
	lib.Type_DURATION} | _TIME_TYPES | _DATE_TYPES |
	_INTERVAL_TYPES)
	_UNION_TYPES = {lib.Type_SPARSE_UNION, lib.Type_DENSE_UNION}
	_NESTED_TYPES = {lib.Type_LIST, lib.Type_FIXED_SIZE_LIST, lib.Type_LARGE_LIST,
	lib.Type_LIST_VIEW, lib.Type_LARGE_LIST_VIEW,
	lib.Type_STRUCT, lib.Type_MAP} | _UNION_TYPES

with checks that might already exist in C++. For example, can we replace this line

arrow/python/pyarrow/types.py

Line 177 in e1de9c5

return t.id in _NESTED_TYPES

with an existing Arrow C++ function that we could expose via cython.

Let me know if that helps.

[AlenkaF]

Thank you for the clarification @danepitkin! I have also misunderstood the issue and guided @llama90 in wrong directions, very sorry about that! I hope you are still willing to finish this and that the clarification from Dane helps?

[llama90]

I haven’t reviewed it in detail yet, but I will consider feedback and proceed accordingly. Thank you!!

[llama90]

From your description and the mention by @pitrou, I understood the following:

There is an is_nested function in C++, perhaps we can use it instead of relying on hand-maintained constants on the Python side? Same for some of the other predicates actually (I see that _INTERVAL_TYPES is only holding a single kind of interval?)

• For the _${TYPE}_TYPES, only replace with C++ validation functions where possible.

From a straightforward view, the cases that can be replaced with C++ functions are as follows:

• ✅ _SIGNED_INTEGER_TYPES = constexpr bool is_signed_integer(Type::type type_id)
• ✅ _UNSIGNED_INTEGER_TYPES = constexpr bool is_unsigned_integer(Type::type type_id)
• ✅ _INTEGER_TYPES = constexpr bool is_integer(Type::type type_id)
• ✅ _FLOATING_TYPES = constexpr bool is_floating(Type::type type_id)
• ✅ _DECIMAL_TYPES = constexpr bool is_decimal(Type::type type_id)
• ✅ _DATE_TYPES = constexpr bool is_date(Type::type type_id)
• ✅ _TIME_TYPES = constexpr bool is_time(Type::type type_id)
• _INTERVAL_TYPES
• _TEMPORAL_TYPES
• ✅ _UNION_TYPES = constexpr bool is_union(Type::type type_id)
• _NESTED_TYPES

In this case, do you intend to only replace the functions marked with ✅? Or are you considering modifying the C++ functions to align with the Python implementation as well?

• For is_interval, the C++ code also checks for INTERVAL_MONTHS and INTERVAL_DAY_TIME.
• For is_temporal, the C++ code does not check for DURATION.
• For is_nested, the C++ code also checks for RUN_END_ENCODED.

is_interval, is_temporal, and is_nested

/// \brief Check for an interval type
///
/// \param[in] type_id the type-id to check
/// \return whether type-id is an interval type one
constexpr bool is_interval(Type::type type_id) {
  switch (type_id) {
    case Type::INTERVAL_MONTHS:
    case Type::INTERVAL_DAY_TIME:
    case Type::INTERVAL_MONTH_DAY_NANO:
      return true;
    default:
      break;
  }
  return false;
}

/// \brief Check for a temporal type
///
/// \param[in] type_id the type-id to check
/// \return whether type-id is a temporal type one
constexpr bool is_temporal(Type::type type_id) {
  switch (type_id) {
    case Type::DATE32:
    case Type::DATE64:
    case Type::TIME32:
    case Type::TIME64:
    case Type::TIMESTAMP:
      return true;
    default:
      break;
  }
  return false;
}

/// \brief Check for a nested type
///
/// \param[in] type_id the type-id to check
/// \return whether type-id is a nested type one
constexpr bool is_nested(Type::type type_id) {
  switch (type_id) {
    case Type::LIST:
    case Type::LARGE_LIST:
    case Type::LIST_VIEW:
    case Type::LARGE_LIST_VIEW:
    case Type::FIXED_SIZE_LIST:
    case Type::MAP:
    case Type::STRUCT:
    case Type::SPARSE_UNION:
    case Type::DENSE_UNION:
    case Type::RUN_END_ENCODED:
      return true;
    default:
      break;
  }
  return false;
}

Moreover, while it is certainly easier to simply apply what you have mentioned, from a long-term perspective, exporting C++ functions to replace equivalent Python code raises questions about the rationale for maintaining them separately. I would like to seek further opinions on this.

[felipecrv]

The predicates around lists and list-views are quite confusing (for backwards compatibility reasons) with some are deprecated. Is it possible to not repeat these on the Python side or we have to keep exactly the same structure that we have in C++?

https://gist.github.com/felipecrv/3c02f3784221d946dec1b031c6d400db

[llama90]

Aha... I see that maintaining both C++ and Python code identically may complicate maintenance.

[llama90]

I have come too far in this PR to make changes, so I have closed it and opened a new one.

• GH-40282: [Python] Use C++ type traits for is_nested function #41709

[felipecrv]

Aha... I see that maintaining both C++ and Python code identically may complicate maintenance.

To make what I posted last night more concrete, an example: the predicate is_list_like can't consider list-views "list-like" because list-views were added after the is_list_like predicate existed. We ended up in a situation where fixed_size_list and map are list-like, but list-views aren't. It's the right thing to do to not break existing code, but is very confusing to newcomers.

We shouldn't re-use the same names on the Python side with different meaning from what we have on the C++ side, but if backwards-compatibility issues aren't the same on the Python side, we could do better.