fix(database): drop spurious trailing ? from listing-database URIs#3357
Merged
jackye1995 merged 2 commits intoMay 8, 2026
Merged
Conversation
`url::Url::query_pairs_mut()` leaves the URL with `query=Some("")`
after `.clear()` even when the input had no query string. The
listing-database connect path then captured that empty query into
`ListingDatabase::query_string`, and `table_uri()` blindly appended
`?<query>` to every per-table URI — producing URIs like
`s3://bucket/prefix/foo.lance?`.
That trailing `?` is benign for normal table operations, but it
breaks any caller that constructs a sub-path from the table URI:
e.g. MemWAL flushes write to `<table_uri>/_mem_wal/<shard>/<rand>_gen_<n>`,
which `url::Url::parse` then re-parses as
`path=<base table>` + `query=/_mem_wal/...`. `Dataset::write` resolves
the base table dataset, finds it already exists, and fails with
`Dataset already exists: …_gen_1` on the very first MemTable flush
(observed against S3, deterministic across modes).
Fix: treat `Some("")` query the same as no query when capturing
`query_string`. A real `?foo=bar` query is still propagated
unchanged.
Adds a regression test covering both the empty-query and
non-empty-query paths.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The original Windows job failed because connect_with_options' local
setup path tries to mkdir on a `file:///C:/...` URI, which Windows
rejects before we ever reach the URL-mutation logic this test was
checking.
Splits the regression into two tests:
* `test_capture_query_treats_empty_as_none` mirrors the URL
mutation step and asserts the empty-query → None invariant.
Pure URL handling, runs on every platform.
* `test_table_uri_url_path_has_no_trailing_question_mark` is now
gated on non-Windows since it exercises the full
connect_with_options + filesystem path.
Other CI failures (pydantic1x, aarch64-windows) on the previous run
were build timeouts, not test failures, so no other code changes are
needed.
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
2 participants
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
url::Url::query_pairs_mut()leaves the URL withquery=Some("")after.clear()even when the input had no query string. The listing-database connect path then captured that empty query intoListingDatabase::query_string, andtable_uri()blindly appended?<query>to every per-table URI — producing URIs likes3://bucket/prefix/foo.lance?.The trailing
?is benign for normal table operations, but it breaks any caller that constructs a sub-path from the table URI. In particular, MemWAL flushes write to<table_uri>/_mem_wal/<shard>/<rand>_gen_<n>, whichurl::Url::parsethen re-parses aspath=<base table>+query=/_mem_wal/....Dataset::writeresolves the base table dataset, finds it already exists, and fails withDataset already exists: …_gen_1on the very first MemTable flush (observed deterministically against S3 across all merge_insert LSM modes; tracked in lance-format/lance#6713).Fix
Treat
Some("")query the same as no query when capturingquery_string. A real?foo=barquery is still propagated unchanged.Adds a regression test covering both the empty-query and non-empty-query paths.
Verification
url::Url::parse("s3://bucket/prefix/").query()→None, but afterquery_pairs_mut().clear()→Some(""). Confirmed in a standalone repro.table_uri()for ans3://-style connection ends with?, breaking MemWAL and any future sub-path consumer in the same way.test_table_uri_url_path_has_no_trailing_question_markexercises both code paths.