-
Notifications
You must be signed in to change notification settings - Fork 125
Paginate list_tables #92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds pagination support to the list_tables function, allowing users to retrieve table listings in manageable chunks. The response format has changed from a list to a dictionary containing paginated results with navigation tokens.
Key changes:
- Modified
list_tablesto return a dictionary withtables,next_page_token, andtotal_tablesfields instead of a plain list - Implemented pagination with configurable
page_size(default 50) and token-based navigation using TTL cache - Added
include_detailed_columnsparameter to optionally omit column metadata for reduced payload sizes
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| mcp_clickhouse/mcp_server.py | Implemented pagination logic with helper functions, token management, and cache-based state tracking |
| mcp_clickhouse/init.py | Exported new pagination-related functions and cache object |
| tests/test_tool.py | Updated assertions to handle new dictionary response format and added edge case tests |
| tests/test_pagination.py | Added comprehensive pagination test suite covering various scenarios |
| tests/test_mcp_server.py | Updated test assertions to validate new response structure |
| pyproject.toml | Added cachetools dependency for TTL-based token cache |
| README.md | Updated documentation to reflect new pagination parameters and response format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
iskakaushik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Most of the original work for this was done by @iskakaushik in #34. However, in trying to resolve several month's worth of conflict on main I accidentally got it into a funky state (sorry!). So, I decided to rebuild pagination for
list_tableson top of current main rather than merging main. Note that I also added a metadata trimming switchinclude_detailed_columnsto reduce payload size when needed.Key Changes
list_tablesnow returns adictwith the keystables,next_page_token, andtotal_tables. It was a list previouslypage_size(default 50) andpage_token(one‑time tokens, 1‑hour TTL via cache)include_detailed_columns(defaults toTrue) to preserve previous behavior but can be set toFalseto omit column metadata which will create lighter responses while preserving the fullcreate_table_querycachetoolsfor TTL cachetests/test_pagination.pytests/test_tool.py,tests/test_mcp_server.pyhas been updated to reflect the new response shape and edge casesLLM Usage
list_tableswithinclude_detailed_columns=falseand/or a smallerpage_size.next_page_tokenis returned, the assistant paginates by passing it back until the user narrows scope or coverage is sufficient.Closes #25