Skip to content

Guard SQL ref extraction on sqlglot availability#9656

Merged
mscolnick merged 1 commit into
mainfrom
ms/protect-sql-glot
May 21, 2026
Merged

Guard SQL ref extraction on sqlglot availability#9656
mscolnick merged 1 commit into
mainfrom
ms/protect-sql-glot

Conversation

@mscolnick

@mscolnick mscolnick commented May 21, 2026

Copy link
Copy Markdown
Contributor

Skip find_sql_refs when sqlglot isn't installed instead of raising

Copilot AI review requested due to automatic review settings May 21, 2026 21:06
@vercel

vercel Bot commented May 21, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview, Comment May 21, 2026 9:10pm

Request Review

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

Architecture diagram
sequenceDiagram
    participant UI as Optional Features UI
    participant App as App Config
    participant Cell as Cell Visitor
    participant Deps as DependencyManager
    participant Pkg as Package Manager
    participant Sqlglot as sqlglot library

    Note over UI,Sqlglot: SQL Reference Extraction Flow

    UI->>App: Display optional features
    App->>Deps: Check dependency availability
    Deps->>Pkg: Check if sqlglot[c] is installed
    
    alt sqlglot[c] is installed
        Pkg-->>Deps: Available
        Deps-->>App: sqlglot available
        App-->>UI: Show SQL feature as enabled
    else sqlglot[c] not installed
        Pkg-->>Deps: Not found
        Deps-->>App: sqlglot not available
        App-->>UI: Show SQL feature as disabled with install hint
    end

    Note over Cell,Pkg: Cell Execution & Reference Extraction

    Cell->>Deps: Check has_sqlglot
    Deps-->>Cell: boolean result

    opt sqlglot is available
        Cell->>Sqlglot: find_sql_refs_cached(statement_sql)
        Sqlglot-->>Cell: sql_refs list
        
        loop For each ref
            Cell->>Cell: Check if ref name in defined_names
            alt Name not already defined
                Cell->>Cell: _add_ref(name, sql_ref=ref)
            end
        end
    end

    Note over Cell,Pkg: When sqlglot is unavailable, extraction is skipped silently
Loading

Re-trigger cubic

Skip find_sql_refs when sqlglot isn't installed instead of raising
ModuleNotFoundError, so SQL cells still parse defs via duckdb.
@mscolnick mscolnick force-pushed the ms/protect-sql-glot branch from 28b0121 to 7732813 Compare May 21, 2026 21:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes SQL reference extraction resilient to sqlglot being absent, and updates install guidance to prefer the sqlglot[c] extra for better SQL parsing performance.

Changes:

  • Update DependencyManager.sqlglot to recommend installing sqlglot[c] instead of plain sqlglot.
  • Guard SQL reference extraction in the AST visitor so it is skipped (rather than raising) when sqlglot isn’t installed.
  • Update the frontend “Optional Features” SQL dependency spec to sqlglot[c].

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
marimo/_dependencies/dependencies.py Changes the suggested install target for sqlglot to sqlglot[c].
marimo/_ast/visitor.py Skips find_sql_refs calls when sqlglot isn’t available.
frontend/src/components/app-config/optional-features.tsx Updates the optional SQL feature dependency string to sqlglot[c].

@mscolnick mscolnick added the bug Something isn't working label May 21, 2026
@mscolnick mscolnick merged commit 51b5379 into main May 21, 2026
29 of 46 checks passed
@mscolnick mscolnick deleted the ms/protect-sql-glot branch May 21, 2026 21:15
@github-actions

Copy link
Copy Markdown

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.23.7-dev89

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants