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 2020/07/01 23:42:35 UTC
[GitHub] [arrow] nealrichardson opened a new pull request #7614: ARROW-8977: [R] Table$create with schema crashes with some dictionary index types
nealrichardson opened a new pull request #7614:
URL: https://github.com/apache/arrow/pull/7614
----------------------------------------------------------------
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 #7614: ARROW-8977: [R] Table$create with schema crashes with some dictionary index types
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7614:
URL: https://github.com/apache/arrow/pull/7614#issuecomment-653081246
Thanks @fsaintjacques @pitrou, I did your suggestions and then a little more, PTAL
----------------------------------------------------------------
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 #7614: ARROW-8977: [R] Table$create with schema crashes with some dictionary index types
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7614:
URL: https://github.com/apache/arrow/pull/7614#discussion_r449080223
##########
File path: r/src/array_from_vector.cpp
##########
@@ -351,15 +351,20 @@ std::shared_ptr<Array> MakeFactorArrayImpl(Rcpp::IntegerVector_ factor,
std::shared_ptr<Array> MakeFactorArray(Rcpp::IntegerVector_ factor,
const std::shared_ptr<arrow::DataType>& type) {
- SEXP levels = factor.attr("levels");
- int n = Rf_length(levels);
- if (n < 128) {
+ auto* dict_type = checked_cast<arrow::DictionaryType*>(type.get());
Review comment:
That would be `const auto& dict_type` then. Otherwise +1 to what @fsaintjacques said.
----------------------------------------------------------------
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] fsaintjacques commented on a change in pull request #7614: ARROW-8977: [R] Table$create with schema crashes with some dictionary index types
Posted by GitBox <gi...@apache.org>.
fsaintjacques commented on a change in pull request #7614:
URL: https://github.com/apache/arrow/pull/7614#discussion_r449068026
##########
File path: r/src/array_from_vector.cpp
##########
@@ -351,15 +351,20 @@ std::shared_ptr<Array> MakeFactorArrayImpl(Rcpp::IntegerVector_ factor,
std::shared_ptr<Array> MakeFactorArray(Rcpp::IntegerVector_ factor,
const std::shared_ptr<arrow::DataType>& type) {
- SEXP levels = factor.attr("levels");
- int n = Rf_length(levels);
- if (n < 128) {
+ auto* dict_type = checked_cast<arrow::DictionaryType*>(type.get());
Review comment:
Small nit, we tend to prefer `const &` instead of pointer. This indicates to the reader that you won't mutate the object.
```suggestion
auto dict_type = checked_cast<const arrow::DictionaryType&>(*type);
```
----------------------------------------------------------------
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 #7614: ARROW-8977: [R] Table$create with schema crashes with some dictionary index types
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7614:
URL: https://github.com/apache/arrow/pull/7614#issuecomment-652699046
https://issues.apache.org/jira/browse/ARROW-8977
----------------------------------------------------------------
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] fsaintjacques closed pull request #7614: ARROW-8977: [R] Table$create with schema crashes with some dictionary index types
Posted by GitBox <gi...@apache.org>.
fsaintjacques closed pull request #7614:
URL: https://github.com/apache/arrow/pull/7614
----------------------------------------------------------------
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 #7614: ARROW-8977: [R] Table$create with schema crashes with some dictionary index types
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7614:
URL: https://github.com/apache/arrow/pull/7614#discussion_r449080783
##########
File path: r/src/array_from_vector.cpp
##########
@@ -351,15 +351,20 @@ std::shared_ptr<Array> MakeFactorArrayImpl(Rcpp::IntegerVector_ factor,
std::shared_ptr<Array> MakeFactorArray(Rcpp::IntegerVector_ factor,
const std::shared_ptr<arrow::DataType>& type) {
- SEXP levels = factor.attr("levels");
- int n = Rf_length(levels);
- if (n < 128) {
+ auto* dict_type = checked_cast<arrow::DictionaryType*>(type.get());
+ auto index_type = dict_type->index_type();
+ if (index_type->Equals(int8())) {
Review comment:
Faster if written as `index_type->id() == Type::INT8`. You may or may not bother, depending on how important this function is.
----------------------------------------------------------------
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