feat: upgrade url crate, rework temporary directory utils#4860
feat: upgrade url crate, rework temporary directory utils#4860westonpace merged 14 commits intolance-format:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4860 +/- ##
=======================================
Coverage 80.87% 80.87%
=======================================
Files 332 333 +1
Lines 131687 131690 +3
Branches 131687 131690 +3
=======================================
+ Hits 106507 106510 +3
- Misses 21430 21441 +11
+ Partials 3750 3739 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
westonpace
force-pushed
the
feat/upgrade-url
branch
2 times, most recently
from
4cce53f to
483f0d6
Compare
|
What is the current state of using slashes in paths on Windows? It used to be that you were supposed to use https://doc.rust-lang.org/std/path/constant.MAIN_SEPARATOR.html but I don't know if we still want to do that. |
westonpace
force-pushed
the
feat/upgrade-url
branch
from
06f4abe to
9712519
Compare
The following should work (even on Windows):
The following will fail:
So my recommendation would be to always use |
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
thank you so much for looking into this 🙏 🙏 🙏
| # Hack for Windows due to | ||
| # https://github.com/apache/arrow-rs-object-store/issues/499 | ||
| dst_dataset_uri = dst_dataset_uri.replace("\\", "/", 1) | ||
| print(f"dst_dataset_uri={dst_dataset_uri}") |
| /// It is useful when you need to create a temporary file that is only used as a standard library path. | ||
| #[derive(Default)] | ||
| pub struct TempStdFile { | ||
| _tempfile: TempFile, |
There was a problem hiding this comment.
nit: since this variable is used, this should not have underscore prefix? (similar comment for other _tempfile and _tempdir
There was a problem hiding this comment.
Some of them really are use but TempStdFile and TempStdPath I modified to remove the underscore.
…it tests while object_store gets things sorted out
…lesome Windows paths
westonpace
force-pushed
the
feat/upgrade-url
branch
from
9712519 to
33dd234
Compare
…at#4860) There is a [behavior change](servo/rust-url#1077) in url 2.5.7 that is blocking a datafusion upgrade. This PR works around that behavior change. Unfortunately, the fix here is to just avoid problematic Windows paths in our tests while we wait for guidance from upstream. It will still be possible for users to encounter errors when providing Windows paths. We can look at hacks for that problem in a future PR (or just hope the upstream bug is fixed promptly).
…at#4860) There is a [behavior change](servo/rust-url#1077) in url 2.5.7 that is blocking a datafusion upgrade. This PR works around that behavior change. Unfortunately, the fix here is to just avoid problematic Windows paths in our tests while we wait for guidance from upstream. It will still be possible for users to encounter errors when providing Windows paths. We can look at hacks for that problem in a future PR (or just hope the upstream bug is fixed promptly).
4 participants
There is a behavior change in url 2.5.7 that is blocking a datafusion upgrade. This PR works around that behavior change. Unfortunately, the fix here is to just avoid problematic Windows paths in our tests while we wait for guidance from upstream. It will still be possible for users to encounter errors when providing Windows paths. We can look at hacks for that problem in a future PR (or just hope the upstream bug is fixed promptly).