You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/19 15:36:21 UTC
[GitHub] [arrow] thisisnic opened a new pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
thisisnic opened a new pull request #10100:
URL: https://github.com/apache/arrow/pull/10100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on a change in pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10100:
URL: https://github.com/apache/arrow/pull/10100#discussion_r615966252
##########
File path: cpp/src/arrow/record_batch.cc
##########
@@ -253,6 +253,26 @@ bool RecordBatch::ApproxEquals(const RecordBatch& other) const {
return true;
}
+Result<std::shared_ptr<RecordBatch>> RecordBatch::SelectColumns(const std::vector<int>& indices) const{
+ int n = static_cast<int>(indices.size());
+
+ std::vector<std::shared_ptr<Field>> fields(n);
+ std::vector<std::shared_ptr<Array>> columns(n);
+
+ for (int i = 0; i < n; i++) {
+ int pos = indices[i];
+ if (pos < 0 || pos > num_columns() - 1) {
+ return Status::Invalid("Invalid column index ", pos, " to select columns.");
+ }
+ fields[i] = schema()->field(pos);
+ columns[i] = column(pos);
+ }
+
+ auto schema = std::make_shared<arrow::Schema>(std::move(fields));
Review comment:
Oops, completely missed that - will update now, thanks for highlighting!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10100:
URL: https://github.com/apache/arrow/pull/10100#issuecomment-822603747
https://issues.apache.org/jira/browse/ARROW-11660
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10100:
URL: https://github.com/apache/arrow/pull/10100#issuecomment-824180180
Thanks for the update. I will wait for CI before merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #10100:
URL: https://github.com/apache/arrow/pull/10100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on a change in pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10100:
URL: https://github.com/apache/arrow/pull/10100#discussion_r617659788
##########
File path: cpp/src/arrow/record_batch.cc
##########
@@ -253,6 +253,27 @@ bool RecordBatch::ApproxEquals(const RecordBatch& other) const {
return true;
}
+Result<std::shared_ptr<RecordBatch>> RecordBatch::SelectColumns(
+ const std::vector<int>& indices) const {
+ int n = static_cast<int>(indices.size());
+
+ std::vector<std::shared_ptr<Field>> fields(n);
+ std::vector<std::shared_ptr<Array>> columns(n);
+
+ for (int i = 0; i < n; i++) {
+ int pos = indices[i];
+ if (pos < 0 || pos > num_columns() - 1) {
+ return Status::Invalid("Invalid column index ", pos, " to select columns.");
+ }
+ fields[i] = schema()->field(pos);
+ columns[i] = column(pos);
+ }
+
+ auto new_schema =
+ std::make_shared<arrow::Schema>(std::move(fields), schema()->metadata());
+ return RecordBatch::Make(new_schema, num_rows(), columns);
Review comment:
Thanks - will try to make both of these changes as I'm new to C++ so useful for me to learn!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on a change in pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10100:
URL: https://github.com/apache/arrow/pull/10100#discussion_r617672819
##########
File path: cpp/src/arrow/record_batch.cc
##########
@@ -253,6 +253,27 @@ bool RecordBatch::ApproxEquals(const RecordBatch& other) const {
return true;
}
+Result<std::shared_ptr<RecordBatch>> RecordBatch::SelectColumns(
+ const std::vector<int>& indices) const {
+ int n = static_cast<int>(indices.size());
+
+ std::vector<std::shared_ptr<Field>> fields(n);
+ std::vector<std::shared_ptr<Array>> columns(n);
Review comment:
Done!
##########
File path: cpp/src/arrow/record_batch.cc
##########
@@ -253,6 +253,27 @@ bool RecordBatch::ApproxEquals(const RecordBatch& other) const {
return true;
}
+Result<std::shared_ptr<RecordBatch>> RecordBatch::SelectColumns(
+ const std::vector<int>& indices) const {
+ int n = static_cast<int>(indices.size());
+
+ std::vector<std::shared_ptr<Field>> fields(n);
+ std::vector<std::shared_ptr<Array>> columns(n);
+
+ for (int i = 0; i < n; i++) {
+ int pos = indices[i];
+ if (pos < 0 || pos > num_columns() - 1) {
+ return Status::Invalid("Invalid column index ", pos, " to select columns.");
+ }
+ fields[i] = schema()->field(pos);
+ columns[i] = column(pos);
+ }
+
+ auto new_schema =
+ std::make_shared<arrow::Schema>(std::move(fields), schema()->metadata());
+ return RecordBatch::Make(new_schema, num_rows(), columns);
Review comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #10100:
URL: https://github.com/apache/arrow/pull/10100#discussion_r615961099
##########
File path: cpp/src/arrow/record_batch.cc
##########
@@ -253,6 +253,26 @@ bool RecordBatch::ApproxEquals(const RecordBatch& other) const {
return true;
}
+Result<std::shared_ptr<RecordBatch>> RecordBatch::SelectColumns(const std::vector<int>& indices) const{
+ int n = static_cast<int>(indices.size());
+
+ std::vector<std::shared_ptr<Field>> fields(n);
+ std::vector<std::shared_ptr<Array>> columns(n);
+
+ for (int i = 0; i < n; i++) {
+ int pos = indices[i];
+ if (pos < 0 || pos > num_columns() - 1) {
+ return Status::Invalid("Invalid column index ", pos, " to select columns.");
+ }
+ fields[i] = schema()->field(pos);
+ columns[i] = column(pos);
+ }
+
+ auto schema = std::make_shared<arrow::Schema>(std::move(fields));
Review comment:
Here we probably want to preserve the metadata of the schema (see the `Table::SelectColumns` method)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10100:
URL: https://github.com/apache/arrow/pull/10100#issuecomment-824291648
CI failures are unrelated. Thank you @thisisnic !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10100: ARROW-11660: [C++] Move RecordBatch::SelectColumns method from R to C++ library
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10100:
URL: https://github.com/apache/arrow/pull/10100#discussion_r617619087
##########
File path: cpp/src/arrow/record_batch.cc
##########
@@ -253,6 +253,27 @@ bool RecordBatch::ApproxEquals(const RecordBatch& other) const {
return true;
}
+Result<std::shared_ptr<RecordBatch>> RecordBatch::SelectColumns(
+ const std::vector<int>& indices) const {
+ int n = static_cast<int>(indices.size());
+
+ std::vector<std::shared_ptr<Field>> fields(n);
+ std::vector<std::shared_ptr<Array>> columns(n);
Review comment:
Nit, but you can use the `FieldVector` and `ArrayVector` type aliases, respectively.
##########
File path: cpp/src/arrow/record_batch.cc
##########
@@ -253,6 +253,27 @@ bool RecordBatch::ApproxEquals(const RecordBatch& other) const {
return true;
}
+Result<std::shared_ptr<RecordBatch>> RecordBatch::SelectColumns(
+ const std::vector<int>& indices) const {
+ int n = static_cast<int>(indices.size());
+
+ std::vector<std::shared_ptr<Field>> fields(n);
+ std::vector<std::shared_ptr<Array>> columns(n);
+
+ for (int i = 0; i < n; i++) {
+ int pos = indices[i];
+ if (pos < 0 || pos > num_columns() - 1) {
+ return Status::Invalid("Invalid column index ", pos, " to select columns.");
+ }
+ fields[i] = schema()->field(pos);
+ columns[i] = column(pos);
+ }
+
+ auto new_schema =
+ std::make_shared<arrow::Schema>(std::move(fields), schema()->metadata());
+ return RecordBatch::Make(new_schema, num_rows(), columns);
Review comment:
`std::move(columns)` will avoid a vector copy here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org