Skip to content

Add better ParsedQuery semantics around client APIs#101

Open
Qix- wants to merge 1 commit into
Protryon:masterfrom
Qix-:into-error
Open

Add better ParsedQuery semantics around client APIs#101
Qix- wants to merge 1 commit into
Protryon:masterfrom
Qix-:into-error

Conversation

@Qix-

@Qix- Qix- commented Jun 3, 2026

Copy link
Copy Markdown

Right now passing a ParsedQuery verbatim into the client query methods causes a compilation error. Since TryInto<ParsedQuery> for ParsedQuery is an identity, the blanket implementation by the Rust language names type Error = core::convert::Infallible.

Therefore, there needed to be a way to specify that as long as the error message could be coerced into a KlickhouseError, that that was sufficient for the client library to work. I started by doing

-some_arg: impl TryInto<ParsedQuery, Error = KlickhouseError>
+some_arg: impl TryInto<ParsedQuery, Error = impl Into<KlickhouseError>>

But that caused some (already) awkward semantics around query() (and use<>, a new thing from Edition 2024, that forces the use of explicit generic parameters) to get even worse (requiring .query() without an implicit return type to be spelled .query::<SomeRowType, _, _>(...) for all invocations).

Therefore I pulled out the explicit TryInto argument types into their own trait TryIntoQuery to drastically clean up the code and error handling (primarily around .query()), which didn't yet get rid of the captured lifetimes issue as I had hoped.

Took some finagling but I realized that the use<> could be removed if Row was marked 'static, which is really what the error was complaining about. I would be... surprised (perhaps concerned) if a typical Row is a non-static item being serialized/deserialized from Klickhouse, and given that it's almost always being implemented by the derives, I would imagine this is a fair tradeoff.

Now not only can you pass the already-unwrapped results of parsed queries into the client methods directly without hitting the KlickhouseError == Infallible compilation error, but now .query::<T>() doesn't need to have the extra , _> at the end of each invocation.

Comment on lines +83 to +88
.query::<MyUserData>(
QueryBuilder::new("SELECT * FROM klickhouse_example WHERE 1=?;")
.arg(1)
.finalize()
.unwrap(),
)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Happy to revert this back to the original; I just wanted a reproduction test case that I was facing in my own application that prompted this PR in the first place. This invocation won't compile on the current HEAD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant