GH-40062: [C++][Python] Conversion of Table to Arrow Tensor#41870
Conversation
|
Benchmarks for |
…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 <frim.alenka@gmail.com> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
13c49a7 to
15574f8
Compare
jorisvandenbossche
left a comment
There was a problem hiding this comment.
Generally looks good! Some minor comments, and wondering if we can reduce the duplication in testing a bit
15574f8 to
4decf7f
Compare
|
I have researched the benchmark regression a bit and found that:
benchmark diff outputPlan to also try profiling in python ( |
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. |
|
Thank you for your contribution. Unfortunately, this |
Co-authored-by: Rok Mihevc <rok@mihevc.org>
Co-authored-by: Rok Mihevc <rok@mihevc.org>
|
@github-actions crossbow submit wheel-cp314 |
|
Revision: c821847 Submitted crossbow builds: ursacomputing/crossbow @ actions-436d7bdb5c |
|
@ursabot please benchmark |
|
Benchmark runs are scheduled for commit 7d2e68f. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete. |
rok
left a comment
There was a problem hiding this comment.
This looks good now, let's merge!
|
Great to see this land! |
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 7d2e68f. There were 19 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit ca37093. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
|
Yaaay no regressions on the final commit! I've restarted benchmarks for the other two machines just in case, let's see what happens. |
|
Oh, that is great! I wasn't really sure what to do with the first result (#41870 (comment))! 🤞 |
|
Huh, this is interesting. I'm not sure how to interpret but there are changes. Some data types appear to have better results on others worse results. why would this be. |
|
Hm, there has been a change visible for me locally also. But on average the result should be similar. I did the research then but don't remember exactly what was the reason and whatever I write now will be nonsense =) Will do the research again and paste here the likely reason for this. The only connected comment from the PR I found is this one: #41870 (comment) |
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.