Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the ADBC ClickHouse driver to accept clickhouse:// connection URLs (to support automatic driver discovery in ADBC driver managers) and to eagerly parse/validate the connection URL when it is set, so malformed inputs fail fast.
Changes:
- Added parsing/normalization of database
Uriviaurl::Url, including rewritingclickhouse://tohttps://by default with?protocol=http|httpsoverride. - Implemented
ClickhouseDatabase::get_option_*for string/bytes options (URI/username/password/custom), replacing priorNotImplementedbehavior. - Added the
urlcrate dependency and unit tests for URL rewriting behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/lib.rs |
Adds URL parsing/rewriting logic, stores parsed Url, implements option getters, and adds unit tests for URL rewriting. |
Cargo.toml |
Adds the url dependency needed for parsing/rewriting. |
Cargo.lock |
Locks the added url dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match key { | ||
| OptionDatabase::Uri => Ok(self | ||
| .url | ||
| .as_ref() | ||
| .map_or_else(String::new, |url| url.to_string())), |
There was a problem hiding this comment.
I guess it doesn't hurt but most drivers don't actually support readback here anyways
| OptionDatabase::Username => Ok(self.username.clone().unwrap_or_default()), | ||
| OptionDatabase::Password => Ok(self.password.clone().unwrap_or_default()), | ||
| OptionDatabase::Other(s) => self.get_custom_option(&s), |
There was a problem hiding this comment.
Same here: most drivers don't let you read these back anyways, and I suppose for password (and now thinking about it, URI) there's a risk
Summary
clickhouse://scheme for automatic detection in ADBC driver managers.https://scheme by default; add?protocol=httpto the URL to override.Database::get_option_string()implementation.closes #6
closes #43
closes #44
Checklist
Delete items not relevant to your PR: