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

fix: fail fast for opening non-existent path #1917

Merged

Conversation

dimonchik-suvorov
Copy link
Contributor

@dimonchik-suvorov dimonchik-suvorov commented Nov 28, 2023

Description

Save user from ending up with failed open_table function call and new folder created - failing fast in case user is trying to load some path that doesn't exist

Related Issue(s)

# Description
Save user from ending up with failed `load` function call and new folder created - failing fast in case user is trying to load some path that doesn't exist

# Related Issue(s)
- closes delta-io#1916
@dimonchik-suvorov dimonchik-suvorov changed the title test: Fail fast for loading non-existent path test: Fail fast for opening non-existent path Nov 28, 2023
@github-actions github-actions bot added binding/rust Issues for the Rust crate crate/core labels Nov 28, 2023
Copy link

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.

@dimonchik-suvorov dimonchik-suvorov changed the title test: Fail fast for opening non-existent path test: Fail fast for opening non existent path Nov 28, 2023
@dimonchik-suvorov dimonchik-suvorov changed the title test: Fail fast for opening non existent path fix: Fail fast for opening non existent path Nov 28, 2023
@dimonchik-suvorov dimonchik-suvorov changed the title fix: Fail fast for opening non existent path fix: fail fast for opening non existent path Nov 28, 2023
@dimonchik-suvorov dimonchik-suvorov changed the title fix: fail fast for opening non existent path fix: fail fast for opening non-existent path Nov 28, 2023
/// is either local path or some kind or URL.
///
/// Will return an error if the path is not valid.
fn resolve_uri_type(table_uri: impl AsRef<str>) -> DeltaResult<UriType> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically pulled out from ensure_table_uri to it's own function

@rtyler rtyler enabled auto-merge (rebase) November 29, 2023 17:40
@rtyler rtyler merged commit 461efb5 into delta-io:main Nov 29, 2023
21 checks passed
@roeap
Copy link
Collaborator

roeap commented Nov 29, 2023

@dimonchik-suvorov @rtyler - I see we have a panic in this PR - generally we want to be panic free and let callers handle errors without crashing everything ... could we fix that?

@dimonchik-suvorov
Copy link
Contributor Author

@dimonchik-suvorov @rtyler - I see we have a panic in this PR - generally we want to be panic free and let callers handle errors without crashing everything ... could we fix that?

Sure, will do. It's just my justification was that wrong path it's a pretty wrong to do and I've decided to panic in this case. But sure, I'll rewrite it to return error instead

@roeap
Copy link
Collaborator

roeap commented Nov 29, 2023

that wrong path it's a pretty wrong to do and I've decided to panic in this case

Maybe a bit more reasoning behind that :) - assuming someone uses the delta integration in a server and a user provides a erroneous input. In that case a panic would potentially crash the entire server. In that case we would want to allow integrators to forward that error to the user.

Since we are a library, I believe that callers should be able to decide if they can recover from such errors.

@dimonchik-suvorov
Copy link
Contributor Author

Since we are a library, I believe that callers should be able to decide if they can recover from such errors.

Make sense for the library, agree, will fix 🫡

dimonchik-suvorov pushed a commit to dimonchik-suvorov/delta-rs that referenced this pull request Nov 30, 2023
This is a continuation of the discussion in the delta-io#1917
Getting rid of panic in the library crate in favor of returning an error so lib users could handle it in a way they see it
Test changes accordingly
roeap added a commit that referenced this pull request Dec 2, 2023
# Description
This is a continuation of the discussion in the
#1917 Getting rid of panic in
the library crate in favor of returning an error so lib users could
handle it in a way they see it Test changes accordingly

Co-authored-by: Robert Pack <42610831+roeap@users.noreply.github.com>
Jan-Schweizer pushed a commit to Jan-Schweizer/delta-rs that referenced this pull request Dec 2, 2023
# Description
This is a continuation of the discussion in the
delta-io#1917 Getting rid of panic in
the library crate in favor of returning an error so lib users could
handle it in a way they see it Test changes accordingly

Co-authored-by: Robert Pack <42610831+roeap@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate crate/core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Load DeltaTable from non-existing folder causing empty folder creation
3 participants