Skip to content

Use data.table subsetting to avoid copy of large objects #180

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.

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

Merged
merged 6 commits into from
Jul 23, 2025

Conversation

MichaelChirico
Copy link
Contributor

Closes #179

This also includes #178 since it's logically related and I noticed the former while working on this.

Since I'm working on a fork I don't know of a way to make the diff cleaner (usually I'd put the merge target as #178, but that would only work as a PR to my fork).

@MichaelChirico MichaelChirico changed the title Use data.table suvsetting to avoid copy of large objects Use data.table subsetting to avoid copy of large objects May 12, 2025
@flying-sheep
Copy link
Member

Hi, thanks! Does a S3 method have to be exported in order to exist? I’d prefer not to add a public API that’s unrelated to repr’s purpose!

@MichaelChirico
Copy link
Contributor Author

I guess it depends on what you mean by "exported" -- as you see in the NAMESPACE, we've registered an S3 method, so in some sense that's "public+exported".

OTOH:

  • partition_from_parts is not exported. I think that means downstreams also can't define their own S3 methods for this function, though I'm not sure (and not sure anyone would want that anyway)
  • repr::partition_from_parts, repr::partition_from_parts.data.table, etc., will not work, as these functions are not exported. ::: would be needed.

I think the approach I took here is natural; depending on your concerns, we could also fake S3 dispatch ourselves and keep everything private like:

partition_from_parts = function(...) {
  if (inherits(., "data.table")) return(partition_from_parts.data.table(...))
  partition_from_parts.default(...)
}

@flying-sheep
Copy link
Member

flying-sheep commented Jul 22, 2025

Ah, I see, there’s a separate export(...) for actually exported functions, sorry for the confusion!

Looks like R CMD check is complaining. Could you take care of that please?

@MichaelChirico
Copy link
Contributor Author

Yea, a very unfortunate feature of {roxygen2} that the same @export is used for S3 method registration as for public function export :\

@flying-sheep flying-sheep added this pull request to the merge queue Jul 23, 2025
@flying-sheep
Copy link
Member

Great, thank you!

Merged via the queue into IRkernel:main with commit be21096 Jul 23, 2025
2 checks passed
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.

repr_text does expensive copy of data.table input
2 participants