perf: Ipc exec multiple paths #15040
Conversation
mickvangelderen
force-pushed
the
ipc-exec-multiple-paths
branch
from
75eb0a7
to
80bfb7a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15040 +/- ##
==========================================
- Coverage 81.25% 81.22% -0.04%
==========================================
Files 1354 1348 -6
Lines 175390 175534 +144
Branches 2518 2508 -10
==========================================
+ Hits 142505 142569 +64
- Misses 32404 32485 +81
+ Partials 481 480 -1 ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
performance
|
This PR seems to pass now but won't after merging #15065 |
mickvangelderen
force-pushed
the
ipc-exec-multiple-paths
branch
3 times, most recently
from
82a5e8f
to
7b8bb3c
Compare
| @@ -101,7 +227,7 @@ impl Executor for IpcExec { | |||
| }; | |||
|
|
|||
| let profile_name = if state.has_node_timer() { | |||
| let mut ids = vec![self.path.to_string_lossy().into()]; | |||
| let mut ids = vec![self.paths[0].to_string_lossy().into()]; | |||
There was a problem hiding this comment.
Taken from the parquet implementation. Should we just map each path instead?
mickvangelderen
force-pushed
the
ipc-exec-multiple-paths
branch
5 times, most recently
from
cf9b13a
to
4c116c3
Compare
mickvangelderen
requested review from
ritchie46,
stinodego,
c-peters,
alexander-beedie,
MarcoGorelli and
orlp
as code owners
| def test_scan( | ||
| capfd: Any, monkeypatch: pytest.MonkeyPatch, data_file: _DataFile, force_async: bool | ||
| ) -> None: | ||
| if data_file.path.suffix == ".csv" and force_async: | ||
| pytest.skip(reason="async reading of .csv not yet implemented") | ||
|
|
||
| if force_async: | ||
| _enable_force_async(monkeypatch) | ||
|
|
||
| df = _scan(data_file.path, data_file.df.schema).collect() | ||
|
|
||
| if force_async: | ||
| _assert_force_async(capfd) | ||
|
|
There was a problem hiding this comment.
Each test fn now has a lot of fluff which distracts from what the test is actually doing. I have chosen to leave it as is now so we can get this feature out. If anyone sees a way to simplify and deduplicate this please share your ideas.
What I was thinking of is having fixtures for (ipc|csv|parquet, sync|async) * (single|glob), and then having a separate test file cache which can memoize requested test files (ipc|csv|parquet, single|glob).
|
Hmm.. Something goes wrong with the windows tests. |
@ritchie46 Testing this idea to really fix the windows tests #15191 |
mickvangelderen
force-pushed
the
ipc-exec-multiple-paths
branch
from
062a11b
to
a0194c9
Compare
No description provided.