aminghadersohi commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4109802156
##########
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. "
+ "Omitted: respects SQL LIMIT. "
+ "If set, caps the last statement's outer LIMIT at min(SQL LIMIT, "
+ "this value), adding one if absent. "
Review Comment:
Invalid: in superset/mcp_service/sql_lab/schemas.py:77-78, "adding one if
absent" refers to adding an outer LIMIT clause, not adding a row. The executor
passes the requested cap unchanged (subject to SQL_MAX_ROW) at
superset/sql/execution/executor.py:847-856, and superset/sql/parse.py:744-746
inserts that cap when no limit exists; the schema and execution agree.
##########
superset/sql/parse.py:
##########
@@ -1770,22 +1780,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
+ if isinstance(limit_node, exp.Fetch):
+ literal = limit_node.args.get("count")
+ # FETCH FIRST ROW ONLY has an implicit count of one.
+ if literal is None:
+ return 1
+ else:
+ literal = limit_node.args.get("expression")
+ while isinstance(literal, exp.Paren):
+ literal = literal.this
if isinstance(literal, exp.Literal) and literal.is_int:
return int(literal.name)
return None
Review Comment:
Fixed in cad654d4384f2f4eafbd27943ada6c0cf6cfbe30:
Database.apply_limit_to_sql now uses cap_limit_value unless force is requested,
preserving smaller expression limits and LIMIT 0. Added 16 executable SQLite
regression cases covering expression/zero limits, smaller/larger caps,
FORCE_LIMIT/WRAP_SQL, and forced/non-forced calls; the requested suites plus
models/core_test.py pass (2,316 tests).
--
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]
