Add table existence check and drop utilities to ClickHouse client #3422
Conversation
…thods Add convenience methods to ClickHouseClient for checking table existence and dropping tables: - table_exists: Checks if a table exists in the specified database using the EXISTS TABLE query, returns true/false - drop_table_if_exists: Drops a table if it exists, idempotent operation Both methods use backticks for quoting identifiers to handle special characters in database/table names. https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
The table_exists and drop_table_if_exists methods are not yet used in the codebase, so add the dead_code lint allow attribute to prevent warnings until they are integrated. https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughTwo utility methods added to ClickHouseClient for table management: Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs`:
- Around line 229-266: Add unit tests in a #[cfg(test)] mod that exercise the
public methods table_exists and drop_table_if_exists: create a ClickhouseClient
test double or instantiate the client with a mocked/overridden execute_sql so
you can assert the exact SQL string produced (e.g. "EXISTS TABLE `{}`.`{}`" and
"DROP TABLE IF EXISTS `{}`.`{}`") and the return handling, include tests for
true/false responses for table_exists and for error propagation (simulate
execute_sql returning Err) and edge cases like empty database or table_name to
ensure formatting/escaping behavior is correct.
- Around line 239-244: Replace the forbidden anyhow::Result in table_exists with
a module-local thiserror-based error type (e.g., ClickHouseError) and return
Result<bool, ClickHouseError>; update table_exists (and any call sites) to
map/propagate execute_sql errors into that ClickHouseError (use a QueryFailed
variant). Define the ClickHouseError enum near this client module with
appropriate variants (e.g., QueryFailed(String), RequestFailed(...)) using
#[derive(thiserror::Error)]. Also standardize identifier quoting across methods
(change the backtick quoting in table_exists to match build_insert_query's
double-quote style) so all SQL identifiers use the same quoting convention.
- Around line 255-266: The drop_table_if_exists function uses anyhow::Result and
backtick quoting like the other method; change its signature to return the
crate-specific Result type (the same one used by table_exists) instead of
anyhow::Result, and update the SQL to use consistent double-quote identifier
quoting (e.g. format!("DROP TABLE IF EXISTS \"{}\".\"{}\"", database,
table_name)); keep the call to self.execute_sql(...).await? and propagate errors
with ? so the function body returns the appropriate Result<()> type used across
this client.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
cargo clippyto ensure Rust code passes Clippy's linting standards before each commit
**/*.rs: Usethiserrorwith#[derive(thiserror::Error)]for error handling in Rust; define errors near the fallibility unit with NO globalErrortype
NEVER useanyhow::Resultin Rust; define custom error types instead
Use snake_case for functions and variables, PascalCase for types and traits, SCREAMING_SNAKE_CASE for constants in Rust
Place constants inconstants.rsat the appropriate module level in Rust
Use tuple structs with validation constructors as newtypes in Rust (e.g.,struct UserId(String))
Write inline tests with#[cfg(test)]modules in Rust
Documentation is required for all public APIs in Rust
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/**/*.rs
📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)
apps/framework-cli/**/*.rs: Always runcargo clippy --all-targets -- -D warningsbefore commits; fix all warnings - no Clippy warnings may remain (treat warnings as errors)
Userustfmt --edition 2021for consistent formatting
Write meaningful names for functions, variables, and types
Document all public APIs and breaking changes
Usethiserrorcrate instead ofanyhow::Resultfor error handling
Define errors near their unit of fallibility (no global Error types)
Use#[derive(thiserror::Error)]with#[error()]messages for error definitions
Define newtypes as tuple structs:struct UserId(u64);
Add validation constructors for newtypes:UserId::new(id: u64) -> Result<Self, Error>
Derive standard traits for newtypes:#[derive(Debug, Clone, PartialEq)]
ImplementFrom/TryFromfor newtype conversions
Usederive_moreornutypeto reduce newtype boilerplate
Useconstfor static values (prefer overstatic)
UseUPPER_SNAKE_CASEnaming for constants
Scope constant visibility with preference:pub(crate)>pub(super)>pub
Group related constants together
Write unit tests for all public functions
Test error conditions and edge cases
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
**/framework-cli/src/**
⚙️ CodeRabbit configuration file
**/framework-cli/src/**: When reviewing changes to Moose CLI:
- Check if any user-facing changes were made (commands, flags, configs, apis, etc)
- If yes, verify the documentation for THAT SPECIFIC feature is updated in apps/framework-docs-v2/content
- If docs for that feature doesn't exist yet, it should be added. If the change removes public apis, the documentation for those should also be removed. Changing unrelated docs doesn't satisfy this requirement.
- In addition to reviewing for docs discrepancies, you should also review the code as per usual guidelines.
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test E2E TypeScript Tests Template (Node 20, npm)
- GitHub Check: Test E2E Python Tests Template (Python 3.12)
- GitHub Check: Test E2E TypeScript Tests Template (Node 22, npm)
- GitHub Check: Test E2E TypeScript Tests Template (Node 24, npm)
- GitHub Check: Test E2E Python Tests Template (Python 3.13)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| /// Checks if a table exists in the specified database | ||
| /// | ||
| /// # Arguments | ||
| /// * `database` - The database name | ||
| /// * `table_name` - The table name | ||
| /// | ||
| /// # Returns | ||
| /// `Ok(true)` if the table exists, `Ok(false)` if it doesn't, `Err` on query failure | ||
| #[allow(dead_code)] | ||
| pub async fn table_exists(&self, database: &str, table_name: &str) -> anyhow::Result<bool> { | ||
| let result = self | ||
| .execute_sql(&format!("EXISTS TABLE `{}`.`{}`", database, table_name)) | ||
| .await?; | ||
| Ok(result.trim() == "1") | ||
| } | ||
|
|
||
| /// Drops a table if it exists in the specified database | ||
| /// | ||
| /// # Arguments | ||
| /// * `database` - The database name | ||
| /// * `table_name` - The table name | ||
| /// | ||
| /// # Returns | ||
| /// `Ok(())` on success, `Err` on failure | ||
| #[allow(dead_code)] | ||
| pub async fn drop_table_if_exists( | ||
| &self, | ||
| database: &str, | ||
| table_name: &str, | ||
| ) -> anyhow::Result<()> { | ||
| self.execute_sql(&format!( | ||
| "DROP TABLE IF EXISTS `{}`.`{}`", | ||
| database, table_name | ||
| )) | ||
| .await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Missing unit tests for new public methods.
Guidelines require unit tests for all public functions. Add tests in the #[cfg(test)] module for table_exists and drop_table_if_exists.
At minimum, test the SQL query formatting logic. Integration tests against a real ClickHouse instance would be ideal but may require separate test infrastructure.
#!/bin/bash
# Check if there are any existing integration test patterns in this crate
rg -l "table_exists|drop_table" --type rustAs per coding guidelines: "Write unit tests for all public functions" and "Test error conditions and edge cases".
🤖 Prompt for AI Agents
In `@apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs` around lines
229 - 266, Add unit tests in a #[cfg(test)] mod that exercise the public methods
table_exists and drop_table_if_exists: create a ClickhouseClient test double or
instantiate the client with a mocked/overridden execute_sql so you can assert
the exact SQL string produced (e.g. "EXISTS TABLE `{}`.`{}`" and "DROP TABLE IF
EXISTS `{}`.`{}`") and the return handling, include tests for true/false
responses for table_exists and for error propagation (simulate execute_sql
returning Err) and edge cases like empty database or table_name to ensure
formatting/escaping behavior is correct.
| pub async fn table_exists(&self, database: &str, table_name: &str) -> anyhow::Result<bool> { | ||
| let result = self | ||
| .execute_sql(&format!("EXISTS TABLE `{}`.`{}`", database, table_name)) | ||
| .await?; | ||
| Ok(result.trim() == "1") | ||
| } |
There was a problem hiding this comment.
Guideline violation: anyhow::Result is forbidden.
Per coding guidelines: "NEVER use anyhow::Result in Rust; define custom error types instead." Define a custom error type using thiserror.
Also, inconsistent identifier quoting: uses backticks here but build_insert_query (line 282) uses double quotes. Standardize on one approach.
Suggested approach
Define a dedicated error type near this module:
#[derive(Debug, thiserror::Error)]
pub enum ClickHouseError {
#[error("Query execution failed: {0}")]
QueryFailed(String),
#[error("HTTP request failed: {0}")]
RequestFailed(#[from] hyper_util::client::legacy::Error),
// ... other variants
}Then return Result<bool, ClickHouseError> instead of anyhow::Result<bool>.
As per coding guidelines: "Use thiserror with #[derive(thiserror::Error)] for error handling in Rust; define errors near the fallibility unit with NO global Error type" and "NEVER use anyhow::Result in Rust".
🤖 Prompt for AI Agents
In `@apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs` around lines
239 - 244, Replace the forbidden anyhow::Result in table_exists with a
module-local thiserror-based error type (e.g., ClickHouseError) and return
Result<bool, ClickHouseError>; update table_exists (and any call sites) to
map/propagate execute_sql errors into that ClickHouseError (use a QueryFailed
variant). Define the ClickHouseError enum near this client module with
appropriate variants (e.g., QueryFailed(String), RequestFailed(...)) using
#[derive(thiserror::Error)]. Also standardize identifier quoting across methods
(change the backtick quoting in table_exists to match build_insert_query's
double-quote style) so all SQL identifiers use the same quoting convention.
| pub async fn drop_table_if_exists( | ||
| &self, | ||
| database: &str, | ||
| table_name: &str, | ||
| ) -> anyhow::Result<()> { | ||
| self.execute_sql(&format!( | ||
| "DROP TABLE IF EXISTS `{}`.`{}`", | ||
| database, table_name | ||
| )) | ||
| .await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Same issues: anyhow::Result and inconsistent quoting.
This method also uses anyhow::Result and backtick quoting. Apply the same fixes as table_exists.
Diff with consistent quoting (assuming double quotes)
pub async fn drop_table_if_exists(
&self,
database: &str,
table_name: &str,
- ) -> anyhow::Result<()> {
+ ) -> Result<(), ClickHouseError> {
self.execute_sql(&format!(
- "DROP TABLE IF EXISTS `{}`.`{}`",
+ "DROP TABLE IF EXISTS \"{}\".\"{}\"",
database, table_name
))
.await?;
Ok(())
}🤖 Prompt for AI Agents
In `@apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs` around lines
255 - 266, The drop_table_if_exists function uses anyhow::Result and backtick
quoting like the other method; change its signature to return the crate-specific
Result type (the same one used by table_exists) instead of anyhow::Result, and
update the SQL to use consistent double-quote identifier quoting (e.g.
format!("DROP TABLE IF EXISTS \"{}\".\"{}\"", database, table_name)); keep the
call to self.execute_sql(...).await? and propagate errors with ? so the function
body returns the appropriate Result<()> type used across this client.
Summary
Added two new utility methods to the ClickHouse client for managing table operations: checking if a table exists and dropping a table if it exists.
Changes
table_exists(): New async method that checks whether a table exists in a specified database by executing a ClickHouseEXISTS TABLEquery. ReturnsOk(true)if the table exists,Ok(false)otherwise.drop_table_if_exists(): New async method that safely drops a table from a specified database using theDROP TABLE IF EXISTSstatement, preventing errors if the table doesn't exist.Implementation Details
#[allow(dead_code)]to suppress warnings during development, indicating they may be used in future features or other parts of the codebaseanyhow::Resultexecute_sql()infrastructure for consistencyhttps://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
Note
Low Risk
Low risk: adds two new utility methods on the ClickHouse client without changing existing query/insert behavior; main risk is misuse of user-provided identifiers in formatted SQL.
Overview
Adds two new ClickHouse client utilities in
client.rs:table_exists()(runsEXISTS TABLEand returns a boolean) anddrop_table_if_exists()(runsDROP TABLE IF EXISTS). Both reuse the existingexecute_sql()path and quote database/table identifiers with backticks.Written by Cursor Bugbot for commit efb12d8. This will update automatically on new commits. Configure here.