GH-40062: [C++][Python] Conversion of Table to Arrow Tensor #41870
+824
−79
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Component: C++
Component: Python
awaiting review
|
Benchmarks for |
AlenkaF
added a commit
that referenced
this pull request
Jun 5, 2024
…to tensor.cc (#41932) ### Rationale for this change This is a precursor PR to #41870 with the purpose to make the review of #41870 easier (the diff of the code will be visible as it currently isn't because the code was moved to table.cc. I should also live in tensor.cc). ### What changes are included in this PR? The code from `RecordBatch::ToTensor` in record_batch.cc is moved to `RecordBatchToTensor` in tensor.cc. ### Are these changes tested? Existing tests should pass. ### Are there any user-facing changes? No. **This PR does not close the linked issue yet, it is just a precursor!** * GitHub Issue: #40062 Authored-by: AlenkaF <[email protected]> Signed-off-by: AlenkaF <[email protected]>
AlenkaF
force-pushed
the
gh-40062-table-to-tensor
branch
from
13c49a7 to
15574f8
Compare
vibhatha
pushed a commit
to vibhatha/arrow
that referenced
this pull request
Jun 5, 2024
…ch.cc to tensor.cc (apache#41932) ### Rationale for this change This is a precursor PR to apache#41870 with the purpose to make the review of apache#41870 easier (the diff of the code will be visible as it currently isn't because the code was moved to table.cc. I should also live in tensor.cc). ### What changes are included in this PR? The code from `RecordBatch::ToTensor` in record_batch.cc is moved to `RecordBatchToTensor` in tensor.cc. ### Are these changes tested? Existing tests should pass. ### Are there any user-facing changes? No. **This PR does not close the linked issue yet, it is just a precursor!** * GitHub Issue: apache#40062 Authored-by: AlenkaF <[email protected]> Signed-off-by: AlenkaF <[email protected]>
| @@ -1219,6 +1219,295 @@ def test_recordbatch_to_tensor_unsupported(): | |||
| batch.to_tensor() | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize('typ', [ | |||
There was a problem hiding this comment.
We are adding a lot of code to this test file, while in practice it's not really testing anything new. I am wondering if we could either parametrize some existing tests for RecordBatch to run with both RecordBatch and Table (we have some other tests in this file that are parametrized that way, see eg test_table_basics), or otherwise only test some unique aspects of Table.to_tensor (the fact that the method exists and works, a case with multiple chunks), but for the other aspects (type promotion, null handling, etc) rely on the record batch tests.
There was a problem hiding this comment.
Added parametrization with RecordBatch and Table for mixed type test, others I left to rely on RecordBatch only. Then I kept one test for table with multiple chunks (test_table_to_tensor_uniform_type): feab6e1
github-actions
bot
added
awaiting changes
…rk for Arrow Tables
AlenkaF
force-pushed
the
gh-40062-table-to-tensor
branch
from
15574f8 to
4decf7f
Compare
github-actions
bot
added
awaiting review
|
I have researched the benchmark regression a bit and found that:
benchmark diff outputPlan to also try profiling in python ( |
…st for both batch and table
github-actions
bot
added
awaiting committer review
github-actions
bot
added
awaiting changes
Do you see those regressions of up to 40% for both row major and column major conversions? And both for uniform vs mixed type with casting? |
Benchmarks for RecordBatch only test row-major conversion. The newly added Table benchmarks test both. I think that was due to the fact we were adding features for RecordBatch::ToTensor step by step and we needed one simple benchmark that we could check while adding the features. Row-major conversion was the last to be added. As for the types, we only test uniform types in C++ benchmarks at the moment. ps: haven't been able to find extract any information with neither py-spy nor cProfile. |
github-actions
bot
added
awaiting change review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
There is currently no method to convert Arrow Table to Arrow Tensor (conversion from columnar format to a contiguous block of memory). This work is a continuation of
RecordBatch::ToTensorwork, see #40058.What changes are included in this PR?
This PR:
Table::ToTensorconversionRecordBatch::ToTensorand uses the Table implementation (RecordBatch::ToTensorbenchmarks checked)Are these changes tested?
Yes, in C++ and Python.
Are there any user-facing changes?
No, it is a new feature.