gabotorresruiz commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4137074785
##########
superset/mcp_service/sql_lab/schemas.py:
##########
@@ -72,9 +72,12 @@ class ExecuteSqlRequest(BaseModel):
limit: int | None = Field(
default=None,
description=(
- "Maximum number of rows to return. "
- "If not specified, respects the LIMIT in your SQL query. "
- "If specified, overrides any SQL LIMIT clause."
+ "Maximum rows returned. "
Review Comment:
Just a small NIT on the first line. "Maximum rows returned" is not quite
true for a multi statement script, since only the last statement is capped. On
this branch `SELECT n FROM numbers ORDER BY n; SELECT n FROM numbers ORDER BY n
LIMIT 8` with `limit: 3` came back with `statements[0].row_count` 5000 and
`statements[1].row_count` 3, so the response carries 5000 rows for a `limit: 3`
request. `"Maximum rows returned by the last statement. "` would match what the
rest of the description already says.
##########
superset/sql/parse.py:
##########
@@ -1784,22 +1794,65 @@ def _has_limit_by(self) -> bool:
def get_limit_value(self) -> int | None:
"""
- Parse a SQL query and return the `LIMIT` or `TOP` value, if present.
+ Return a fixed outer `LIMIT`, `TOP`, or `FETCH` row count, if known.
"""
# `LIMIT 2 BY id` bounds each group, not the result set, so reporting 2
# here would make `_set_query_limit()` clamp the whole query to 2 rows.
if self._has_limit_by():
return None
if limit_node := self._parsed.args.get("limit"):
- literal = limit_node.args.get("expression") or getattr(
- limit_node, "this", None
- )
+ options = limit_node.args.get("limit_options")
+ if options and (
+ options.args.get("percent") or options.args.get("with_ties")
+ ):
+ return None
Review Comment:
Not a blocker, and I think `None` is the more honest answer here. But this
also moves SQL Lab, which the release note does not mention.
`get_limit_value` feeds `BaseEngineSpec.get_limit_from_sql`, which
`_set_query_limit` consumes at `superset/commands/sql_lab/execute.py:255-269`.
I ran that resolution on this branch and on the merge base with a 1000 row
dropdown. For SQL Server, `SELECT TOP 5 PERCENT * FROM t` and `SELECT TOP 5
WITH TIES * FROM t ORDER BY n` go from `query.limit` 5 with `limiting_factor`
`QUERY` to `query.limit` 1000 with `limiting_factor` `DROPDOWN`, and
`superset/sql_lab.py:299` then rewrites them to `TOP 1001`. So a SQL Server
user running `TOP 5 PERCENT` in SQL Lab now pulls up to 1000 rows where they
used to get 5. The other direction is a clear win: Postgres `FETCH FIRST 5 ROWS
ONLY` and SQLite `LIMIT (5)` go from the 1000 row dropdown to the 5 the SQL
asked for.
Is the SQL Server direction intended? If so, `UPDATING.md` is probably worth
a line, since the note currently scopes the change to MCP `execute_sql` and
`Database.execute`.
##########
superset/sql/execution/executor.py:
##########
@@ -271,7 +324,18 @@ def execute_sql_with_cursor(
# Fetch results from ALL statements
description = cursor.description
if description:
- rows = database.db_engine_spec.fetch_data(cursor)
+ fetch_cursor = cursor
+ # SQL restrictions cannot always be safely wrapped or replaced.
+ # Match SQL limit application: cap only the last statement, and
+ # only when the caller supplied a limit (also honoring
SQL_MAX_ROW).
+ if i == total - 1 and query.limit is not None:
+ row_limit: int = query.limit
+ if sql_max_row := app.config.get("SQL_MAX_ROW"):
+ row_limit = min(row_limit, sql_max_row)
+ fetch_cursor = _LimitedCursor(cursor, row_limit)
Review Comment:
This block worries me a bit, not because the cap is wrong but because it is
invisible to the caller. When the statement could not carry the cap in SQL,
`executed_sql` is the only record the caller has of what bounded the result,
and here it stops being one.
Driving the real `execute_sql` tool through a FastMCP client against a real
SQLite database on this branch:
- `PRAGMA compile_options` returns 73 rows with no `limit` and 3 rows with
`limit: 3`. `executed_sql` is the unmodified `PRAGMA compile_options` in both
cases, and nothing else in the response says the result was cut.
`set_limit_value` skips non query statements on purpose
(`superset/sql/parse.py:1864-1876`, "`SHOW` returns bounded metadata, so there
is nothing to truncate"), so the cursor budget now truncates exactly the
statements the parser refuses to truncate.
- `INSERT INTO numbers VALUES (90001), (90002), (90003) RETURNING n` returns
`row_count: 3` with no `limit` and `row_count: 2` with `limit: 2`, same
`executed_sql` both times and `affected_rows: null` both times. The caller
cannot tell "this write touched 2 rows" from "it touched 3 and one was dropped".
SQL Lab signals this by overfetching one row and setting `limiting_factor`
(`superset/sql_lab.py:350-356`), which is the same code path the description
already cites as precedent for the `min()`. Would a `truncated` flag on
`StatementResult` and `StatementInfo` be enough here? `_LimitedCursor` is the
natural place to compute it, though as in SQL Lab it would have to read one row
past the budget to tell "exactly at the cap" from "cut".
`test_execute_caps_rows_without_sql_rewrite` asserting
`statements[-1].truncated is True` would lock it in; the current tests miss it
because they only assert row counts, which look identical either way.
--
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]
