Skip to content
Permalink
Browse files

summarise() correctly materialize list columns subsets.

closes #4349
  • Loading branch information...
romainfrancois committed May 10, 2019
1 parent 9c6f59e commit 036de90fbf9e3eef72c015982a5d1294d2157a2c
Showing with 13 additions and 9 deletions.
  1. +5 −6 inst/include/dplyr/data/DataMask.h
  2. +8 −3 inst/include/tools/SlicingIndex.h
@@ -27,6 +27,8 @@ template <class SlicedTibble> class DataMaskWeakProxy;
template <typename SlicedTibble>
struct ColumnBinding {
private:
typedef typename SlicedTibble::slicing_index Index;

// is this a summary binding, i.e. does it come from summarise
bool summary;

@@ -48,7 +50,7 @@ struct ColumnBinding {
// the active binding function calls eventually calls DataMask<>::materialize
// which calls this method
inline SEXP get(
const typename SlicedTibble::slicing_index& indices,
const Index& indices,
SEXP mask_resolved)
{
return materialize(indices, mask_resolved);
@@ -81,7 +83,7 @@ struct ColumnBinding {
// this is only used for its side effect of storing the result
// in the right environment
inline void update_indices(
const typename SlicedTibble::slicing_index& indices,
const Index& indices,
SEXP mask_resolved)
{
materialize(indices, mask_resolved);
@@ -145,13 +147,11 @@ struct ColumnBinding {

// materialize
Rcpp::Shield<SEXP> value(summary ?
column_subset(data, RowwiseSlicingIndex(indices.group()), frame) :
column_subset(data, Index(indices.group()), frame) :
column_subset(data, indices, frame)
);
MARK_NOT_MUTABLE(value);

// store it in the mask_resolved environment
Rf_defineVar(symbol, value, mask_resolved);

This comment has been minimized.

Copy link
@romainfrancois

romainfrancois Jul 3, 2019

Author Member

🤕

return value;
}

@@ -520,7 +520,6 @@ class DataMask {
get_current_indices(), mask_resolved
);


// remember to pro-actievely materialize this binding on the next group
materialized.push_back(idx);

@@ -20,17 +20,21 @@ class SlicingIndex {
// It is used in grouped operations (group_by()).
class GroupedSlicingIndex : public SlicingIndex {
public:
GroupedSlicingIndex(): data(), group_index(-1) {
GroupedSlicingIndex(): data(), group_index(-1), preserved(true) {
R_PreserveObject(data);
}

~GroupedSlicingIndex() {
if (group_index == -1) {
if (preserved) {
R_ReleaseObject(data);
}
}

GroupedSlicingIndex(SEXP data_, int group_) : data(data_), group_index(group_) {}
GroupedSlicingIndex(SEXP data_, int group_) : data(data_), group_index(group_), preserved(false) {}

GroupedSlicingIndex(int group_) : data(Rf_ScalarInteger(group_ + 1)), group_index(group_), preserved(true) {
R_PreserveObject(data);
}

virtual int size() const {
return data.size();
@@ -57,6 +61,7 @@ class GroupedSlicingIndex : public SlicingIndex {
Rcpp::IntegerVectorView data;

int group_index;
bool preserved;
};

// A RowwiseSlicingIndex selects a single row, which is also the group ID by definition.

0 comments on commit 036de90

Please sign in to comment.
You can’t perform that action at this time.