fix(python): use resolving storage for python binding#2246
fix(python): use resolving storage for python binding#2246CTTY wants to merge 3 commits intoapache:mainfrom
Conversation
geruh
left a comment
geruh
left a comment
There was a problem hiding this comment.
This looks like a great addition for the python side! Looks like the cargo file only brings in s3, fs, and memory here but the resolving factory lists more storage backends from opendal.
iceberg-rust/bindings/python/Cargo.toml
Line 36 in 1817c98
Good point! will fix it |
geruh
left a comment
geruh
left a comment
There was a problem hiding this comment.
LGTM! All tests pass, built the binding and verified it works e2e with PyIceberg, and file scheme.
| .map_err(|e| PyRuntimeError::new_err(format!("Invalid table identifier: {e}")))?; | ||
|
|
||
| let factory = storage_factory_from_path(&metadata_location)?; | ||
| let factory = Arc::new(OpenDalResolvingStorageFactory::new()); |
There was a problem hiding this comment.
love it! its like java's ResolvingFileIO
There was a problem hiding this comment.
maybe we can start exporting this as another FileIO for pyiceberg
| arrow = { version = "57.1", features = ["pyarrow", "chrono-tz"] } | ||
| iceberg = { path = "../../crates/iceberg" } | ||
| iceberg-storage-opendal = { path = "../../crates/storage/opendal", features = ["opendal-s3", "opendal-fs", "opendal-memory"] } | ||
| iceberg-storage-opendal = { path = "../../crates/storage/opendal", features = ["opendal-memory", "opendal-fs", "opendal-s3", "opendal-gcs", "opendal-oss", "opendal-azdls"] } |
There was a problem hiding this comment.
👍
There was a problem hiding this comment.
(actually can we just use opendal-all here?)
3 participants
Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?