-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Implementing SQL toolkit #3567
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
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
|
Seems there is a pre-commit error. Will investigate first. |
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.
thanks @HxyScotthuang 's contribution, left some comments below, please run pre-commit run --all-files locally to check pre-commit issue, I created one enhance PR #3568 based on comment below, feel free to review
camel/toolkits/sql_toolkit.py
Outdated
| database_type (str, optional): Type of database to use. Currently | ||
| supports "duckdb" and "sqlite". (default: :obj:`"duckdb"`) |
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.
would be better to use Literal for limited type value
camel/toolkits/sql_toolkit.py
Outdated
| raise ValueError( | ||
| "Write operations are not allowed in read-only mode. " | ||
| "The query contains write operations (INSERT, UPDATE, DELETE, " | ||
| "DROP, CREATE, ALTER, TRUNCATE, etc.). Only SELECT queries " | ||
| "are permitted." | ||
| ) |
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.
would be better to return error message instead of raise error, since this toolkit would be used by agent, we just need to pass the key information to agent instead of stop the running, same for other error raising part
| # SQL keywords that indicate write operations | ||
| _WRITE_KEYWORDS: ClassVar[List[str]] = [ |
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.
there are more commands that can also write to files like COPY
camel/toolkits/sql_toolkit.py
Outdated
| "column_name": row["name"], | ||
| "column_type": row["type"], | ||
| "null": "YES" if row["notnull"] == 0 else "NO", | ||
| "key": "PRI" if row["pk"] == 1 else None, |
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.
would not work as expected if there's composite primary key, we need to check whether row["pk"] > 0
camel/toolkits/sql_toolkit.py
Outdated
| self._validate_query_mode(query) | ||
|
|
||
| try: | ||
| logger.debug(f"Executing query: {query[:100]}...") |
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.
better show the whole query
camel/toolkits/sql_toolkit.py
Outdated
| # Query did not return results or is a write operation | ||
| # (INSERT, UPDATE, DELETE, etc.) | ||
| # Commit the transaction | ||
| self._connection.commit() | ||
| logger.debug("Non-SELECT query executed successfully") | ||
| return [] |
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.
we shouldn't handle this silently and return [], we need to provide meaningful information to agent
camel/toolkits/sql_toolkit.py
Outdated
| except Exception: | ||
| pass |
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.
same here, shouldn't handle silently
| """ | ||
| if self.database_type == "duckdb": | ||
| try: | ||
| import duckdb |
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.
we need to add dependency into .toml file and run uv lock to update the lock file
Merge the sql-toolkit and sql-toolkit_wd branch to main, supporting DuckDB and sqlite3 database toolkit support.
Description
This PR implements a comprehensive SQL Toolkit for CAMEL-AI, providing native database/SQL support as requested in #3566.
Features Implemented:
Files Added:
camel/toolkits/sql_toolkit.py- Main toolkit implementationtest/toolkits/test_sql_toolkit.py- Comprehensive test suiteexamples/toolkits/sql_toolkit.py- Usage examplesFiles Modified:
camel/toolkits/__init__.py- Added SQLToolkit exportTesting:
Fixes #3566