Skip to content

Conversation

@DavisVaughan
Copy link
Member

Follow up to #1537

That PR implemented:

vec_proxy_equal.vctrs_rcrd <- function(x, ...) {
  # Recursively proxy using a data frame
  vec_proxy_equal(new_data_frame(x))
}

but that means that if vec_proxy_order/compare() for a rcrd class fall through to this, then vec_proxy_equal() will be called on each field, which isn't right.

To fix that, we need vec_proxy_order/compare() methods specifically for vctrs_rcrd which call their corresponding function internally after creating a data frame.

This requires tweaking the vctrs-rational class, because it relied on the vec_proxy_compare.vctrs_rational() method being run when sort(x) was called, but this is no longer the case because the new vec_proxy_order.vctrs_rcrd() method intercepts it instead.

@lionel-
Copy link
Member

lionel- commented Sep 16, 2022

It feels like we could make it simpler by recursively proxying data frame proxies from the generics? Then the rcrd methods only need to call new_data_frame(). Do you think that would work?

@DavisVaughan
Copy link
Member Author

Would that mean we remove the special data frame methods (like the one below) and let data frames fall through to vec_proxy() (which would be a no-op)?

> vec_proxy_equal.data.frame
function(x, ...) {
  df_proxy(x, VCTRS_PROXY_KIND_equal)
}

And then internally we'd say something like

r_obj* vec_proxy_equal(r_obj* x) {
  r_obj* method = KEEP(vec_proxy_equal_method(x));
  r_obj* out = KEEP(vec_proxy_equal_invoke(x, method));
  if (is_data_frame(out)) {
    # Apply proxy recursively if the proxy is a data frame
    out = df_proxy(out, VCTRS_PROXY_KIND_equal)
  }
  FREE(2);
  return out;
}

Noting that I still don't think we'd do this automatic recursion for vec_proxy(), that is handled by your other PR

@lionel-
Copy link
Member

lionel- commented Sep 16, 2022

yep something like that

@DavisVaughan
Copy link
Member Author

DavisVaughan commented Sep 16, 2022

Ok, this PR now removes all of the specialized methods for data frames and record vectors, and instead relies on the fact that:

  • Data frames return themselves as the proxy
  • Records fall through to vec_proxy.vctrs_rcrd() which returns a data frame
  • If a data frame is returned as the proxy, we now apply the proxy function recursively over the columns

I think this is nice because someone could, in theory, return a complicated data frame containing a df-col from their custom vec_proxy_equal() method, which currently breaks our internal C code because it assumes df-cols have been flattened. This change ensures that even if a custom method returns a data frame like this, we post-process it to flatten and potentially unwrap it.

Revdepcheck "b4fb11fb-cbef-445a-af0b-1601e7f6bd58" - passes with no changes to worse from this PR

@DavisVaughan DavisVaughan force-pushed the fix/recursive-rcrd-proxies branch from 99e1cd6 to d570de3 Compare September 16, 2022 17:43
@DavisVaughan DavisVaughan changed the title Add recursive vec_proxy_compare() and vec_proxy_order() rcrd methods Always apply specialized proxy methods recursively over data frames Sep 16, 2022
Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

Looks like a great improvement.

Required because now there is a `vec_proxy_order.vctrs_rcrd()` method that would otherwise sit in front of the `vec_proxy_compare.vctrs_rational()` method
This allows us to remove the data frame and record methods:
- Data frames return themselves as the proxy by default, which is then automatically recursively proxied
- Records fall through to `vec_proxy.vctrs_rcrd()`, which returns a data frame that is then proxied recursively
@DavisVaughan DavisVaughan force-pushed the fix/recursive-rcrd-proxies branch from d570de3 to 5fc4d6c Compare September 19, 2022 13:55
@DavisVaughan DavisVaughan merged commit 413460f into r-lib:main Sep 19, 2022
@DavisVaughan DavisVaughan deleted the fix/recursive-rcrd-proxies branch September 19, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants