Skip to content

GH-38007: [C++] Add VariableShapeTensor implementation#38008

Merged
rok merged 73 commits intoapache:mainfrom
rok:38007
Feb 24, 2026
Merged

GH-38007: [C++] Add VariableShapeTensor implementation#38008
rok merged 73 commits intoapache:mainfrom
rok:38007

Conversation

@rok
Copy link
Member

@rok rok commented Oct 4, 2023 •
edited
Loading

Rationale for this change

We want to add VariableShapeTensor extension type definition for arrays containing tensors with variable shapes.

What changes are included in this PR?

This adds a C++ implementation.

Are these changes tested?

Yes.

Are there any user-facing changes?

This adds a new extension type C++.

This was referenced Oct 4, 2023
Closed
Merged
@rok rok mentioned this pull request Oct 11, 2023
Open
@rok rok force-pushed the 38007 branch 2 times, most recently from eec16ec to 6287402 Compare October 11, 2023 15:47
@rok rok requested a review from pitrou October 12, 2023 11:14
@rok rok assigned rok and unassigned rok Oct 12, 2023
@rok rok force-pushed the 38007 branch 2 times, most recently from dff5441 to 9c3b464 Compare October 29, 2023 05:12
@AlenkaF AlenkaF added this to the 15.0.0 milestone Nov 16, 2023
@AlenkaF
Copy link
Member

AlenkaF commented Nov 16, 2023

Hi @rok! It would be great to have this in 15.0.0 (added the milestone for visibility). Do you think you have enough bandwidth to work on it at the moment?

@rok
Copy link
Member Author

rok commented Nov 16, 2023

@AlenkaF yes I'd very much like to continue working on this, I was waiting for the release before chasing reviewers :D
I'll rebase tonight and would very much appreciate reviews afterwards.

rok
rok commented Nov 23, 2023
View reviewed changes
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Nov 23, 2023
pitrou
pitrou reviewed Nov 27, 2023
View reviewed changes
@github-actions github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Nov 28, 2023
@rok
Copy link
Member Author

rok commented Feb 24, 2026

@pitrou I addressed the review comments and would like to proceed and merge this tomorrow. Any further comments?

pitrou
pitrou reviewed Feb 24, 2026
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks for the update @rok . A couple more minor comments, otherwise LGTM.

cpp/src/arrow/extension/variable_shape_tensor.h Outdated
Comment on lines 94 to 96
const std::vector<int64_t>& permutation = {},
const std::vector<std::string>& dim_names = {},
const std::vector<std::optional<int64_t>>& uniform_shape = {});
Copy link
Member

@pitrou pitrou Feb 24, 2026

Choose a reason for hiding this comment

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

Why not pass those vectors by value just like in the constructor?

Copy link
Member Author

@rok rok Feb 24, 2026

Choose a reason for hiding this comment

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

Done, 0de39f2

cpp/src/arrow/extension/variable_shape_tensor.h Outdated
Comment on lines 109 to 111
const std::vector<int64_t>& permutation = {},
const std::vector<std::string>& dim_names = {},
const std::vector<std::optional<int64_t>>& uniform_shape = {});
Copy link
Member

@pitrou pitrou Feb 24, 2026

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

@rok rok Feb 24, 2026

Choose a reason for hiding this comment

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

Same as above 0de39f2

cpp/src/arrow/extension/tensor_internal.h

#include "arrow/status.h"
#include "arrow/util/print_internal.h"
#include <span>
Copy link
Member

@pitrou pitrou Feb 24, 2026

Choose a reason for hiding this comment

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

Please move these with the other stdlib includes above?

Copy link
Member Author

@rok rok Feb 24, 2026

Choose a reason for hiding this comment

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

Done.

@pitrou
Copy link
Member

pitrou commented Feb 24, 2026

By the way, do you want to expose it in Python soon after? That would allow validating against NumPy.

@rok
Copy link
Member Author

rok commented Feb 24, 2026

By the way, do you want to expose it in Python soon after? That would allow validating against NumPy.

Yes, I'm jumping on #40354 right after we merge here :)

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Feb 24, 2026
@rok
Copy link
Member Author

rok commented Feb 24, 2026

Oh wait, rebases are cheap now #40354

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 24, 2026
@rok rok merged commit 2fcc3ec into apache:main Feb 24, 2026
53 of 57 checks passed
@rok rok deleted the 38007 branch February 24, 2026 18:42
@rok rok removed the awaiting change review Awaiting change review label Feb 24, 2026
@rok
Copy link
Member Author

rok commented Feb 24, 2026

Thank you for all the reviews @pitrou & others!

@pitrou
Copy link
Member

pitrou commented Feb 24, 2026

Thanks for pushing this through @rok ! It's a good thing we finally have an implementation 🎉

@conbench-apache-arrow
Copy link

conbench-apache-arrow bot commented Feb 25, 2026

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

There were no benchmark performance regressions. 🎉

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

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

Reviewers

@AlenkaF AlenkaF AlenkaF left review comments

@pitrou pitrou pitrou approved these changes

@jorisvandenbossche jorisvandenbossche Awaiting requested review from jorisvandenbossche

@westonpace westonpace Awaiting requested review from westonpace

@raulcd raulcd Awaiting requested review from raulcd raulcd is a code owner

Assignees

No one assigned

Labels

Component: C++ Component: Documentation Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++][Python] Add VariableShapeTensor implementation

6 participants

@rok @AlenkaF @pitrou @pimdh @jorisvandenbossche @raulcd