Refactor ClickHouse MCP tools with improved documentation and functionality #18
Conversation
…nality - Added detailed descriptions for MCP tools - Enhanced `get_table_sample` with rand() sampling, filtering, and error handling - Improved logging and parameter validation
|
|
- Add `.specstory/history/*` to .gitignore - Remove previously tracked SpecStory history markdown files
|
Why disable TLS by default? |
| raise ValueError( | ||
| f"Missing required environment variables: {', '.join(missing_vars)}" | ||
| ) | ||
| os.environ[var] = default_value |
There was a problem hiding this comment.
This ends up calling putenv underneath. Ideally we wouldn't mutate process environment variables. Especially since this changes getenv to always be used with a default, it seems unnecessary
| ClickHouseError: If there's an error executing the query | ||
| """ | ||
| # Validate limit | ||
| if limit > 10: |
There was a problem hiding this comment.
Since this is an MCP and the package is meant to work with AI agents, I had to set this limit to avoid overflowing the language model’s context window. I deal with huge tables, and the AI kept trying to grab a way too large sample. Do you think I should increase the cap?
There was a problem hiding this comment.
10 seems low, have you checked how other MCP servers handle this?
Local development artifact |
I guess TLS off by default is fair when the default host is localhost. Could have TLS default be off when host is localhost & on otherwise, but having static defaults is nice. At least TLS verification should be on by default (which won't matter when TLS is disabled) |
get_table_sampletool with rand() sampling, filtering, and error handling