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

Check an object store is on local file system #918

Merged
merged 6 commits into from
May 30, 2023
Merged

Check an object store is on local file system #918

merged 6 commits into from
May 30, 2023

Conversation

eddyxu
Copy link
Contributor

@eddyxu eddyxu commented May 30, 2023

No description provided.

@eddyxu eddyxu requested a review from wjones127 May 30, 2023 23:01
@eddyxu eddyxu requested a review from gsilvestrin May 30, 2023 23:01
@@ -41,7 +41,7 @@ use super::object_reader::ObjectReader;
pub struct ObjectStore {
// Inner object store
pub inner: Arc<dyn OSObjectStore>,
scheme: String,
pub scheme: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be public if we have is_local?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i can make it private for now

@@ -147,6 +147,11 @@ impl ObjectStore {
}
}

/// Returns true if the object store pointed to a local file system.
pub fn is_local(&self) -> bool {
self.scheme == "file"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_from_path creates a store with scheme flle - I never figured out why, but can this be a problem here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_from_path creates a local object store, right? that's the correct expectation?

Copy link
Contributor

@gsilvestrin gsilvestrin May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean there is a type there - flle instead of file, but I don't know why:

scheme: String::from("flle"),

Copy link
Contributor Author

@eddyxu eddyxu May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, this appears to be a typo. lemme fix in this PR then

Copy link
Contributor

@gsilvestrin gsilvestrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eddyxu eddyxu merged commit 4ceb590 into main May 30, 2023
@eddyxu eddyxu deleted the lei/lancedb_fix branch May 30, 2023 23:52
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.

3 participants