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

Path separator fix windows #8345

Merged
merged 28 commits into from
Jul 27, 2023
Merged

Conversation

samansmink
Copy link
Contributor

@samansmink samansmink commented Jul 24, 2023

Changes

This PR fixes #7723 The problem was that the path separator function was static, which meant that path separator on windows would always be '' on windows which failed on hive partitioned writes.

To fix this, an API change to the FileSystem class was made: some methods are now no longer static. Also the PathSeparator call now requires a path to determine the path separator. This broke the spatial extension, so I decided to also added the same patching mechanism we added for duckdb-wasm in #7926 Check out the READMEs I added in .github/patches/*/README.md for details on the patching mechanism. TLDR; when adding change to duckdb that breaks either an extension built with duckdb CI or duckdb-wasm, you can add a patch to ensure duckdb CI passes, enabling a cleaner workflow.

I tried setting up the test server to catch this issue in CI, but there's a whole world of hurt in setting up the minio on windows ci too which would just get a bit too messy. I left it out of this PR for that reason. I did test this on windows manually and have confirmed it now works.

Follow ups

  1. For testing s3 on windows, I think we should set up a job in nightly which has a real S3 token to some test bucket we own. Then this can run actualy tests against S3. As long as we keep tests reasonable this would allow actually testing this from all platforms and also have the benefit of testing against AWS S3 itself. Other services like Azure can then also be tested in similar fashion

  2. The patch should be applied to the spatial extension.

@Mause
Copy link
Member

Mause commented Jul 24, 2023

Are you able to fix this for fsspec as well while you're at it? There's a test that should fail if it's worked

@Mause
Copy link
Member

Mause commented Jul 24, 2023

Specifically this one:

if sys.platform == 'win32' else

@samansmink samansmink marked this pull request as draft July 24, 2023 09:20
@samansmink samansmink marked this pull request as ready for review July 25, 2023 18:52
@github-actions github-actions bot marked this pull request as draft July 25, 2023 20:28
@samansmink samansmink marked this pull request as ready for review July 25, 2023 20:28
@github-actions github-actions bot marked this pull request as draft July 26, 2023 08:41
@samansmink samansmink marked this pull request as ready for review July 26, 2023 08:41
@samansmink samansmink requested a review from Mytherin July 27, 2023 07:50
@Mytherin Mytherin merged commit 811683f into duckdb:master Jul 27, 2023
68 of 70 checks passed
@Mytherin
Copy link
Collaborator

Looks great, thanks!

samansmink added a commit to samansmink/duckdb_spatial that referenced this pull request Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node.js uses backslashes when doing copy....to s3 parquet files on windows machine (partitions don't work)
3 participants