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/03/09 21:10:32 UTC
[GitHub] [arrow] ianmcook opened a new pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
ianmcook opened a new pull request #9665:
URL: https://github.com/apache/arrow/pull/9665
This copies the schema checks from `RecordBatch__from_arrays__known_schema` to `CollectTableColumns` to prevent errors when the schema does not match the columns
----------------------------------------------------------------
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] nealrichardson closed pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #9665:
URL: https://github.com/apache/arrow/pull/9665
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#discussion_r591703013
##########
File path: r/tests/testthat/test-Table.R
##########
@@ -359,9 +359,9 @@ test_that("table() auto splices (ARROW-5718)", {
})
test_that("Validation when creating table with schema (ARROW-10953)", {
- tab <- Table$create(data.frame(), schema = schema(a = int32()))
- skip("This segfaults")
- expect_identical(dim(as.data.frame(tab)), c(0L, 1L))
+ expect_error(Table$create(data.frame(), schema = schema(a = int32())))
Review comment:
Should we assert the error messages, just to make sure it looks like we want? (And that it's throwing the error we expect)
----------------------------------------------------------------
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] nealrichardson commented on pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-795740467
> You can always do something like this
>
> ```r
> Table$create(a = integer(0), b = integer(0), schema = schema(a = int32(), b = int32()))
> ```
We can probably do better, but out of scope 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
[GitHub] [arrow] ianmcook edited a comment on pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
ianmcook edited a comment on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-794601007
> > maybe there are other reasons for wanting a table with dim(0, 1)?
>
> See the original issue linked to this one, [ARROW-10642](https://issues.apache.org/jira/browse/ARROW-10642). I think one could (the reporter did) make the case for wanting to create an empty table with a Schema, though maybe there should be a different mechanism for that. Either way, it's always better to error than segfault, and errors with helpful messages are even better.
You can always do something like this
```r
Table$create(a = integer(0), b = integer(0), schema = schema(a = int32(), b = int32()))
```
----------------------------------------------------------------
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 #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-794508384
https://issues.apache.org/jira/browse/ARROW-10953
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#discussion_r590823206
##########
File path: r/src/table.cpp
##########
@@ -257,8 +257,16 @@ arrow::Status AddMetadataFromDots(SEXP lst, int num_fields,
arrow::Status CollectTableColumns(
SEXP lst, const std::shared_ptr<arrow::Schema>& schema, int num_fields, bool inferred,
std::vector<std::shared_ptr<arrow::ChunkedArray>>& columns) {
+ if (!inferred && schema->num_fields() != num_fields) {
+ cpp11::stop("incompatible. schema has %d fields, and %d columns are supplied",
+ schema->num_fields(), num_fields);
+ }
auto extract_one_column = [&columns, &schema, inferred](int j, SEXP x,
- cpp11::r_string) {
+ std::string name) {
+ if (!inferred && schema->field(j)->name() != name) {
+ cpp11::stop("field at index %d has name '%s' != '%s'", j + 1,
+ schema->field(j)->name().c_str(), name.c_str());
+ }
if (Rf_inherits(x, "ChunkedArray")) {
columns[j] = cpp11::as_cpp<std::shared_ptr<arrow::ChunkedArray>>(x);
} else if (Rf_inherits(x, "Array")) {
Review comment:
@romainfrancois can probably give a better answer, but I would say that if you're inside a function that returns `arrow::Status` (like this one), you should probably `return arrow::status::RError`.
----------------------------------------------------------------
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] ianmcook commented on a change in pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#discussion_r590722753
##########
File path: r/src/table.cpp
##########
@@ -257,8 +257,16 @@ arrow::Status AddMetadataFromDots(SEXP lst, int num_fields,
arrow::Status CollectTableColumns(
SEXP lst, const std::shared_ptr<arrow::Schema>& schema, int num_fields, bool inferred,
std::vector<std::shared_ptr<arrow::ChunkedArray>>& columns) {
+ if (!inferred && schema->num_fields() != num_fields) {
+ cpp11::stop("incompatible. schema has %d fields, and %d columns are supplied",
+ schema->num_fields(), num_fields);
+ }
auto extract_one_column = [&columns, &schema, inferred](int j, SEXP x,
- cpp11::r_string) {
+ std::string name) {
+ if (!inferred && schema->field(j)->name() != name) {
+ cpp11::stop("field at index %d has name '%s' != '%s'", j + 1,
+ schema->field(j)->name().c_str(), name.c_str());
+ }
if (Rf_inherits(x, "ChunkedArray")) {
columns[j] = cpp11::as_cpp<std::shared_ptr<arrow::ChunkedArray>>(x);
} else if (Rf_inherits(x, "Array")) {
Review comment:
Does it matter whether I use `return status::RError` or `cpp11::stop()`?
----------------------------------------------------------------
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] nealrichardson commented on pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-794586321
> maybe there are other reasons for wanting a table with dim(0, 1)?
See the original issue linked to this one, ARROW-10642. I think one could (the reporter did) make the case for wanting to create an empty table with a Schema, though maybe there should be a different mechanism for that. Either way, it's always better to error than segfault, and errors with helpful messages are even better.
----------------------------------------------------------------
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] jonkeane commented on pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-794527158
It looks like https://github.com/apache/arrow/blob/master/r/tests/testthat/test-Table.R#L361-L365 should be updated to catch the error?
----------------------------------------------------------------
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] ianmcook commented on pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-794601007
> > maybe there are other reasons for wanting a table with dim(0, 1)?
>
> See the original issue linked to this one, [ARROW-10642](https://issues.apache.org/jira/browse/ARROW-10642). I think one could (the reporter did) make the case for wanting to create an empty table with a Schema, though maybe there should be a different mechanism for that. Either way, it's always better to error than segfault, and errors with helpful messages are even better.
You can always do something like this
```r
dim(Table$create(a = integer(0), b = integer(0), schema = schema(a = int32(), b = int32())))
```
----------------------------------------------------------------
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] ianmcook commented on a change in pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#discussion_r590921731
##########
File path: r/src/table.cpp
##########
@@ -257,8 +257,16 @@ arrow::Status AddMetadataFromDots(SEXP lst, int num_fields,
arrow::Status CollectTableColumns(
SEXP lst, const std::shared_ptr<arrow::Schema>& schema, int num_fields, bool inferred,
std::vector<std::shared_ptr<arrow::ChunkedArray>>& columns) {
+ if (!inferred && schema->num_fields() != num_fields) {
+ cpp11::stop("incompatible. schema has %d fields, and %d columns are supplied",
+ schema->num_fields(), num_fields);
+ }
auto extract_one_column = [&columns, &schema, inferred](int j, SEXP x,
- cpp11::r_string) {
+ std::string name) {
+ if (!inferred && schema->field(j)->name() != name) {
+ cpp11::stop("field at index %d has name '%s' != '%s'", j + 1,
+ schema->field(j)->name().c_str(), name.c_str());
+ }
if (Rf_inherits(x, "ChunkedArray")) {
columns[j] = cpp11::as_cpp<std::shared_ptr<arrow::ChunkedArray>>(x);
} else if (Rf_inherits(x, "Array")) {
Review comment:
The second one is inside a lambda, so it's a lot simpler to just throw an error. But if there's a good reason to change it, I can.
----------------------------------------------------------------
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] ianmcook commented on pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-794596650
> It looks like https://github.com/apache/arrow/blob/master/r/tests/testthat/test-Table.R#L361-L365 should be updated to catch the error
Done in b712e810e9caa89394923eb03525665dc3caa058
----------------------------------------------------------------
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] ianmcook removed a comment on pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
ianmcook removed a comment on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-794512927
@github-actions crossbow submit -g r
----------------------------------------------------------------
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] jonkeane edited a comment on pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
jonkeane edited a comment on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-794527158
It looks like https://github.com/apache/arrow/blob/master/r/tests/testthat/test-Table.R#L361-L365 should be updated to catch the error? And the implied behavior in the skip portion also seems different than an error. I think the error is probably better, but maybe there are other reasons for wanting a table with dim(0, 1)?
----------------------------------------------------------------
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] ianmcook commented on a change in pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#discussion_r590722753
##########
File path: r/src/table.cpp
##########
@@ -257,8 +257,16 @@ arrow::Status AddMetadataFromDots(SEXP lst, int num_fields,
arrow::Status CollectTableColumns(
SEXP lst, const std::shared_ptr<arrow::Schema>& schema, int num_fields, bool inferred,
std::vector<std::shared_ptr<arrow::ChunkedArray>>& columns) {
+ if (!inferred && schema->num_fields() != num_fields) {
+ cpp11::stop("incompatible. schema has %d fields, and %d columns are supplied",
+ schema->num_fields(), num_fields);
+ }
auto extract_one_column = [&columns, &schema, inferred](int j, SEXP x,
- cpp11::r_string) {
+ std::string name) {
+ if (!inferred && schema->field(j)->name() != name) {
+ cpp11::stop("field at index %d has name '%s' != '%s'", j + 1,
+ schema->field(j)->name().c_str(), name.c_str());
+ }
if (Rf_inherits(x, "ChunkedArray")) {
columns[j] = cpp11::as_cpp<std::shared_ptr<arrow::ChunkedArray>>(x);
} else if (Rf_inherits(x, "Array")) {
Review comment:
Does it matter whether I use `return arrow::status::RError` or `cpp11::stop()`?
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#discussion_r591703013
##########
File path: r/tests/testthat/test-Table.R
##########
@@ -359,9 +359,9 @@ test_that("table() auto splices (ARROW-5718)", {
})
test_that("Validation when creating table with schema (ARROW-10953)", {
- tab <- Table$create(data.frame(), schema = schema(a = int32()))
- skip("This segfaults")
- expect_identical(dim(as.data.frame(tab)), c(0L, 1L))
+ expect_error(Table$create(data.frame(), schema = schema(a = int32())))
Review comment:
Should we assert the error messages, just to make sure it looks like we want?
----------------------------------------------------------------
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] ianmcook commented on a change in pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#discussion_r591751302
##########
File path: r/tests/testthat/test-Table.R
##########
@@ -359,9 +359,9 @@ test_that("table() auto splices (ARROW-5718)", {
})
test_that("Validation when creating table with schema (ARROW-10953)", {
- tab <- Table$create(data.frame(), schema = schema(a = int32()))
- skip("This segfaults")
- expect_identical(dim(as.data.frame(tab)), c(0L, 1L))
+ expect_error(Table$create(data.frame(), schema = schema(a = int32())))
Review comment:
Done in 50badd4e1af999cca6ff9f173d2d6a797d5407b9
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#discussion_r591700224
##########
File path: r/src/table.cpp
##########
@@ -257,8 +257,16 @@ arrow::Status AddMetadataFromDots(SEXP lst, int num_fields,
arrow::Status CollectTableColumns(
SEXP lst, const std::shared_ptr<arrow::Schema>& schema, int num_fields, bool inferred,
std::vector<std::shared_ptr<arrow::ChunkedArray>>& columns) {
+ if (!inferred && schema->num_fields() != num_fields) {
+ cpp11::stop("incompatible. schema has %d fields, and %d columns are supplied",
+ schema->num_fields(), num_fields);
+ }
auto extract_one_column = [&columns, &schema, inferred](int j, SEXP x,
- cpp11::r_string) {
+ std::string name) {
+ if (!inferred && schema->field(j)->name() != name) {
+ cpp11::stop("field at index %d has name '%s' != '%s'", j + 1,
+ schema->field(j)->name().c_str(), name.c_str());
+ }
if (Rf_inherits(x, "ChunkedArray")) {
columns[j] = cpp11::as_cpp<std::shared_ptr<arrow::ChunkedArray>>(x);
} else if (Rf_inherits(x, "Array")) {
Review comment:
🤷 ok with me, we can change later if there's a reason to.
----------------------------------------------------------------
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] ianmcook commented on pull request #9665: ARROW-10953: [R] Validate when creating Table with schema
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9665:
URL: https://github.com/apache/arrow/pull/9665#issuecomment-794512927
@github-actions crossbow submit -g r
----------------------------------------------------------------
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