-
Notifications
You must be signed in to change notification settings - Fork 817
fix(io): normalize OpenDAL listed paths #8654
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
1d7f8e3
fix(io): normalize OpenDAL listed paths
lance-gatefixer[bot] 3247cf1
fix(io): preserve escaped OpenDAL list paths
lance-gatefixer[bot] 44c8958
Merge remote-tracking branch 'origin/main' into gatekeeper/fix-8652-1
lance-gatefixer[bot] 6b75b1a
Merge remote-tracking branch 'origin/main' into gatekeeper/fix-8652-1
lance-gatefixer[bot] fdddb25
Merge remote-tracking branch 'origin/main' into gatekeeper/fix-8652-1
lance-gatefixer[bot] 8eec1e2
Merge branch 'main' into gatekeeper/fix-8652-1
Xuanwo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,226 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // SPDX-FileCopyrightText: Copyright The Lance Authors | ||
|
|
||
| use std::fmt; | ||
| use std::ops::Range; | ||
|
|
||
| use async_trait::async_trait; | ||
| use bytes::Bytes; | ||
| use futures::{StreamExt, TryStreamExt, future, stream::BoxStream}; | ||
| use object_store::path::Path; | ||
| use object_store::{ | ||
| CopyOptions, GetOptions, GetResult, ListResult, MultipartUpload, ObjectMeta, | ||
| ObjectStore as OSObjectStore, PutMultipartOptions, PutOptions, PutPayload, PutResult, | ||
| RenameOptions, | ||
| }; | ||
| use object_store_opendal::OpendalStore as InnerOpendalStore; | ||
| use opendal::Operator; | ||
|
|
||
| /// Adapts OpenDAL listing paths to the spelling used by the request. | ||
| /// | ||
| /// The upstream bridge builds listed locations with [`Path::from`], which | ||
| /// percent-encodes reserved characters. Lance builds dataset base paths with | ||
| /// [`Path::from_url_path`], so mismatched listed locations must be decoded. | ||
| /// Locations that already match the requested prefix retain their spelling to | ||
| /// preserve paths containing literal percent escapes. | ||
| #[derive(Debug, Clone)] | ||
| pub(super) struct OpendalStore { | ||
| inner: InnerOpendalStore, | ||
| } | ||
|
|
||
| impl OpendalStore { | ||
| pub(super) fn new(operator: Operator) -> Self { | ||
| Self { | ||
| inner: InnerOpendalStore::new(operator), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for OpendalStore { | ||
| fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| self.inner.fmt(formatter) | ||
| } | ||
| } | ||
|
|
||
| fn normalize_location(location: &Path, prefix: Option<&Path>) -> object_store::Result<Path> { | ||
| if prefix.is_none_or(|prefix| location.prefix_matches(prefix)) { | ||
| return Ok(location.clone()); | ||
| } | ||
|
|
||
| Path::from_url_path(location.as_ref()).map_err(|source| object_store::Error::Generic { | ||
| store: "OpendalStore", | ||
| source: Box::new(source), | ||
| }) | ||
| } | ||
|
|
||
| fn normalize_object_meta( | ||
| mut meta: ObjectMeta, | ||
| prefix: Option<&Path>, | ||
| ) -> object_store::Result<ObjectMeta> { | ||
| meta.location = normalize_location(&meta.location, prefix)?; | ||
| Ok(meta) | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl OSObjectStore for OpendalStore { | ||
| async fn put_opts( | ||
| &self, | ||
| location: &Path, | ||
| payload: PutPayload, | ||
| options: PutOptions, | ||
| ) -> object_store::Result<PutResult> { | ||
| self.inner.put_opts(location, payload, options).await | ||
| } | ||
|
|
||
| async fn put_multipart_opts( | ||
| &self, | ||
| location: &Path, | ||
| options: PutMultipartOptions, | ||
| ) -> object_store::Result<Box<dyn MultipartUpload>> { | ||
| self.inner.put_multipart_opts(location, options).await | ||
| } | ||
|
|
||
| async fn get_opts( | ||
| &self, | ||
| location: &Path, | ||
| options: GetOptions, | ||
| ) -> object_store::Result<GetResult> { | ||
| self.inner.get_opts(location, options).await | ||
| } | ||
|
|
||
| async fn get_ranges( | ||
| &self, | ||
| location: &Path, | ||
| ranges: &[Range<u64>], | ||
| ) -> object_store::Result<Vec<Bytes>> { | ||
| self.inner.get_ranges(location, ranges).await | ||
| } | ||
|
|
||
| fn delete_stream( | ||
| &self, | ||
| locations: BoxStream<'static, object_store::Result<Path>>, | ||
| ) -> BoxStream<'static, object_store::Result<Path>> { | ||
| self.inner.delete_stream(locations) | ||
| } | ||
|
|
||
| fn list(&self, prefix: Option<&Path>) -> BoxStream<'static, object_store::Result<ObjectMeta>> { | ||
| let listed = self.inner.list(prefix); | ||
| let prefix = prefix.cloned(); | ||
| listed | ||
| .map(move |result| result.and_then(|meta| normalize_object_meta(meta, prefix.as_ref()))) | ||
| .boxed() | ||
| } | ||
|
|
||
| fn list_with_offset( | ||
| &self, | ||
| prefix: Option<&Path>, | ||
| offset: &Path, | ||
| ) -> BoxStream<'static, object_store::Result<ObjectMeta>> { | ||
| if self.inner.info().capability().list_with_start_after { | ||
| let listed = self.inner.list_with_offset(prefix, offset); | ||
| let prefix = prefix.cloned(); | ||
| listed | ||
| .map(move |result| { | ||
| result.and_then(|meta| normalize_object_meta(meta, prefix.as_ref())) | ||
| }) | ||
| .boxed() | ||
| } else { | ||
| // The bridge's fallback compares its encoded output with the raw | ||
| // offset. Filter normalized locations so both sides use one form. | ||
| let offset = offset.clone(); | ||
| self.list(prefix) | ||
| .try_filter(move |meta| future::ready(meta.location > offset)) | ||
| .boxed() | ||
| } | ||
| } | ||
|
|
||
| async fn list_with_delimiter(&self, prefix: Option<&Path>) -> object_store::Result<ListResult> { | ||
| let mut result = self.inner.list_with_delimiter(prefix).await?; | ||
| for object in &mut result.objects { | ||
| object.location = normalize_location(&object.location, prefix)?; | ||
| } | ||
| for common_prefix in &mut result.common_prefixes { | ||
| *common_prefix = normalize_location(common_prefix, prefix)?; | ||
| } | ||
| Ok(result) | ||
| } | ||
|
|
||
| async fn copy_opts( | ||
| &self, | ||
| from: &Path, | ||
| to: &Path, | ||
| options: CopyOptions, | ||
| ) -> object_store::Result<()> { | ||
| self.inner.copy_opts(from, to, options).await | ||
| } | ||
|
|
||
| async fn rename_opts( | ||
| &self, | ||
| from: &Path, | ||
| to: &Path, | ||
| options: RenameOptions, | ||
| ) -> object_store::Result<()> { | ||
| self.inner.rename_opts(from, to, options).await | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use bytes::Bytes; | ||
| use futures::TryStreamExt; | ||
| use object_store::ObjectStoreExt; | ||
| use opendal::services::Memory; | ||
| use rstest::rstest; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[rstest] | ||
| #[case::raw_reserved_character("tables/run~1/t.lance")] | ||
| #[case::literal_percent_escape("tables/run%25231/t.lance")] | ||
| #[tokio::test] | ||
| async fn test_list_preserves_request_path_spelling(#[case] base_url: &str) { | ||
| let operator = Operator::new(Memory::default()).unwrap(); | ||
| let store = OpendalStore::new(operator); | ||
| let base = Path::from_url_path(base_url).unwrap(); | ||
| let direct_location = base.clone().join("manifest.lance"); | ||
| let nested_location = Path::from_url_path(format!("{base_url}/data/part.lance")).unwrap(); | ||
| for location in [&direct_location, &nested_location] { | ||
| store | ||
| .put(location, Bytes::from_static(b"data").into()) | ||
| .await | ||
| .unwrap(); | ||
| } | ||
|
|
||
| let listed = store | ||
| .list(Some(&base)) | ||
| .try_collect::<Vec<_>>() | ||
| .await | ||
| .unwrap(); | ||
| let mut listed_locations = listed | ||
| .into_iter() | ||
| .map(|meta| meta.location) | ||
| .collect::<Vec<_>>(); | ||
| listed_locations.sort(); | ||
| let mut expected_locations = vec![direct_location.clone(), nested_location.clone()]; | ||
| expected_locations.sort(); | ||
| assert_eq!(listed_locations, expected_locations); | ||
| assert!( | ||
| listed_locations | ||
| .iter() | ||
| .all(|location| location.prefix_matches(&base)) | ||
| ); | ||
|
|
||
| let listed_after_nested = store | ||
| .list_with_offset(Some(&base), &nested_location) | ||
| .try_collect::<Vec<_>>() | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(listed_after_nested.len(), 1); | ||
| assert_eq!(listed_after_nested[0].location, direct_location); | ||
|
|
||
| let delimited = store.list_with_delimiter(Some(&base)).await.unwrap(); | ||
| assert_eq!(delimited.objects.len(), 1); | ||
| assert_eq!(delimited.objects[0].location, direct_location); | ||
| assert_eq!(delimited.common_prefixes, vec![base.clone().join("data")]); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unconditionally decoding the returned location breaks a path spelling that works before this change. A URI segment
run%25231produces the logicalPathrun%231;InnerOpendalStoredecodes requests to physicalrun#1and, on the base revision, re-encodes that listing asrun%231, so it still matches the logical base. This adapter decodes it again torun#1, andprefix_matches(run%231)now fails. Relative-listing consumers then receive the absolute location, and cleanup can misclassify a referenced file when that location begins with a managed subtree such asdata.The mapping must preserve existing double-encoded URI paths while normalizing the reserved-character mismatch. One bounded option is to retain the upstream spelling when it already matches the requested prefix and decode only a mismatch; a full bidirectional translation needs a compatibility path for keys written through the current bridge. Please cover a literal percent-escape case.
Reproducer
I inserted this temporary test in the new adapter test module:
Run with:
Observed:
1 passed; 0 failed. The base listing matchedrun%231; the adapter listing becamerun#1and did not match.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 3247cf1: the adapter now preserves upstream locations that already match the requested prefix and decodes only mismatches. Parameterized regressions cover raw reserved characters and literal percent escapes across recursive, offset, and delimiter listings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3247cf181: prefix-aware normalization now retains upstream locations that already match the request, so raw reserved characters and literal percent escapes both preserve the requested spelling across recursive, offset, and delimiter listings. Both parameterized regressions pass on the current head.