Skip to content

feat: add Error::External variant for preserving user errors #5606

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.

Sign up for GitHub

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

Jump to bottom
Open
wjones127 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: add Error::External variant for preserving user errors #5606

wjones127 wants to merge 2 commits into from

Conversation

@wjones127
Copy link
Contributor

@wjones127 wjones127 commented Dec 31, 2025

Summary

  • Add Error::External variant with #[snafu(transparent)] to preserve user errors passed through streams
  • Update From<ArrowError> to extract inner error from ExternalError variant
  • Update From<DataFusionError> to handle External and nested ArrowError::ExternalError
  • Add helper methods: external(), external_source(), into_external()

Test plan

  • Unit tests for error conversions in lance-core
  • Integration tests for InsertBuilder::execute_stream and MergeInsertJob::execute_reader

🤖 Generated with Claude Code

@wjones127 @claude
feat: add Error::External variant for preserving user errors
125e2bf 
When users pass streams with custom error types (e.g., wrapped in
ArrowError::ExternalError), the original error was being converted to
a string and lost. This adds an Error::External variant that preserves
the boxed error, allowing users to downcast and recover their original
error type.

Changes:
- Add #[snafu(transparent)] External variant to Error enum
- Update From<ArrowError> to extract inner error from ExternalError
- Update From<DataFusionError> to handle External and nested cases
- Add helper methods: external(), external_source(), into_external()

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@github-actions github-actions bot added the enhancement New feature or request label Dec 31, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 31, 2025

PR Review: feat: add Error::External variant for preserving user errors

This PR adds functionality to preserve user-defined errors passed through Lance APIs. The implementation is clean and the test coverage is comprehensive.

No blocking issues found

The code quality is good, error handling is consistent with existing patterns, and tests cover the key scenarios.

Minor observations (non-blocking)

  1. From<Error> for DataFusionError - The existing conversion at line 354-357 converts all Lance errors to DataFusionError::Execution(e.to_string()), which loses the External variant's recoverability on the return path. This is likely acceptable since users typically extract errors from Lance, not the reverse, but worth noting for completeness.

  2. Documentation - The doc comments on the new methods are clear and helpful. The examples in the tests effectively demonstrate the intended usage pattern.

Overall this is a well-designed addition that addresses a real user need for error recovery in streaming APIs.

@wjones127 @claude
feat: enable lance_core::Error round-trip through Arrow/DataFusion
f24fdc3 
When a lance_core::Error is converted to ArrowError or DataFusionError
and then back, the original error type is now recovered via downcasting
instead of being wrapped in External or converted to strings.

This enables workflows where iterators/streams use lance_core::Error
internally but go through Arrow/DataFusion error types at API boundaries.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@codecov
Copy link

codecov bot commented Dec 31, 2025

Codecov Report

❌ Patch coverage is 88.41699% with 30 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-core/src/error.rs 88.96% 17 Missing ⚠️
rust/lance/src/dataset/write/insert.rs 87.87% 4 Missing and 4 partials ⚠️
rust/lance/src/dataset/write/merge_insert.rs 87.17% 3 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

@wjones127 wjones127 marked this pull request as ready for review December 31, 2025 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@wjones127