fix: Uses SQL tables if available for script calls #4785
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances SQL execution by allowing the optional inclusion of SQL tables for script calls and resolves some lint errors. Key changes include:
- Adding tests for script hooks in tests/_sql/test_sql.py.
- Modifying the wrapped_sql and sql functions to accept an optional "tables" parameter.
- Updating engine execute signatures across different modules to support the new "tables" parameter.
- Adapting the frontend Codemirror SQL string generation to include table parameters for DuckDB.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/_sql/test_sql.py | Added a new test (test_as_script) that ensures proper error handling for script calls. |
| marimo/_sql/utils.py | Enhanced wrapped_sql to accept a "tables" argument; updated globals handling with eval. |
| marimo/_sql/sql.py | Integrated support for the "tables" parameter in the sql function for DuckDB engines. |
| marimo/_sql/engines/types.py, sqlalchemy.py, pyiceberg.py, ibis.py, duckdb.py, clickhouse.py | Updated execute method signatures to include an optional tables parameter. |
| frontend/src/core/codemirror/language/sql.ts | Modified code generation to include table parameters when using the default engine. |
| frontend/src/core/codemirror/language/tests/sql.test.ts | Added tests to validate that table parameters are correctly formatted in generated code. |
| frontend/src/core/codemirror/completion/utils.ts | Only a copyright comment update. |
Co-authored-by: Myles Scolnick <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Copilot <[email protected]>
|
just tested and it feels good! ditto on Myles' comment about conflicting table names, but I usually get errors when trying to define/create tables with the same name. |
|
Actually copilot just created a bug |
for more information, see https://pre-commit.ci
|
|
||
| const end = `\n """${showOutputParam}${engineParam}${tablesParam}\n)`; | ||
|
|
||
| // TODO: Ruff-wasm is now more main stream (adopted by jupyter-ruff) |
There was a problem hiding this comment.
this manual formatting is quite easy (and gets reformatted with black in the backend). im not sure this is a TODO we want to do for the extra dependency
There was a problem hiding this comment.
Noticed that the line length varies the behavior (ruff rather fit everything on one line if it can). I think the manual formatting may eventually be more effort than it is worth
Also fixes some lint errors
fixes #4675