fix: more sql prepared statements and quoting#8431
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens SQL execution across multiple engines by centralizing identifier quoting and replacing unsafe string interpolation with parameterized queries where supported.
Changes:
- Added a shared
marimo._sql.sql_quotingmodule for dialect-aware identifier quoting and parsing of fully-qualified table names. - Introduced
execute_duckdb_sqlto run parameterized DuckDB queries while preserving kernel globals context. - Updated Redshift, ClickHouse, Ibis, and SQL summaries code paths to use quoting and/or parameters instead of raw interpolation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/_sql/test_sql_quoting.py |
New unit tests covering identifier/qualified-name quoting and FQN parsing. |
marimo/_sql/utils.py |
Adds execute_duckdb_sql helper for parameterized DuckDB execution in kernel context. |
marimo/_sql/sql_quoting.py |
New module implementing identifier quoting, qualified-name quoting, and FQN parsing. |
marimo/_sql/engines/redshift.py |
Uses quote_qualified_name when querying row counts and columns. |
marimo/_sql/engines/ibis.py |
Replaces interpolated information_schema query with parameterized execution. |
marimo/_sql/engines/clickhouse.py |
Quotes database/table identifiers and switches system table lookup to parameters. |
marimo/_data/sql_summaries.py |
Uses the new FQN parser + parameterized DuckDB queries for column type lookup. |
marimo/_data/get_datasets.py |
Reuses shared DuckDB identifier quoting helper instead of local implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # First, get the column info and data type | ||
| if "." in fully_qualified_table_name: | ||
| # Fully qualified table name | ||
| db_name, schema_name, table_name = _parse_fully_qualified_table_name( | ||
| db_name, schema_name, table_name = parse_fully_qualified_table_name( | ||
| fully_qualified_table_name | ||
| ) |
There was a problem hiding this comment.
get_column_type decides “fully-qualified” vs “simple” table name using a raw "." in fully_qualified_table_name check. That means a single quoted identifier that legitimately contains dots (e.g. "my.table") will be treated as a 3-part name and then rejected by parse_fully_qualified_table_name. Consider trying to parse as FQN in a try/except and falling back to the simple-table path if parsing fails.
There was a problem hiding this comment.
this is maybe an existing bug that we can fix in a followup
marimo/_sql/sql_quoting.py
Outdated
| else: | ||
| # Default to double-quote (ANSI SQL standard) | ||
| escaped = identifier.replace('"', '""') | ||
| return f'"{escaped}"' |
There was a problem hiding this comment.
I think maybe we don't want to quote by default. It can fail/treated as literals for certain databasess
marimo/_sql/sql_quoting.py
Outdated
| # Double-quote style: escape embedded " as "" | ||
| escaped = identifier.replace('"', '""') | ||
| return f'"{escaped}"' | ||
| elif dialect in ("clickhouse", "mysql"): |
There was a problem hiding this comment.
should be able to include bigquery: https://docs.cloud.google.com/bigquery/docs/reference/standard-sql/lexical
quote with backticks
sql_quotingmodule with dialect-aware identifier quoting (double-quote for DuckDB/Redshift/Postgres, backtick for ClickHouse/MySQL) and a parser for quoted fully-qualified table namesexecute_duckdb_sqlhelper that supports parameterized queries ($1,$2, ...) while preserving the kernel globals context thatwrapped_sqlprovides