Do we need FullDataFrame at all ?

[romainfrancois]

The only place where this is used now is summarise_not_grouped (see below).
and if I do this replacement:

result = results[i] = res->process(NaturalSlicingIndex(df.nrows()));
// result = results[i] = res->process(FullDataFrame(df));

it passes the tests, etc ... that would mean we could rm all the process( FullDataFrame ) methods which would simplify the code. I have a feeling it might be related to #2311 and the refactoring of SlicingIndex into a class hierarchy ? @krlmlr ?

DataFrame summarise_not_grouped(DataFrame df, const QuosureList& dots) {
  int nexpr = dots.size();
  if (nexpr == 0) return DataFrame();

  LazySubsets subsets(df);
  NamedListAccumulator<DataFrame> accumulator;
  List results(nexpr);

  for (int i = 0; i < nexpr; i++) {
    Rcpp::checkUserInterrupt();

    const NamedQuosure& quosure = dots[i];
    Environment env = quosure.env();
    Shield<SEXP> expr_(quosure.expr());
    SEXP expr = expr_;
    SEXP result;

    // Unquoted vectors are directly used as column. Expressions are
    // evaluated in each group.
    if (is_vector(expr)) {
      result = validate_unquoted_value(expr, 1, quosure.name());
    } else {
      boost::scoped_ptr<Result> res(get_handler(expr, subsets, env));
      if (res) {
        result = results[i] = res->process(NaturalSlicingIndex(df.nrows()));
        // result = results[i] = res->process(FullDataFrame(df));
      } else {
        result = results[i] = CallProxy(quosure.expr(), subsets, env).eval();
      }
      check_supported_type(result, quosure.name());
      check_length(Rf_length(result), 1, "a summary value", quosure.name());
    }
    accumulator.set(quosure.name(), result);
    subsets.input_summarised(quosure.name(), SummarisedVariable(result));
  }

  List data = accumulator;
  copy_most_attributes(data, df);
  data.names() = accumulator.names();
  set_rownames(data, 1);
  return data;
}

[krlmlr]

Thanks. Let's get rid of it, I love PRs with more lines deleted than added ;-)

[romainfrancois]

Discovered this while preparing a new hybrid function as part of #1185
It looks like this is the only overload that really is used in Result and child classes:

virtual SEXP process(const SlicingIndex&)

ok, I'll make a PR to dump FullDataFrame

[krlmlr]

On the other hand, I'm not sure. I remember adding this class, but I forgot why. Let me look it up in the logs.

[romainfrancois]

It was there before, but back then there was no hierarchy in SlicingIndex.

The plan initially was that perhaps knowing that we treat all the data would be useful, but perhaps mutate and summarise have been refactored or something and the uses have disappeared

[krlmlr]

Thanks. I just wanted to double-check that removing it doesn't undo efforts planned for #2311.

[romainfrancois]

I don't think so.

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/