codeant-ai-for-open-source[bot] commented on code in PR #42412:
URL: https://github.com/apache/superset/pull/42412#discussion_r3662917650
##########
superset/commands/streaming_export/base.py:
##########
@@ -219,19 +218,44 @@ def _execute_query_and_stream(
delimiter = csv_export_config.get("sep", ",")
decimal_separator = csv_export_config.get("decimal", ".")
+ # Apply SQL mutations (e.g. SQL_QUERY_MUTATOR config hook) before
+ # execution. All non-streaming paths go through this — the streaming
+ # path was originally skipping it, which left trailing semicolons
+ # unstripped for engines like Trino that reject them.
+ sql = database.mutate_sql_based_on_config(sql)
+
with db.session(future=True) as session:
# Merge database to prevent DetachedInstanceError
merged_database = session.merge(database)
- with merged_database.get_sqla_engine(
- catalog=catalog, schema=schema
- ) as engine:
- with engine.connect() as connection:
- result_proxy = connection.execution_options(
- stream_results=True
- ).execute(text(sql))
-
- columns = list(result_proxy.keys())
+ # Use get_raw_connection() instead of get_sqla_engine() directly.
+ # This is critical for:
+ # 1. User impersonation — get_raw_connection() goes through the
+ # ENGINE_CONTEXT_MANAGER which applies impersonate_user settings
+ # (e.g. X-Trino-User header). Without this, all streaming CSV
+ # exports run as the service principal, breaking audit trails
+ # and potentially bypassing per-user authorization (Ranger, OPA,
+ # RLS views).
+ # 2. SSH tunnels — get_raw_connection() sets up SSH tunnels if
+ # configured on the database.
+ # 3. OAuth2 — get_raw_connection() wraps execution in
+ # check_for_oauth2() context.
+ with closing(
+ merged_database.get_raw_connection(catalog=catalog,
schema=schema)
+ ) as conn:
+ cursor = conn.cursor()
+ # Set cursor.arraysize to control the batch size for
fetchmany().
+ # This ensures DBAPI drivers (Trino, PostgreSQL, etc.) fetch
+ # rows in manageable chunks instead of buffering the entire
+ # result set client-side.
+ cursor.arraysize = limit
Review Comment:
**Suggestion:** `limit` is `None` for chart exports and unlimited SQL Lab
exports, but DBAPI `cursor.arraysize` requires an integer. This assignment can
raise a driver-specific type error for every unlimited streaming export, and it
is unnecessary because `_process_rows()` already passes `self._chunk_size`
explicitly to `fetchmany()`. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unlimited chart CSV exports can fail during cursor setup.
- ❌ Unlimited SQL Lab exports can fail during cursor setup.
- ⚠️ Driver behavior becomes database-dependent.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Request a chart streaming CSV export through
superset/charts/data/api.py:756-765;
StreamingCSVExportCommand._get_row_limit() explicitly returns None for every
chart export
at superset/commands/chart/data/streaming_export_command.py:76-83.
2. After entering the raw connection context, execution reaches
BaseStreamingCSVExportCommand._execute_query_and_stream() at
superset/commands/streaming_export/base.py:194.
3. The code assigns None to cursor.arraysize at
superset/commands/streaming_export/base.py:251. DBAPI cursor arraysize is an
integer
fetch-size attribute, and drivers may reject None with a TypeError or
equivalent driver
error.
4. The subsequent _process_rows() call at base.py:281-283 already invokes
fetchmany(self._chunk_size), so arraysize is not needed to implement the
configured batch
size. The export therefore fails for unlimited chart exports before rows are
streamed; SQL
Lab exports with select_sql set also return None from
superset/commands/sql_lab/streaming_export_command.py:119-127.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a449922712ac4e209ee00944ef0badd8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a449922712ac4e209ee00944ef0badd8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/streaming_export/base.py
**Line:** 251:251
**Comment:**
*Type Error: `limit` is `None` for chart exports and unlimited SQL Lab
exports, but DBAPI `cursor.arraysize` requires an integer. This assignment can
raise a driver-specific type error for every unlimited streaming export, and it
is unnecessary because `_process_rows()` already passes `self._chunk_size`
explicitly to `fetchmany()`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42412&comment_hash=f92dccbf585eecc59380d1ef20fa4a92a181a3692a9060f6cae6b98760e05a9e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42412&comment_hash=f92dccbf585eecc59380d1ef20fa4a92a181a3692a9060f6cae6b98760e05a9e&reaction=dislike'>👎</a>
##########
superset/commands/streaming_export/base.py:
##########
@@ -219,19 +218,44 @@ def _execute_query_and_stream(
delimiter = csv_export_config.get("sep", ",")
decimal_separator = csv_export_config.get("decimal", ".")
+ # Apply SQL mutations (e.g. SQL_QUERY_MUTATOR config hook) before
+ # execution. All non-streaming paths go through this — the streaming
+ # path was originally skipping it, which left trailing semicolons
+ # unstripped for engines like Trino that reject them.
+ sql = database.mutate_sql_based_on_config(sql)
Review Comment:
**Suggestion:** The SQL mutator receives the original database object before
it is merged into the active session. Streaming generators run after the
request session may have ended, so a configured `SQL_QUERY_MUTATOR` that reads
a lazy or session-bound database attribute can raise `DetachedInstanceError`.
Merge the database first and invoke the mutator with the merged instance.
[stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Configured database-aware SQL mutators can fail during exports.
- ⚠️ Streaming response timing exposes detached ORM state.
- ⚠️ Affected SQL mutation and audit customization paths.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start a chart or SQL Lab streaming export through
superset/charts/data/api.py:756-765
or superset/sqllab/api.py:417-435. BaseStreamingCSVExportCommand.run()
captures the
database and returns a generator at
superset/commands/streaming_export/base.py:129-145, so
query execution occurs during response streaming rather than during command
construction.
2. Configure SQL_QUERY_MUTATOR with a function that uses the supplied
database keyword
argument, which is explicitly passed by
Database.mutate_sql_based_on_config() at
superset/models/core.py:787-805.
3. When the generator reaches
superset/commands/streaming_export/base.py:225, it invokes
the mutator on the original database object before entering
db.session(future=True) and
before session.merge(database) at base.py:227-230.
4. Because the generator runs after the original request work may have
released or
detached its ORM session, a mutator that reads a deferred or session-bound
Database
attribute can raise DetachedInstanceError. The merged instance is available
at base.py:229
and should be used for mutation before opening the connection.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5dee3a490c6f4cd39fe13668d095ae61&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5dee3a490c6f4cd39fe13668d095ae61&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/streaming_export/base.py
**Line:** 225:225
**Comment:**
*Stale Reference: The SQL mutator receives the original database object
before it is merged into the active session. Streaming generators run after the
request session may have ended, so a configured `SQL_QUERY_MUTATOR` that reads
a lazy or session-bound database attribute can raise `DetachedInstanceError`.
Merge the database first and invoke the mutator with the merged instance.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42412&comment_hash=8e868db8d85d33066d37c35ad60a1e711e6022287859bb5e49ffcd1c117371d3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42412&comment_hash=8e868db8d85d33066d37c35ad60a1e711e6022287859bb5e49ffcd1c117371d3&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
