-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-40062: [C++] Move RecordBatch::ToTensor code from record_batch.cc to tensor.cc #41932
GH-40062: [C++] Move RecordBatch::ToTensor code from record_batch.cc to tensor.cc #41932
Conversation
I have run the benchmarks locally just in case, everything is looking OK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
@felipecrv mind having a quick look? This PR only moves the existing code from record_batch.cc to tensor.cc and changes the signature of the main method from |
I will merge this so I can make #41870 easier to review. We can move code reviews to the linked PR. |
…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 <frim.alenka@gmail.com> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 5a86441. 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. |
Sorry for not looking at this before @AlenkaF. LGTM. |
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 toRecordBatchToTensor
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!