Skip to content

GH-47390: [C++][Acero] Allow for any type of scalar in Pivot longer features - #47391

Merged
pitrou merged 5 commits into
apache:mainfrom
gitmodimo:pivotLongerScalarFeatures
Aug 26, 2026
Merged

GH-47390: [C++][Acero] Allow for any type of scalar in Pivot longer features#47391
pitrou merged 5 commits into
apache:mainfrom
gitmodimo:pivotLongerScalarFeatures

Conversation

@gitmodimo

@gitmodimo gitmodimo commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

Rationale for this change

Allow supplying any scalar as feature

What changes are included in this PR?

Support scalar in PivotLonger

Are these changes tested?

Yes

Are there any user-facing changes?

Yes.

This PR includes breaking changes to public APIs.

PivotLongerRowTemplate::feature_values is now a std::vector<std::shared_ptr<Scalar>> while it used to be std::vector<std::string>).
The PivotLongerRowTemplate still allows passing a vector of strings, though.

@gitmodimo
gitmodimo requested a review from westonpace as a code owner August 21, 2025 11:34
@github-actions

Copy link
Copy Markdown

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

@github-actions

Copy link
Copy Markdown

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

@github-actions github-actions Bot added the Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise label Aug 22, 2026
@gitmodimo
gitmodimo force-pushed the pivotLongerScalarFeatures branch from 5198f72 to 324cde6 Compare August 24, 2026 11:02
@gitmodimo

Copy link
Copy Markdown
Contributor Author

@zanmato1984 @westonpace @pitrou What do I need to do to get my PRs reviewed? I am waiting over a year with no response. Bot is now trying to remove what I believe valid contribution.

@github-actions github-actions Bot removed the Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise label Aug 24, 2026
Comment thread cpp/src/arrow/acero/options.h
Comment thread cpp/src/arrow/acero/pivot_longer_node.cc Outdated
Comment thread cpp/src/arrow/acero/pivot_longer_node_test.cc
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 25, 2026
-fix error type
-restore test to use legacy constructor
@gitmodimo
gitmodimo requested a review from pitrou August 25, 2026 10:10
@pitrou

pitrou commented Aug 25, 2026

Copy link
Copy Markdown
Member

@zanmato1984 Do you think this is a reasonable addition?

Comment thread cpp/src/arrow/acero/pivot_longer_node.cc
@zanmato1984

Copy link
Copy Markdown
Contributor

@pitrou Yes, I think the capability itself is reasonable. Feature columns produced by pivot-longer can naturally contain typed values, for example integer week numbers, dates, or timestamps. Forcing them through strings loses type information and requires downstream casts. Keeping the vector<string> constructor also preserves the common existing spelling.

I left one inline comment about handling null scalar pointers safely.

Also, since the public feature_values member type changes, I think the Breaking Change label should be added even though the legacy constructor is retained.

Co-authored-by: Rossi Sun <zanmato1984@gmail.com>
@gitmodimo

Copy link
Copy Markdown
Contributor Author

@zanmato1984 good catch. I believe I have no permissions to modify labels. Can you add it?

@pitrou pitrou added the Breaking Change Includes a breaking change to the API label Aug 26, 2026
@pitrou
pitrou merged commit f8dcfc7 into apache:main Aug 26, 2026
82 of 84 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking Change Includes a breaking change to the API Component: C++

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants