Conversation
|
Warning Rate limit exceeded@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughA new tool, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CrateDbMcp
participant get_table_columns
participant Queries
User->>CrateDbMcp: Request table/column info via get_table_columns
CrateDbMcp->>get_table_columns: Call function
get_table_columns->>Queries: Use TABLES_COLUMNS SQL query
get_table_columns->>CrateDbMcp: Return categorized metadata
CrateDbMcp->>User: Respond with schema/column info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cratedb_mcp/prompt/instructions.md (1)
5-10: Synchronise tool list with newly addedget_table_columns
The numbered list still shows four tools and omits the new one, yet the rules below reference it. Add the entry to prevent user confusion.1. `get_table_metadata`: Return table metadata for all tables stored in CrateDB. +2. `get_table_columns`: Return schema & column information for all tables. 2. `query_sql`: Execute an SQL query on CrateDB and return the results. … 3. `get_cratedb_documentation_index`: Return the table of contents … 4. `fetch_cratedb_docs`: Given a `link` …
🧹 Nitpick comments (4)
cratedb_mcp/knowledge.py (1)
12-21: Add deterministic ordering to the query result set
Without anORDER BY, consumers get columns in an arbitrary sequence which can differ between clusters and between successive calls. Adding an explicit sort makes downstream processing and manual inspection easier.FROM information_schema.columns c +ORDER BY + c.table_schema, + c.table_name, + c.ordinal_positiontests/test_examples.py (1)
8-8: Avoid hard-coded timeout magic numbers
Bumping the constant works short-term, but the script may keep growing. Consider deriving the timeout from an env var or marking the test@pytest.mark.slowand usingpytest-timeoutto centralise configuration.examples/mcptools.sh (1)
31-31: Quote tool name to guard against shell alias expansion
mcptis aliased, but if someone defines a shell function namedget_table_columns, unquoted arguments can mis-behave. For absolute safety:-mcpt call get_table_columns cratedb-mcp serve +mcpt call "get_table_columns" cratedb-mcp servetests/test_mcp.py (1)
61-64: Test implementation is appropriate but could be more robust.The test correctly verifies the function output contains expected schema names. However, consider checking the structure of the returned dictionary for more comprehensive validation.
Consider enhancing the test to verify the expected dictionary structure:
def test_get_table_columns(): response = str(get_table_columns()) assert "information_schema" in response assert "pg_catalog" in response + + # Verify the returned structure + result = get_table_columns() + assert isinstance(result, dict) + expected_keys = {"user", "information_schema", "pg_catalog", "sys"} + assert set(result.keys()) == expected_keys
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGES.md(1 hunks)cratedb_mcp/core.py(2 hunks)cratedb_mcp/knowledge.py(1 hunks)cratedb_mcp/prompt/instructions.md(1 hunks)cratedb_mcp/tool.py(1 hunks)examples/mcptools.sh(1 hunks)tests/test_examples.py(1 hunks)tests/test_knowledge.py(1 hunks)tests/test_mcp.py(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: amotl
PR: crate/cratedb-mcp#17
File: CHANGES.md:5-5
Timestamp: 2025-05-16T13:35:04.487Z
Learning: For the cratedb-mcp repository, version numbers and release dates in CHANGES.md are updated by a dedicated "Release vX.Y.Z" commit as part of the release process, not during documentation preparation PRs.
CHANGES.md (2)
Learnt from: amotl
PR: #17
File: CHANGES.md:5-5
Timestamp: 2025-05-16T13:35:04.487Z
Learning: For the cratedb-mcp repository, version numbers and release dates in CHANGES.md are updated by a dedicated "Release vX.Y.Z" commit as part of the release process, not during documentation preparation PRs.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the backlog.md document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
cratedb_mcp/prompt/instructions.md (3)
Learnt from: amotl
PR: #17
File: CHANGES.md:5-5
Timestamp: 2025-05-16T13:35:04.487Z
Learning: For the cratedb-mcp repository, version numbers and release dates in CHANGES.md are updated by a dedicated "Release vX.Y.Z" commit as part of the release process, not during documentation preparation PRs.
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the backlog.md document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
examples/mcptools.sh (1)
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
tests/test_mcp.py (1)
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
🧬 Code Graph Analysis (5)
tests/test_knowledge.py (1)
cratedb_mcp/knowledge.py (1)
Queries(11-101)
examples/mcptools.sh (2)
cratedb_mcp/tool.py (1)
get_table_columns(31-62)cratedb_mcp/cli.py (1)
serve(73-94)
tests/test_mcp.py (1)
cratedb_mcp/tool.py (1)
get_table_columns(31-62)
cratedb_mcp/core.py (1)
cratedb_mcp/tool.py (1)
get_table_columns(31-62)
cratedb_mcp/tool.py (1)
cratedb_mcp/knowledge.py (1)
Queries(11-101)
🔇 Additional comments (6)
CHANGES.md (1)
6-6: Changelog entry LGTM
Entry is concise and follows the existing unreleased-section pattern.tests/test_knowledge.py (1)
15-15: LGTM! Test assertion follows established pattern.The new assertion correctly verifies that the
TABLES_COLUMNSquery references the expectedinformation_schema.columnstable, maintaining consistency with the existing test structure.tests/test_mcp.py (1)
7-7: LGTM! Import follows alphabetical ordering.The import is correctly added in alphabetical order with the other function imports.
cratedb_mcp/core.py (2)
13-13: LGTM! Import correctly added in alphabetical order.The import follows the established pattern and maintains alphabetical ordering within the import statement.
50-56: LGTM! Tool registration follows established pattern.The tool registration is correctly implemented using the same pattern as other tools, with appropriate tags and placement within the text-to-SQL section.
cratedb_mcp/tool.py (1)
31-62: LGTM! Well-implemented function with comprehensive documentation.The function implementation is solid with several strengths:
- Comprehensive docstring explaining purpose, return fields, and categorization
- Clean categorization of schemas into user tables vs. system tables
- Proper use of existing
query_sql()function for execution and error handling- Logical structure that returns organized results by schema type
The implementation follows established patterns in the codebase and provides valuable functionality for schema discovery.
f34b392 to
9142a8e
Compare
| } | ||
|
|
||
| response = {} | ||
| for variant, where in variants.items(): |
There was a problem hiding this comment.
nitpick: Could have used dict comprehension:
return {
variant: query_sql( Queries.TABLES_COLUMNS.format(where) )
for variant, where in variants.items()
}
About
This patch adds a tool to inquire column information. We thought
get_table_metadatawould be doing it, but it didn't.References
get_table_metadata#64