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

feat(rust)!: use BoxedError in Error::IO #2329

Merged
merged 16 commits into from
May 20, 2024
Merged

Conversation

broccoliSpicy
Copy link
Contributor

@broccoliSpicy broccoliSpicy commented May 13, 2024

Closes #2328

currently, lots of error in our codebase are wrapped in Error::IO, some are not accurate. like this one:
source example

            return Err(Error::IO {
                message: "HashJoiner: No data".to_string(),
                location: location!(),
            });

this PR still wraps this error in Error::IO, as we haven't define more precise Error type for HashJoiner yet.

we may need to define our own Error enum in each of our submodule, lance-encoding::Error, lance-file::Error, lance-index::Error etc.
so we can write error return like this:

            let err = lance_core::Error::from(lance::dataset::Error::HashJoinerError {
                message: "HashJoiner: No data".to_string(),
                location: location!(),
            });
            Err(err)

Copy link

ACTION NEEDED

Lance 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.

For details on the error please inspect the "PR Title Check" action.

@broccoliSpicy broccoliSpicy changed the title enhancement: use BoxedError in Error::IO enhancement: use BoxedError in Error::IO(draft, for discussion only) May 13, 2024
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

I don't really see the point of creating lance-file::Error (and others) and wrapping it in Error::IO. The wrapping errors is more for wrapping our dependencies errors like those from Arrow, ObjectStore, and DataFusion.

rust/lance-file/src/format/metadata.rs Outdated Show resolved Hide resolved
@broccoliSpicy broccoliSpicy changed the title enhancement: use BoxedError in Error::IO(draft, for discussion only) chore: use BoxedError in Error::IO(draft, for discussion only) May 13, 2024
@github-actions github-actions bot added the chore label May 13, 2024
@broccoliSpicy broccoliSpicy changed the title chore: use BoxedError in Error::IO(draft, for discussion only) chore: use BoxedError in Error::IO May 13, 2024
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

This is looking cleaner. Got two small suggestions and then I think this is ready to go :)

Comment on lines 759 to 763
// TODO: Check the correct way to construct a object_store::Error::Generic
let err = lance_core::Error::from(object_store::Error::Generic {
store: "",
source: format!("Unsupported URI scheme: {}", unknow_scheme).into(),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd maybe go with ObjectStore::NotSupported here instead:

Suggested change
// TODO: Check the correct way to construct a object_store::Error::Generic
let err = lance_core::Error::from(object_store::Error::Generic {
store: "",
source: format!("Unsupported URI scheme: {}", unknow_scheme).into(),
});
let err = lance_core::Error::from(object_store::Error::NotSupported {
source: format!("Unsupported URI scheme: {}", unknow_scheme).into(),
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, I forget this two TODO comments, it's removed now, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you missed my actual change here, which is to use Error::NotSupported instead of Error::Generic for this spot. Same with the other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ha, I totally missed your comment, fixed.

Comment on lines 386 to 393
unknow_scheme => {
// TODO: Check the correct way to construct a object_store::Error::Generic
let err = lance_core::Error::from(object_store::Error::Generic {
store: "",
source: format!("Unsupported URI scheme: {}", unknow_scheme).into(),
});
Err(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here:

Suggested change
unknow_scheme => {
// TODO: Check the correct way to construct a object_store::Error::Generic
let err = lance_core::Error::from(object_store::Error::Generic {
store: "",
source: format!("Unsupported URI scheme: {}", unknow_scheme).into(),
});
Err(err)
}
unknown_scheme => {
Err(lance_core::Error::from(object_store::Error::NotSupported {
source: format!("Unsupported URI scheme: {}", unknown_scheme).into(),
}))
}

@wjones127 wjones127 changed the title chore: use BoxedError in Error::IO feat(rust)!: use BoxedError in Error::IO May 16, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2024

Codecov Report

Attention: Patch coverage is 10.72089% with 483 lines in your changes are missing coverage. Please review.

Project coverage is 80.79%. Comparing base (711ac03) to head (4d1ae1a).

Files Patch % Lines
rust/lance-table/src/io/commit/dynamodb.rs 0.00% 69 Missing ⚠️
rust/lance/src/io/exec/planner.rs 0.00% 56 Missing and 1 partial ⚠️
rust/lance/src/dataset/scanner.rs 17.85% 42 Missing and 4 partials ⚠️
rust/lance/src/utils/tfrecord.rs 0.00% 29 Missing ⚠️
rust/lance/src/index/vector/ivf/io.rs 0.00% 28 Missing ⚠️
rust/lance-file/src/writer.rs 0.00% 19 Missing and 3 partials ⚠️
rust/lance/src/dataset/fragment.rs 29.62% 19 Missing ⚠️
rust/lance/src/io/exec/knn.rs 0.00% 16 Missing and 2 partials ⚠️
rust/lance/src/index/vector/ivf.rs 0.00% 15 Missing and 1 partial ⚠️
rust/lance-io/src/object_writer.rs 0.00% 10 Missing and 4 partials ⚠️
... and 24 more
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2329   +/-   ##
=======================================
  Coverage   80.79%   80.79%           
=======================================
  Files         192      192           
  Lines       56213    56212    -1     
  Branches    56213    56212    -1     
=======================================
+ Hits        45416    45417    +1     
- Misses       8128     8137    +9     
+ Partials     2669     2658   -11     
Flag Coverage Δ
unittests 80.79% <10.72%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wjones127 wjones127 added enhancement New feature or request breaking-change and removed chore labels May 16, 2024
@wjones127
Copy link
Contributor

@broccoliSpicy there seem to be some lint / syntax errors that need to be fixed.

@broccoliSpicy
Copy link
Contributor Author

@broccoliSpicy there seem to be some lint / syntax errors that need to be fixed.

thank you so much @wjones127 ! fixed lint locally but I will see how github CI goes

@broccoliSpicy
Copy link
Contributor Author

broccoliSpicy commented May 17, 2024

sorry this PR has awfully too many commits, I have issue with my local cargo fmt and kind of used github CI to manually fix lint a few times, I can do a force-push if that's needed

@wjones127 wjones127 merged commit 7e8e5c7 into lancedb:main May 20, 2024
19 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Error::IO variant to hold BoxError, so it could be downcast if needed
3 participants