Skip to content

GH-45362: [C++] Fix identity cast for time and list scalar - #45370

Merged
pitrou merged 4 commits into
apache:mainfrom
mapleFU:GH-45362
Feb 5, 2025
Merged

GH-45362: [C++] Fix identity cast for time and list scalar#45370
pitrou merged 4 commits into
apache:mainfrom
mapleFU:GH-45362

Conversation

@mapleFU

@mapleFUmapleFU commented Jan 28, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The PR #40237 introduced code:

// time to time
template <typename To, typename From, typename T = typename To::TypeClass>

However, the Time64Type::TypeClass doesn't exist, so SFINAE always failed.

Are these changes tested?

Yes

Are there any user-facing changes?

No.

@github-actions

Copy link
Copy Markdown

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

@mapleFU

Copy link
Copy Markdown
MemberAuthor

Windows CI failed seems to be related.

@mapleFU
mapleFUforce-pushed the GH-45362 branch 2 times, most recently from a615d7c to 22134a8CompareJanuary 29, 2025 13:42
@mapleFU

Copy link
Copy Markdown
MemberAuthor

Also cc @pitrou since this is new year and @zanmato1984 might not have time for this

@pitroupitrou changed the title GH-45362: [C++] Fix Identity cast for time scalarGH-45362: [C++] Fix identity cast for time scalarFeb 4, 2025
Comment threadcpp/src/arrow/scalar_test.cc
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Feb 4, 2025
Comment threadcpp/src/arrow/scalar_test.cc Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

decimal seems not have castTo

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we open issues for the missing casts?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added, but seems previous issue wants to deprecate them, I think these issues may also be mark as not planned, lol

Comment threadcpp/src/arrow/scalar_test.cc Outdated
Comment on lines 182 to 183

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

struct and map seems not supports castTo non string types

@mapleFUmapleFU changed the title GH-45362: [C++] Fix identity cast for time scalarGH-45362: [C++] Fix identity cast for time and list scalarFeb 5, 2025
@mapleFU
mapleFU requested a review from pitrouFebruary 5, 2025 12:18

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the update @mapleFU !

Comment threadcpp/src/arrow/scalar_test.cc Outdated
Comment threadcpp/src/arrow/scalar_test.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we open issues for the missing casts?

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the update @mapleFU ! LGTM.

@pitrou
pitrou merged commit d9e7fa6 into apache:mainFeb 5, 2025
@pitroupitrou removed the awaiting committer review Awaiting committer review label Feb 5, 2025
@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Feb 5, 2025
@mapleFU
mapleFU deleted the GH-45362 branch February 5, 2025 14:04
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit d9e7fa6.

There were 8 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto 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.

2 participants

@mapleFU@pitrou