fix(reader): honor absolute paths in duckdb:// URIs and fail fast on missing files#347
Merged
georgestagg merged 3 commits intoposit-dev:mainfrom Apr 22, 2026
Merged
Conversation
…sing files Closes posit-dev#345. - `duckdb:///abs/path` now opens `/abs/path` (SQLAlchemy convention) instead of stripping the leading slash and silently creating a relative phantom DB. - Reject `duckdb:////...` (4+ slashes) with a clear ambiguity message. - `DuckDBReader::from_connection_string` errors up-front when the target file does not exist, surfacing a helpful message instead of a confusing downstream "Table not found" error. - Apply the same `//` vs `///` parsing rules to `sqlite://` URIs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
georgestagg
requested changes
Apr 22, 2026
Collaborator
georgestagg
left a comment
There was a problem hiding this comment.
Thanks for this PR!
I have some feedback, details in comments below.
After my requested changes, the PR will be quite small. If I am permitted, I am going to commit my changes direct to this branch before merging, otherwise I'll follow up in a new PR.
Comment on lines
+115
to
+119
| ConnectionInfo::DuckDBFile(path) => { | ||
| // Fail fast if the path does not exist. DuckDB's default | ||
| // `open` creates the file if missing, which silently masks | ||
| // typos and produces confusing "Table not found" errors | ||
| // downstream. See issue #345. |
Collaborator
There was a problem hiding this comment.
This is actually intended, the same behaviour as invoking duckdb via the CLI:
$ rm nonexistent.db
$ duckdb nonexistent.db
DuckDB v1.4.3 (Andium) d1dc88f950
Enter ".help" for usage hints.
D CREATE TABLE table_name AS FROM VALUES (1) as d;
$ ls nonexistent.db
nonexistent.db
| } | ||
|
|
||
| /// Parse a DuckDB/SQLite URI body into a filesystem path, following | ||
| /// SQLAlchemy conventions. |
Collaborator
There was a problem hiding this comment.
I don't believe this is a SQLAlchemy specific convention, simply a leading slash after the uri:// portion, indicating an absolute path.
Additionally, IMO extra slashes at the start (or in fact anywhere) should not be an error, they are accepted in paths. Consider e.g.
$ touch /tmp/test.txt
$ ls /tmp/test.txt
/tmp/test.txt
$ ls ///tmp/////test.txt
///tmp/////test.txt
georgestagg
approved these changes
Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #345.
duckdb:///abs/pathnow opens/abs/path(SQLAlchemy convention) instead of stripping the leading slash and silently creating a relative phantom DB.duckdb:////...(4+ slashes) with a clear ambiguity message.DuckDBReader::from_connection_stringerrors up-front when the target file does not exist, surfacing a helpful message instead of a confusing downstreamTable not founderror.//vs///parsing rules tosqlite://URIs for consistency.Before the fix,
duckdb:///tmp/demo.duckdbwould be stripped to the relative pathtmp/demo.duckdb; DuckDB would then silently create an empty DB at$CWD/tmp/demo.duckdband queries would fail with a misleading catalog error.Test plan
cargo test -p ggsql --lib --no-default-features --features "duckdb,sqlite,vegalite,ipc,parquet,builtin-data"passes (1305 tests)src/reader/connection.rscover: absolute path preserved, 4-slash rejected, sqlite mirrorsrc/reader/duckdb.rscover: missing file errors and does not create a phantom DB; existing absolute-path DB opens correctlyggsql exec "..." --reader "duckdb:///nonexistent"now errors with a helpful message instead of silently creating the fileNotes
This PR was authored end-to-end with Claude Code (issue filed from the same session, reproducer built against an installed release, fix developed on a fresh branch off
main). Flagging in case you want to tag or track AI-assisted contributions separately; happy to adjust the workflow or commit trailers if you have a preferred convention.🤖 Generated with Claude Code