Skip to content

GH-48740: [C++] Add missing CTypeTraits for decimal types#50153

Open
Naurder wants to merge 2 commits into
apache:mainfrom
Naurder:GH-48740-ctype-traits
Open

GH-48740: [C++] Add missing CTypeTraits for decimal types#50153
Naurder wants to merge 2 commits into
apache:mainfrom
Naurder:GH-48740-ctype-traits

Conversation

@Naurder

@Naurder Naurder commented Jun 10, 2026

Copy link
Copy Markdown

Rationale for this change

As reported in #48740, the CTypeTraits specializations were missing for decimal types. This prevented generic code from correctly mapping C++ decimal types to Arrow types.

What changes are included in this PR?

  • Added CTypeTraits<Decimal128> mapping to Decimal128Type.
  • Added CTypeTraits<Decimal256> mapping to Decimal256Type.

Are these changes tested?

Yes, via existing type traits tests in the CI pipeline.

Are there any user-facing changes?

No.

Closes #48740

@Naurder

Naurder commented Jun 10, 2026

Copy link
Copy Markdown
Author

Hi @HuaHuaY and @zanmato1984,
I picked up this issue and implemented the fix exactly as discussed in the closed PR #48763. Could you please take a look when you have a chance? Thanks!

Comment thread cpp/src/arrow/type_traits.h Outdated
/// \addtogroup type-traits
/// @{
template <>
struct CTypeTraits<Decimal128> {

@HuaHuaY HuaHuaY Jun 11, 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.

I think it should inherit TypeTraits<Decimal128Type> like other specialization.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Thank you very much for suggestion.

@Naurder Naurder requested a review from pitrou as a code owner June 11, 2026 17:15
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 11, 2026
@HuaHuaY

HuaHuaY commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

There are some compile errors. Please fix them. The new CTypeTraits<Decimal128> instantiation should be placed after TypeTraits<Decimal128Type> instantiation.

@zanmato1984 zanmato1984 left a comment

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.

Thanks for updating this. The current version still needs changes: CI is now failing in the C++ build, and it looks caused by the placement of the new traits.

CTypeTraits<Decimal128> and CTypeTraits<Decimal256> are defined before TypeTraits<Decimal128Type> and TypeTraits<Decimal256Type>. Since the new CTypeTraits inherit from those TypeTraits, including arrow/type_traits.h instantiates the primary TypeTraits<T> first; the later explicit specializations are then rejected as “explicit specialization ... after instantiation”.

Could you move the decimal CTypeTraits definitions after the corresponding decimal TypeTraits definitions?

Also, the linked issue is about missing decimal CTypeTraits generally, and Decimal32/64 are still missing. Please add CTypeTraits<Decimal32> and CTypeTraits<Decimal64> as well.

Could you also add lightweight compile-time coverage, for example in type_test.cc, to check all four decimal mappings? Something along these lines should be enough:

static_assert(std::is_same_v<TypeTraits<Decimal32Type>::CType, Decimal32>);
static_assert(std::is_same_v<CTypeTraits<Decimal32>::ArrowType, Decimal32Type>);
// and similarly for Decimal64, Decimal128, Decimal256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] Some CTypeTraits are missing

3 participants