Skip to content
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

ARROW-17387: [R] Implement dplyr::across() inside filter() #14281

Merged
merged 15 commits into from Oct 11, 2022

Conversation

thisisnic
Copy link
Member

@thisisnic thisisnic commented Sep 30, 2022

The implementation differs here from dplyr in that some steps are removed as the dplyr functionality evaluates functions sooner and so has extra steps required.

@github-actions
Copy link

@thisisnic thisisnic changed the title ARROW-17387: [R] Implement dplyr::across() inside filter() [WIP] ARROW-17387: [R] Implement dplyr::across() inside filter() Sep 30, 2022
@thisisnic thisisnic marked this pull request as ready for review September 30, 2022 10:17
Copy link
Contributor

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

One suggested cleanup but otherwise LGTM!

r/R/dplyr-across.R Outdated Show resolved Hide resolved
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
@thisisnic thisisnic merged commit a1d7a44 into apache:master Oct 11, 2022
@ursabot
Copy link

ursabot commented Oct 11, 2022

Benchmark runs are scheduled for baseline = ece5b65 and contender = a1d7a44. a1d7a44 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] test-mac-arm
[Failed ⬇️1.1% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.21% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] a1d7a449 ec2-t3-xlarge-us-east-2
[Failed] a1d7a449 test-mac-arm
[Failed] a1d7a449 ursa-i9-9960x
[Finished] a1d7a449 ursa-thinkcentre-m75q
[Finished] ece5b654 ec2-t3-xlarge-us-east-2
[Failed] ece5b654 test-mac-arm
[Failed] ece5b654 ursa-i9-9960x
[Finished] ece5b654 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Oct 11, 2022

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

@jorisvandenbossche
Copy link
Member

I got notified in another PR (which was merged after this one) about potential perf regression, but so I think it is this PR that gives a slight slowdown in some benchmarks. The biggest regression (TPCH-08) seems a flaky measurement, but for TPCH-16 it seems persistent (although it's only a very small slowdown, no idea if it is significant)

@thisisnic
Copy link
Member Author

I got notified in another PR (which was merged after this one) about potential perf regression, but so I think it is this PR that gives a slight slowdown in some benchmarks. The biggest regression (TPCH-08) seems a flaky measurement, but for TPCH-16 it seems persistent (although it's only a very small slowdown, no idea if it is significant)

What's a good way to investigate this? I wouldn't expect a slowdown from this PR, but can't rule it out entirely.

@jorisvandenbossche
Copy link
Member

I suppose the easiest would be to first try to get the code behind that benchmark running locally (but not familiar with the R benchmarks for what's the easiest way to so this), and then if you can run it locally, then checking if you can reproduce the slowdown, and if so you can profile both cases (but again, I don't know how to do that in R)

@thisisnic
Copy link
Member Author

Thinking about this more, I'd expect it to be a tiny bit slower but not enough to affect the benchmarks, and given that this block of code has been added to other operations which are used in the benchmarks, I'd expect to see a regression in those too if it was problematic. @jonkeane, I don't suppose you'd mind helping me take a look at this?

@nealrichardson
Copy link
Contributor

I don't think this is concerning. None of the TPC-H queries have the features that this PR adds support for, so there's not much extra work being added that would add meaningful time. I did a quick benchmark of the "fast" path of the function in question here, and it costs <1ms:

> q <- rlang:::quos(a + b, c + d, e - f, f(a, b, c))
> bench::mark(arrow:::expand_across(list(), q))
# A tibble: 1 × 13
  expression                            min median itr/s…¹ mem_a…² gc/se…³ n_itr
  <bch:expr>                       <bch:tm> <bch:>   <dbl> <bch:b>   <dbl> <int>
1 arrow:::expand_across(list(), q)   87.4µs 91.5µs  10755.  42.9KB    43.7  4923

@jorisvandenbossche
Copy link
Member

Conbench also only identified the 0.01 and 0.1 scale factors as slowdown. The same benchmark but with scale factor 1 or 10 didn't show a consistent slowdown: https://conbench.ursa.dev/compare/benchmarks/44eef0b11f204c09bebde7b2a4050c98...07d114936d9c4b94b69aa712f0a8423e/ (which matches with what Neal is saying)

@nealrichardson
Copy link
Contributor

Yeah, I've separately found those low scale factor benchmarks to be sensitive/flaky. They're useful in alerting when we add 10ms to the query assembly time (see also #13985), and those micro-regressions do add up if not addressed. But they're not all that noticeable if you're working with larger data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants