-
Notifications
You must be signed in to change notification settings - Fork 221
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
chore: part 1/N of refactoring vector index into separate crate #1388
Conversation
@@ -382,8 +382,8 @@ impl Dataset { | |||
let m = d.manifest.as_ref(); | |||
if schema != m.schema { | |||
return Err(Error::SchemaMismatch { | |||
original: m.schema.clone(), |
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.
Gonna re-eanble these after we cleanly migrate schema
to lance-core
.await?; | ||
.await | ||
.map_err(|e| Error::IO { | ||
message: format!("dynamodb error: {}", e,), |
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.
can you keep the source error?
dynamodb formatter hides the read error if we only print {}
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.
349 | .map_err(|e| Error::IO {
| - binding `e` declared here
350 | message: format!("dynamodb error: {}, source: {:?}", e, e.into_source()),
| -----------------------------------------------^---------------
| | | |
| | | move out of `e` occurs here
| | borrow of `e` occurs here
| borrow later used here
did i do sth wrong here?
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.
We will have the From<SDKError> for Error
back once this code is migrated to lance-core
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.
I think these changes are good. Will defer to @chebbyChefNEQ on the impact of the dynamo errors. I think the arrow schema error being temporarily disabled is probably fine.
/// IVF - IVF file partition | ||
/// | ||
#[derive(Debug, Clone)] | ||
pub struct Ivf { |
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.
This is duplicating the Ivf
struct that's in the lance
crate right? Or was this supposed to be a move?
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.
I am going to move the IVF model from lance to here. Wanted to get this in before it becomes too big.
No description provided.