ump45nose opened a new pull request, #26531: URL: https://github.com/apache/datafusion/pull/26531
## Which issue does this PR close? - Closes #26446. ## Rationale for this change `DiskManagerBuilder::build` in `DiskManagerMode::Directories` mode creates a `datafusion-*` directory in every configured directory, and each one is removed again when the `DiskManager` is dropped. Engines that build a `RuntimeEnv` per query or per task pay those syscalls on every build, even when nothing spills (see the numbers from Comet in the issue). `DiskManagerMode::OsTmpDirectory` already creates its directory on the first `create_tmp_file`. ## What changes are included in this PR? - `DiskManager` keeps the configured directories (`configured_dirs`) and starts with an empty `local_dirs` in `Directories` mode, the same as `OsTmpDirectory`. - On the first `create_tmp_file`, it creates one `datafusion-*` directory in each configured directory (using the existing `create_local_dirs`), or an OS temp directory when none were configured, as before. The directories are only stored once all of them were created, so a failure is returned to that caller and the next call tries again. - Doc comments on `DiskManagerMode::Directories` and `temp_dir_paths` describe the new timing. ## What is the testing strategy for this PR? New unit tests in `disk_manager.rs`: - `lazy_configured_dir_creation`: building in `Directories` mode leaves the configured directories empty and `temp_dir_paths()` empty; the first temp file creates one `datafusion-*` directory per configured directory, later temp files reuse them, and they are removed after the files and the `DiskManager` are dropped. - `unusable_configured_dir_fails_first_temp_file`: a configured path that is a regular file now makes the first `create_tmp_file` fail instead of `build()`. - `test_disk_manager_create_spill_folder` now checks that a missing configured directory is created together with the first temp file. I ran `cargo test -p datafusion-execution`, `cargo clippy -p datafusion-execution --all-targets --all-features -- -D warnings` and `cargo fmt`. I did not run the `datafusion` core integration tests or sqllogictests locally. ## Are there any user-facing changes? Yes, as described in the issue: - `DiskManager::temp_dir_paths()` returns an empty list until the first temp file is created. This also means `SHOW datafusion.runtime.temp_directory` / `information_schema.df_settings` show `NULL` for a configured temp directory until something spills. - An unusable configured directory is reported by the first spill rather than by `DiskManagerBuilder::build()` / `RuntimeEnvBuilder::build()`. No public API signatures change. I'm not sure whether the `temp_directory` display change is acceptable or whether you'd prefer it to show the configured paths before the first spill; happy to adjust. --- This PR was prepared with AI assistance (code and description); I reviewed the change and the tests. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
