Skip to content

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

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

GH-48740: [C++] Add missing CTypeTraits for decimal types#50153
Naurder wants to merge 3 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

@Naurder

Naurder commented Jun 12, 2026

Copy link
Copy Markdown
Author

Moved the traits below their respective TypeTraits definitions, added the missing Decimal32/Decimal64, and included the compile-time coverage in type_test.cc.

@pitrou

pitrou commented Jul 6, 2026

Copy link
Copy Markdown
Member

@Naurder Is this ready for another review? Also cc @zanmato1984 who did the first review already.

@Naurder

Naurder commented Aug 6, 2026

Copy link
Copy Markdown
Author

@Naurder Is this ready for another review? Also cc @zanmato1984 who did the first review already.

Yes, the requested code changes are ready for another review.

But, CI is not green yet: 10 jobs are still failing, and I am currently
triaging the logs, starting with the C++ AVX2 job. I do not want to claim
that the PR is merge-ready until I understand those failures.

Thank you kindly for following up.

@Naurder
Naurder force-pushed the GH-48740-ctype-traits branch from 220a94b to 280213b Compare August 7, 2026 08:06
@Naurder

Naurder commented Aug 7, 2026

Copy link
Copy Markdown
Author

@pitrou I've now rebased the branch onto the current main.

I did this because several of the previous CI failures looked unrelated to the decimal CTypeTraits change, and at least some of them were caused by issues that have already been fixed upstream since my last run. Rebasing should give us a cleaner signal from a fresh CI run against the current Arrow codebase.

The new CI run is currently waiting for maintainer approval (11 workflows). Once it is approved and finishes, I'll go through any remaining failures and address anything that is actually related to this PR.

Thanks again for taking another look.

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

4 participants