DuckDB connections use ATTACH (only file based) #98
Conversation
mare5x
left a comment
mare5x
left a comment
There was a problem hiding this comment.
The code is much more understandable now that Executor handles the connection and not Session :)
| if isinstance(connection, duckdb.DuckDBPyConnection): | ||
| path = get_db_path(connection) | ||
| if path is not None: | ||
| connection.close() |
There was a problem hiding this comment.
Do we need to close()? What if the user wants to keep using the connection outside databao?
There was a problem hiding this comment.
It is difficult to work with DuckDB. At least for me. I started with no close(), but it failed because during connection creation relative path could be used. But we can use only full path. In this case DuckDB raises an error, that the same file is connected with different path. So the only way I found to work around was to close the connection. It would be great if you find better solution.
| self._cached_graph: ExecuteSubmit | None = None | ||
| self._prompt_template = read_prompt_template(Path("system_prompt.jinja")) | ||
|
|
||
| # Create a DuckDB connection for the agent |
There was a problem hiding this comment.
Copying the comment from #100 since it's relevant here. We can leave the code as-is for now though.
With the current design, Executor gets a Session in execute so it should technically work with many Sessions. So if the Executor wants to cache the system prompt (or anything else), it needs to keep track to which Session the prompt belongs.
There was a problem hiding this comment.
Let's keep it for now. We have a point to discuss on design review. Then we will fix it in the separate PR.
Uh oh!
There was an error while loading. Please reload this page.