You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Tpt (via GitHub)" <gi...@apache.org> on 2026/10/07 09:47:12 UTC
[PR] DiskManager: include the description in file names [datafusion]
Tpt opened a new pull request, #26110:
URL: https://github.com/apache/datafusion/pull/26110
Ease debugging: the file names are now `.tmp.{description}.{randomness}` instead of `.tmp{randomness}`
This MR only changes the temporary file names, a test is extended to ensure that escaping works properly.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#discussion_r4219021305
##########
datafusion/execution/src/disk_manager.rs:
##########
@@ -585,6 +586,32 @@ impl SpillFile for RefCountedTempFile {
}))
}
}
+
+/// Escape a string to be safe to use in a temp file name prefix.
+///
+/// All non-alphanumeric characters are replaced by `_` and characters are put in lowercase.
+/// A `.tmp.` is added at the beginning and a last `.` is set at the end.
+///
+/// For example "This kind.of /input" becomes ".tmp.this_kind_of_input."
+fn escape_for_temp_file_name_prefix(text: &str) -> String {
Review Comment:
Done f3033c91a13716aaebad86e50b8f946bbe42ecb7
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#issuecomment-6035811306
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26110?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `96.55172%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.72%. Comparing base ([`b7c7bc2`](https://app.codecov.io/gh/apache/datafusion/commit/b7c7bc2c3930e58f55ea809b2e33755fc5241e65?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`30c457d`](https://app.codecov.io/gh/apache/datafusion/commit/30c457d795dcae75a64aa65fd5eea5cd29b50455?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26110?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/execution/src/disk\_manager.rs](https://app.codecov.io/gh/apache/datafusion/pull/26110?src=pr&el=tree&filepath=datafusion%2Fexecution%2Fsrc%2Fdisk_manager.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leGVjdXRpb24vc3JjL2Rpc2tfbWFuYWdlci5ycw==) | 96.55% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26110 +/- ##
==========================================
- Coverage 82.73% 82.72% -0.01%
==========================================
Files 1147 1147
Lines 449213 449240 +27
Branches 449213 449240 +27
==========================================
+ Hits 371634 371654 +20
- Misses 54929 54933 +4
- Partials 22650 22653 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26110?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin merged PR #26110:
URL: https://github.com/apache/datafusion/pull/26110
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#issuecomment-6095697527
@alamb Thank you so much for your time reviewing this MR. Sadly merge train has failed. I have rebased the MR using the GitHub button, may you try merging again :pray:
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#discussion_r4218798808
##########
datafusion/execution/src/disk_manager.rs:
##########
@@ -585,6 +586,32 @@ impl SpillFile for RefCountedTempFile {
}))
}
}
+
+/// Escape a string to be safe to use in a temp file name prefix.
+///
+/// All non-alphanumeric characters are replaced by `_` and characters are put in lowercase.
+/// A `.tmp.` is added at the beginning and a last `.` is set at the end.
+///
+/// For example "This kind.of /input" becomes ".tmp.this_kind_of_input."
+fn escape_for_temp_file_name_prefix(text: &str) -> String {
Review Comment:
Do you think we should also potentially cap the length of text too? like to 20 characters?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#issuecomment-6123242672
Hopefully we'll get it this time
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#issuecomment-6143552373
Gaa -- if I just keep clicking the button maybe it will go in 😢
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#issuecomment-6187465131
@blaginin @alamb thank you for going through the hassle of merging this MR
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#discussion_r4218890044
##########
datafusion/execution/src/disk_manager.rs:
##########
@@ -585,6 +586,32 @@ impl SpillFile for RefCountedTempFile {
}))
}
}
+
+/// Escape a string to be safe to use in a temp file name prefix.
+///
+/// All non-alphanumeric characters are replaced by `_` and characters are put in lowercase.
+/// A `.tmp.` is added at the beginning and a last `.` is set at the end.
+///
+/// For example "This kind.of /input" becomes ".tmp.this_kind_of_input."
+fn escape_for_temp_file_name_prefix(text: &str) -> String {
Review Comment:
Thank you! Definitely. 20 characters seems a bit small to me. Still quite short descriptions like `sort_merge_join_buffered_spill` will be truncated. What about something larger like 128 to avoid extremely long file names will still keeping the full description in most case?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#discussion_r4218930753
##########
datafusion/execution/src/disk_manager.rs:
##########
@@ -585,6 +586,32 @@ impl SpillFile for RefCountedTempFile {
}))
}
}
+
+/// Escape a string to be safe to use in a temp file name prefix.
+///
+/// All non-alphanumeric characters are replaced by `_` and characters are put in lowercase.
+/// A `.tmp.` is added at the beginning and a last `.` is set at the end.
+///
+/// For example "This kind.of /input" becomes ".tmp.this_kind_of_input."
+fn escape_for_temp_file_name_prefix(text: &str) -> String {
Review Comment:
128 sounds good to me
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#issuecomment-6177826928
ok, we have added a longer timeout to the queue, so hopeuflly we'll be able to get these through
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#issuecomment-6190190799
Thank you for your patience as we work through issues with the queue
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] DiskManager: include the description in file names [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #26110:
URL: https://github.com/apache/datafusion/pull/26110#discussion_r4218941873
##########
datafusion/execution/src/disk_manager.rs:
##########
@@ -585,6 +586,32 @@ impl SpillFile for RefCountedTempFile {
}))
}
}
+
+/// Escape a string to be safe to use in a temp file name prefix.
+///
+/// All non-alphanumeric characters are replaced by `_` and characters are put in lowercase.
+/// A `.tmp.` is added at the beginning and a last `.` is set at the end.
+///
+/// For example "This kind.of /input" becomes ".tmp.this_kind_of_input."
+fn escape_for_temp_file_name_prefix(text: &str) -> String {
Review Comment:
Perfect! Will implement that.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org