refactor: use lance-io object store for dir namespace and improve builder#5045
refactor: use lance-io object store for dir namespace and improve builder#5045jackye1995 merged 6 commits intolance-format:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }; | ||
|
|
||
| // Create the Lance dataset using the actual Lance API | ||
| Dataset::write(reader, &table_path, Some(write_params)) | ||
| Dataset::write(reader, &table_uri, Some(write_params)) | ||
| .await |
There was a problem hiding this comment.
The refactor builds an ObjectStore with the provided storage.* properties but create_table now calls Dataset::write(reader, &table_uri, Some(write_params)) without forwarding those options (write_params is left at the default other than mode). As a result, the storage options collected in DirectoryNamespaceConfig are ignored during dataset creation. For remote backends that depend on those options (custom S3 endpoints, credentials, regions, etc.), Dataset::write will initialize a new object store without the necessary configuration and the table creation will fail. Consider populating WriteParams::store_params (or otherwise reusing the initialized object store) so writes inherit the configured storage options.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5045 +/- ##
==========================================
- Coverage 81.74% 81.70% -0.04%
==========================================
Files 340 340
Lines 137505 138554 +1049
Branches 137505 138554 +1049
==========================================
+ Hits 112397 113208 +811
- Misses 21370 21618 +248
+ Partials 3738 3728 -10
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:
|
jackye1995
changed the title
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
LGTM while all CI passing. But I think the aws-* crates is not introduced by opendal.
|
Oh yeah that's a separated issue, not introduced by Opendal, let me clarify in the description |
|
Known flaky test #4925 |
…lder (lance-format#5045) I ended up doing these in lance-format#4984 and lance-format#4905 so I decided to pull it out and get it cleaned up first. This PR moves the directory namespace from using OpenDAL directly to using Lance ObjectStore. This avoids the inconsistency between the dir namespace and the underlying lance table storage configurations. User can still use OpenDAL, and if we fully migrate Lance to OpenDAL it will be applied to both layers at the same time as well. The PR also improves the builder of the namespaces with builder style and allow supplying a Lance session. Since we have not published a stable version yet, we do not care about backwards compatibility. This PR also ensures the lance-namespace-impls features are consistent with lance-io features. Related to lance-format#5042
3 participants
I ended up doing these in #4984 and #4905 so I decided to pull it out and get it cleaned up first.
This PR moves the directory namespace from using OpenDAL directly to using Lance ObjectStore. This avoids the inconsistency between the dir namespace and the underlying lance table storage configurations. User can still use OpenDAL, and if we fully migrate Lance to OpenDAL it will be applied to both layers at the same time as well.
The PR also improves the builder of the namespaces with builder style and allow supplying a Lance session. Since we have not published a stable version yet, we do not care about backwards compatibility.
This PR also ensures the lance-namespace-impls features are consistent with lance-io features. Related to #5042