nealrichardson commented on PR #19706: URL: https://github.com/apache/arrow/pull/19706#issuecomment-1377786906
> Happy to review more as this progresses...just a few observations: > > * I think `ls(x)` is not a robust way to check for collisions. You may have to use `get()` or `mget()` with `inherits = TRUE` since the symbol might exist in a parent environment? For R6 this works because I'm just looking for a method name, and inheritance isn't handled via nested environments it seems (i.e. `class_name` is defined in the base `ArrowObject` class but it shows up in `ls()` for `Expression`). We've been using this trick for a while for Tables and RecordBatches: https://github.com/apache/arrow/blob/master/r/R/arrow-tabular.R#L153-L160 > * Can `Exression$field_ref()` be updated to to accept a vector? Then you could avoid the C++ `compute___expr__nested_field_ref()` and do it all in R.? It could, and that was my first instinct too, but that's actually not helpful for the `$` method, where you have a FieldRef and the name of the thing you want to nest further. You'd still have to call out to C++ to get the vector of field names in the path of the existing FieldRef. > * There should probably be a way to get all the components of a field ref (maybe `compute___expr__get_field_ref_name()` should return `cpp11::strings()`? Probably. I'm going to focus on getting the essential dplyr stuff working first and we'll see what's required for that. One further complication that is completely punted here is that FieldRefs can be from strings or integers, so a nested path could contain a mix of integers and strings: https://github.com/apache/arrow/blob/master/cpp/src/arrow/type.h#L1651-L1660 So just switching to `std::vector<std::string>` everywhere isn't a simple solution. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
