Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
🟡 Changes recommended
Fix the DuckDB result-limit regression and case-insensitive CTE scope handling before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Polars as a selectable SQL engine with lazy execution, dependency discovery, editor integration, WASM loading, datasource support, tests, and documentation.
Changes:
- Added Polars SQL execution for
DataFrameandLazyFrame. - Added reactive reference discovery and dependency loading.
- Integrated Polars across the SQL editor, datasource UI, and WASM.
- Added tests and documentation.
File summaries
| File | Description |
|---|---|
tests/_sql/test_sql.py |
Tests dependency bundling and result limits. |
tests/_sql/test_polars.py |
Tests Polars engine behavior and conversions. |
tests/_runtime/test_runtime.py |
Tests reactive Polars SQL dependencies. |
tests/_ast/test_visitor.py |
Tests Polars SQL reference discovery. |
marimo/_sql/sql.py |
Adds Polars engine selection and result handling. |
marimo/_sql/error_utils.py |
Classifies Polars SQL errors. |
marimo/_sql/engines/polars.py |
Implements Polars SQL execution. |
marimo/_ast/visitor.py |
Discovers Polars SQL references. |
marimo/_ast/sql_visitor.py |
Adds fallback SQL scanning. |
frontend/src/core/wasm/worker/worker.ts |
Loads SQL-specific WASM dependencies. |
frontend/src/core/wasm/worker/bootstrap.ts |
Preloads notebook SQL dependencies. |
frontend/src/core/wasm/utils.ts |
Detects SQL engines and package needs. |
frontend/src/core/wasm/bridge.ts |
Passes SQL output configuration to loading. |
frontend/src/core/wasm/__tests__/utils.test.ts |
Tests WASM package detection. |
frontend/src/core/wasm/__tests__/bridge.test.ts |
Tests bridge configuration forwarding. |
frontend/src/core/islands/worker/worker.tsx |
Adds SQL dependency handling for islands. |
frontend/src/core/datasets/engines.ts |
Registers the Polars engine. |
frontend/src/core/datasets/data-source-connections.ts |
Adds the Polars datasource connection. |
frontend/src/core/datasets/__tests__/data-source.test.ts |
Tests Polars datasource registration. |
frontend/src/core/codemirror/language/languages/sql/utils.ts |
Adds Polars dialect recognition. |
frontend/src/core/codemirror/language/languages/sql/sql.ts |
Integrates Polars engine selections. |
frontend/src/core/codemirror/language/languages/sql/renderers.tsx |
Displays internal engine labels. |
frontend/src/core/codemirror/language/__tests__/sql.test.ts |
Tests Polars editor integration. |
frontend/src/core/codemirror/format.ts |
Adds Polars SQL formatting. |
frontend/src/core/ai/context/providers/datasource.ts |
Labels Polars datasource context. |
frontend/src/core/ai/context/providers/__tests__/datasource.test.ts |
Tests datasource labels. |
frontend/src/components/datasources/utils.ts |
Adds Polars datasource formatting. |
frontend/src/components/databases/display.tsx |
Adds Polars display naming. |
frontend/src/components/app-config/app-config-form.tsx |
Documents Polars output behavior. |
docs/guides/working_with_data/sql.md |
Documents the Polars SQL engine. |
Review details
Suppressed comments (3)
frontend/src/core/codemirror/language/languages/sql/sql.ts:132
- Because this PR is authored by a coding agent and changes the visible SQL editor flow, please provide screenshots if possible showing Polars selected and the generated cell in edit/run views, and verify the edge cases around DuckDB versus Polars output modes rather than only the direct selection path.
return `_df = mo.sql(f"""SELECT * FROM """, engine=${toPythonEngine(engine)})`;
frontend/src/core/datasets/data-source-connections.ts:43
- Because this PR is agent-authored and changes the SQL editor/data-source UI, please provide screenshots of the Polars engine in the relevant edit/run/app views if possible, and verify edge cases such as switching between DuckDB and Polars and running with optional packages unavailable. This helps validate the UI integration beyond the direct happy path.
[
POLARS_ENGINE,
{
name: POLARS_ENGINE,
dialect: "polars",
source: "polars",
display_name: "Polars",
databases: [],
},
marimo/_ast/sql_visitor.py:736
- The fallback scope check compares CTE names case-sensitively, but unquoted SQL identifiers are case-insensitive. For
WITH Orders AS (...) SELECT * FROM orders, this reportsordersas an external frame even though the CTE shadows it, causing an unnecessary reactive dependency (and frame registration). Preserve whether each CTE name is quoted and compare unquoted names case-insensitively when checking scope.
name == table and start <= index < end
for name, start, end in cte_scopes
- Files reviewed: 30/30 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The reported DuckDB test_applies_limit regression does not reproduce: auto returns an eager frame and keeps total-rows="too_many" and pagination. Exercise auto, polars, and pandas explicitly, and cover the existing lazy renderer for native DuckDB and lazy-polars outputs. Also assert that the Polars default result limit never collects a lazy result. Polars SQLContext resolves quoted and unquoted relation names with exact case. WITH Orders ... FROM orders reads the external orders frame, so case-folding CTE names would remove a required reactive dependency. Add execution and AST regression tests with and without SQLGlot, and document why the fallback preserves case. Cover DuckDB-to-Polars-to-DuckDB editor round trips with output enabled and disabled, preserving the query, interpolation, and result variable. Validation: 129 focused Python tests and 111 frontend tests passed. Ruff and changed-file formatting checks passed. Full make check could not run because the local environment lacks the typos executable. Optional UI screenshots were not captured.
Re the CTE concern - Polars resolves both quoted and unquoted relation names case-sensitively. |
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-layer parser, runtime, WASM, and editor changes require final human and visual validation.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/src/core/datasets/engines.ts:11
- This agent-authored change adds a user-visible Polars option to the SQL engine selector, but the PR includes no visual evidence. Please attach screenshots or a short recording showing the selector and generated cell in edit view and the result in run/app view, and confirm the missing-package and unsupported-dataframe error states were checked.
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This pull request was authored by a coding agent.
📝 Summary
Closes #10810.
Adds Polars as a built-in SQL cell engine, selectable from the SQL editor and through
mo.sql(..., engine="polars").DataFrameandLazyFramevariables using a freshpolars.SQLContext.auto,native, andlazy-polarsoutput modes, collecting only for explicit eager Polars or pandas output.SQLGlot note
SQLGlot remains optional. Since marimo constructs the reactive graph before missing notebook packages can be dynamically installed, Polars reference discovery includes a dependency-free fallback scanner and augments it with SQLGlot when available. SQLGlot also currently has no dedicated Polars dialect, so the fallback covers supported Polars syntax that the existing DuckDB-dialect parse may reject. Making SQLGlot a core dependency could simplify this in a follow-up. This mainly affects the diff in sql_visitor.py.
🧪 Tests
.venv/bin/pytest -q tests/_sql/test_polars.py tests/_sql/test_sql.py tests/_ast/test_visitor.py tests/_runtime/test_runtime.py -k "polars_sql or TestPolarsEngine or TestSQLDepsEndToEnd or applies_limit_to_pandas"— 47 passedpnpm test src/core/wasm/__tests__/utils.test.ts src/core/wasm/__tests__/bridge.test.ts— 30 passedpnpm test src/core/codemirror/language/__tests__/sql.test.ts— 79 passed📋 Pre-Review Checklist
✅ Merge Checklist
Screenshot