[HSTACK] delta engine injection support#17
Conversation
aditanase
left a comment
aditanase
left a comment
There was a problem hiding this comment.
2 requests:
- it should work with the old provider too, not just the new one
- we should make sure this works especially during metadata load operations. this is the main entrypoint that we care about
- we should add a builder method similar to the log size limiter, not rely on extensions - so maybe inject it via
DeltaTableConfig?
cdobre
left a comment
cdobre
left a comment
There was a problem hiding this comment.
I think we also need to support injecting engine in LogStore::engine ( crates/core/src/logstore/mod.rs ) which is used when loading snapshots
aditanase
left a comment
aditanase
left a comment
There was a problem hiding this comment.
👍 with minor comments
| ) -> DeltaResult<Self> { | ||
| // TODO: bundle operation_id with logstore ... | ||
| let engine = log_store.engine(None); | ||
| let engine = config.engine.as_ref().map(|e| e.0.clone()) |
There was a problem hiding this comment.
use the engine fn you just added?
There was a problem hiding this comment.
we don't have self here, but found a good alternative
| } | ||
|
|
||
| /// Returns the configured engine, falling back to the log store's default engine. | ||
| pub fn engine(&self, log_store: &dyn LogStore) -> Arc<dyn Engine> { |
There was a problem hiding this comment.
I'm not sure about this one - if I don't invoke with the same log_store both when doing try_new or engine I could get different results right?
I'm ok with having this a private method for simplifying the rest of methods, but I'm not sure it makes a lot of sense as public
There was a problem hiding this comment.
You're right, i made it pub(crate)
| return Ok(()); | ||
| } | ||
|
|
||
| let engine = self.snapshot.engine(log_store); |
There was a problem hiding this comment.
is this the reason for having public?
VLCDaniel
changed the title
|
ACTION NEEDED delta-rs follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
3 participants
No description provided.