Repository: roapi/roapi (language: python)

Issue:
Error reading Parquet files from S3 with a space in the path.
Hi, I had a problem loading parquet file from s3 when there's a space in the path. I tried with `%20` but it doesn't work either.

Example path:  
```
s3://my-bucket/trusted/receita_socios/version=2021-10-30 09:00:56/my-file.parquet
```

Error message:
```
Error: Error loading Parquet: no parquet file found
```

The same file without space in the path works fine.


Discussion / hints:
hmm, I am not able to reproduce this, uri with space results in `Error: Invalid table URI` error, which is expected. Changing it to `%20` works for me with aws localstack. Sorry I don't have real AWS s3 bucket setup for testing. Could you try running it with `RUST_LOG=debug`?
Ran into a similar issue and I can replicate this using a real S3 bucket.

If you attempt to pass through the file's location _with_ spaces:

```sh
--table "test=s3://${S3_BUCKET}/folder with spaces/data.parquet"
```

…then `roapi` warns you:

```sh
Error: Invalid table URI:…Make sure it's URI encoded.
```

If you then URI-encode it:

```sh
--table "test=s3://${S3_BUCKET}/folder%20with%20spaces/data.parquet"
```

…then `roapi` parses the URI correctly but you run into the `no parquet file found` error.

Using `RUST_LOG=debug`, I _think_ I can see what's happening:

```sh
[2022-10-03T09:16:33Z DEBUG rusoto_core::request] Full request: 
     method: GET
     final_uri: …?list-type=2&prefix=folder%2520with%2520spaces%2Fdata.parquet%2F&start-after=folder%2520with%2520spaces%2Fdata.parquet%2F
    Headers:
```

The spaces, encoded as `%20`, are getting doubly-encoded by the time `rusoto_core::request` makes its call.

I've verified this locally by having `columnq::io::s3::parse_uri()` URL-decode the `key` and it then works as expected. However, I'm not sure if that's necessarily desired behaviour…?

Fix patch:
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index b21932115..3e57e3b04 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -153,6 +153,7 @@ jobs:
         $HOME/minio-binaries/mc alias set local http://127.0.0.1:9000 minioadmin minioadmin
         $HOME/minio-binaries/mc mb local/test-data
         $HOME/minio-binaries/mc cp test_data/blogs.parquet local/test-data
+        $HOME/minio-binaries/mc cp test_data/blogs.parquet "local/test-data/blogs space.parquet"
         $HOME/minio-binaries/mc cp test_data/blogs.parquet local/test-data/blogs/
     - name: Install fake gcs server (GCS)
       run: |
@@ -229,6 +230,7 @@ jobs:
         $HOME/minio-binaries/mc alias set local http://127.0.0.1:9000 minioadmin minioadmin
         $HOME/minio-binaries/mc mb local/test-data
         $HOME/minio-binaries/mc cp test_data/blogs.parquet local/test-data
+        $HOME/minio-binaries/mc cp test_data/blogs.parquet "local/test-data/blogs space.parquet"
         $HOME/minio-binaries/mc cp test_data/blogs.parquet local/test-data/blogs/
     - name: Install fake gcs server (GCS)
       run: |
diff --git a/Cargo.lock b/Cargo.lock
index 60e436774..93eaa9abe 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -951,6 +951,7 @@ dependencies = [
  "lazy_static",
  "log",
  "object_store",
+ "percent-encoding",
  "pretty_assertions",
  "regex",
  "reqwest",
diff --git a/columnq/Cargo.toml b/columnq/Cargo.toml
index 04135bb87..b97c8d098 100644
--- a/columnq/Cargo.toml
+++ b/columnq/Cargo.toml
@@ -17,6 +17,7 @@ arrow-schema = { version ="26", features = ["serde"] }
 
 datafusion = "14"
 object_store = { version = "0.5.3", features = ["aws_profile", "gcp", "azure"] }
+percent-encoding = "2.2.0"
 url = "2.2"
 
 log = "0"
diff --git a/columnq/src/io/object_store.rs b/columnq/src/io/object_store.rs
index f7fb7b95e..cea2cc661 100644
--- a/columnq/src/io/object_store.rs
+++ b/columnq/src/io/object_store.rs
@@ -1,5 +1,6 @@
 use futures::TryStreamExt;
 use std::str::FromStr;
+use percent_encoding;
 use url::Url;
 use crate::table::TableSource;
 use uriparse::URIReference;
@@ -57,9 +58,12 @@ where
     let client = object_store_provider.get_by_url(url)?;
     let mut partitions = vec![];
 
-    // first try loading table uri as single object
     // url.path starts with "/", but object_store does not expect "/" at the beginning
-    let path = object_store::path::Path::from(&url.path()[1..]);
+    // decode percent: https://github.com/apache/arrow-datafusion/pull/3750/files
+    let decoded_path = percent_encoding::percent_decode_str(&url.path()[1..]).decode_utf8_lossy();
+    let path = object_store::path::Path::from(decoded_path.as_ref());
+
+    // first try loading table uri as single object
     match partition_key_to_reader(client.clone(), &path).await {
         Ok(reader) => {
             partitions.push(partition_reader(reader)?);


Test patch:
diff --git a/test_end_to_end/query_blogs.sh b/test_end_to_end/query_blogs.sh
index 6f60ac9fb..e2e0ed5a7 100755
--- a/test_end_to_end/query_blogs.sh
+++ b/test_end_to_end/query_blogs.sh
@@ -10,6 +10,17 @@ else
     echo "success"
 fi
 
+http_status=$(curl -o /dev/null -s -w "%{http_code}" -X POST -d "SELECT count(1) from s3_blogs_space_encode" 127.0.0.1:8000/api/sql)
+
+echo $http_status
+if [[ $http_status != "200" ]]
+then
+    echo "error"
+    exit 1
+else
+    echo "success"
+fi
+
 http_status=$(curl -o /dev/null -s -w "%{http_code}" -X POST -d "SELECT count(1) from s3_blogs_dir" 127.0.0.1:8000/api/sql)
 
 echo $http_status
diff --git a/test_end_to_end/test_object_store_direct.yml b/test_end_to_end/test_object_store_direct.yml
index 8beac8cb2..6c3856dbc 100644
--- a/test_end_to_end/test_object_store_direct.yml
+++ b/test_end_to_end/test_object_store_direct.yml
@@ -10,6 +10,11 @@ tables:
       format: "parquet"
       # following line is different from test_end_to_end/test_s3_memory.yml
       use_memory_table: false
+  - name: "s3_blogs_space_encode"
+    uri: "s3://test-data/blogs%20space.parquet"
+    option:
+      format: "parquet"
+      use_memory_table: false
   - name: "s3_blogs_dir"
     uri: "s3://test-data/blogs/"
     option:
diff --git a/test_end_to_end/test_object_store_memory.yml b/test_end_to_end/test_object_store_memory.yml
index 35561ce49..6a03858bf 100644
--- a/test_end_to_end/test_object_store_memory.yml
+++ b/test_end_to_end/test_object_store_memory.yml
@@ -9,6 +9,11 @@ tables:
     option:
       format: "parquet"
       use_memory_table: true
+  - name: "s3_blogs_space_encode"
+    uri: "s3://test-data/blogs%20space.parquet"
+    option:
+      format: "parquet"
+      use_memory_table: true
   - name: "s3_blogs_dir"
     uri: "s3://test-data/blogs/"
     option: