Skip to content

fix: reduce verbosity of errors due to string conversion#5600

Merged
wjones127 merged 1 commit intolance-format:mainfrom
wjones127:fix/no-error-strings
Jan 13, 2026
Merged

fix: reduce verbosity of errors due to string conversion#5600
wjones127 merged 1 commit intolance-format:mainfrom
wjones127:fix/no-error-strings

Conversation

@wjones127
Copy link
Copy Markdown
Contributor

  • Eliminates cases where we needlessly convert errors to strings.
  • Uses DataFusionError::ExecutionJoin() when appropriate.

@github-actions github-actions Bot added the bug Something isn't working label Dec 31, 2025
@github-actions
Copy link
Copy Markdown
Contributor

Code Review

This PR improves error handling by preserving error context rather than converting to strings. The changes are straightforward and follow existing patterns in the codebase.

P1: Consider using Error::Internal instead of Error::Stop as dummy value

In rust/lance-index/src/vector/ivf/shuffler.rs:

Some(Err(err)) => {
    // Using Error::Stop as dummy value to take the error out.
    return Err(std::mem::replace(err, Error::Stop));
}

While this pattern exists in builder.rs, using Error::Stop (which semantically means "stream early stop") as a placeholder after extracting the real error is a code smell. The value left behind after mem::replace should never be observed, but if it somehow is, Error::Stop could cause confusing behavior compared to Error::Internal { message: "error consumed", .. }.

This is a minor concern since the pattern is already established, but worth noting for future consideration.

All changes look correct

  • Using DataFusionError::ExecutionJoin for JoinError is appropriate
  • Using DataFusionError::External for Lance errors and DataFusionError::ArrowError for Arrow errors preserves type information
  • The ? operator changes correctly leverage existing From implementations
  • The empty stream error is now correctly typed as InvalidInput rather than generic IO error

No blocking issues.

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 31, 2025

@wjones127 wjones127 marked this pull request as ready for review December 31, 2025 17:10
@wjones127 wjones127 merged commit db9540e into lance-format:main Jan 13, 2026
30 checks passed
@wjones127 wjones127 deleted the fix/no-error-strings branch January 13, 2026 01:15
jackye1995 pushed a commit to jackye1995/lance that referenced this pull request Jan 21, 2026
…t#5600)

* Eliminates cases where we needlessly convert errors to strings.
* Uses `DataFusionError::ExecutionJoin()` when appropriate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants