Skip to content
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.

Sign up for GitHub

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

Jump to bottom

when a column has not classes but has the object bit set, pretend it … #3394

Merged
merged 8 commits into from Mar 12, 2018
Merged

when a column has not classes but has the object bit set, pretend it … #3394

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
87af854
when a column has not classes but has the object bit set, pretend it …
romainfrancois Mar 5, 2018
0dcf9d4
tidy style
romainfrancois Mar 6, 2018
1bcaf60
+ NEWS entry
romainfrancois Mar 6, 2018
e4acbac
revert to almost previous version of `warn_loss_attr` / `is_class_kno…
romainfrancois Mar 7, 2018
e173848
move the tests down to inherits_from
romainfrancois Mar 12, 2018
230d884
that test is useless as `inherits_from` because is only ever called w…
romainfrancois Mar 12, 2018
bca0480
astyle
romainfrancois Mar 12, 2018
175ef19
Merge branch 'master' into fix-issue-3349
Mar 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Expand Up @@ -69,6 +69,8 @@

* Added an `.onDetach()` hook that allows for plyr to be loaded and attached without the warning message that says functions in dplyr will be masked, since dplyr is no longer attached (#3359, @jwnorman).

* `bind_rows()` works around corrupt columns that have the object bit set while having no class attribute (#3349).

# dplyr 0.7.4

* Fix recent Fedora and ASAN check errors (#3098).
Expand Down
5 changes: 2 additions & 3 deletions inst/include/dplyr/Collecter.h
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ static bool is_class_known(SEXP x) {
known_classes.insert("integer64");
known_classes.insert("table");
}
if (OBJECT(x)) {
if (OBJECT(x) && !Rf_isNull(Rf_getAttrib(x, R_ClassSymbol))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to understand the problem here. If you want to avoid querying the class symbol and the OBJECT() bit twice (and also clean up the code a bit), maybe this check could be added to line 17 in this file? I suspect inherits_from() will fail if the class attribute is NULL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inherits_from and is_class_known are only used once each anyway. it all looks kind of confusing. But I feel that this patch is good enough for fixing the problem, and we can revisit the code later.

I suspect inherits_from() will fail if the class attribute is NULL.

yes that was part of the initial problem.

return inherits_from(x, known_classes);
} else {
return true;
Expand All @@ -44,9 +44,8 @@ static bool is_class_known(SEXP x) {
static inline void warn_loss_attr(SEXP x) {
/* Attributes are lost with unknown classes */
if (!is_class_known(x)) {
SEXP classes = Rf_getAttrib(x, R_ClassSymbol);
Rf_warning("Vectorizing '%s' elements may not preserve their attributes",
CHAR(STRING_ELT(classes, 0)));
CHAR(STRING_ELT(Rf_getAttrib(x, R_ClassSymbol), 0)));
}
}

Expand Down
7 changes: 7 additions & 0 deletions tests/testthat/test-binds.R
View file
Open in desktop
Expand Up @@ -571,6 +571,13 @@ test_that("bind_rows() accepts lists of dataframe-like lists as first argument",
expect_identical(bind_rows(list(list(a = 1, b = 2))), tibble(a = 1, b = 2))
})

test_that("columns that are OBJECT but have NULL class are handled gracefully (#3349)", {
mod <- lm(y~ ., data = freeny)
data <- model.frame(mod)
data_list <- list(data, data)
res <- bind_rows(data_list)
expect_equal(names(res), names(data))
})

# Vectors ------------------------------------------------------------

Expand Down