You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/09/24 09:51:32 UTC
[PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
aminghadersohi opened a new pull request, #44604:
URL: https://github.com/apache/superset/pull/44604
### SUMMARY
SC-121813
This is a deliberate **contract change**, not a restoration of the documented behavior. The MCP `execute_sql` schema previously said: **"If specified, overrides any SQL LIMIT clause."** An explicit request `limit` is instead a safety cap: use the smaller of the last statement's outer SQL limit and the request limit, subject to the existing `SQL_MAX_ROW` clamp. A stricter SQL limit, including zero, must never be enlarged.
- Root cause at base `8141d666`: `execute_sql` → `Database.execute` → `SQLExecutor._apply_limit_to_script` in `superset/sql/execution/executor.py:789` unconditionally called `set_limit_value`. This path has no `force` flag.
- Use the parser's `get_limit_value` / `set_limit_value` APIs. Skip rewriting when the outer limit is already equal or smaller. Only the last statement is eligible; earlier statements, inner/CTE limits and offsets remain intact.
- Preserve SC-100195's omitted-limit behavior from `d5cf77cd60beaa2b82b6dd2ed1cf50a70c0e2b77` (#38277): request `limit` defaults to `None`, leaving SQL limits unchanged.
- Correct SQL Lab precedent: `superset/commands/sql_lab/execute.py:267` computes `min()` of SQL and request limits, then `superset/sql_lab.py:299` applies that result plus one for overfetch detection. SQL Lab's execution path does **not** use `Database.apply_limit_to_sql`.
- Scope: the executor is shared by the public `Database.execute()` and `Database.execute_async()` APIs. Repository caller searches found no other non-MCP production callers. External API consumers also receive cap semantics, including dry runs; callers wanting more rows must explicitly increase/remove their SQL limit. Documented in `QueryOptions`, the MCP schema, admin docs and `UPDATING.md`.
- No changes to authorization, non-destructive validation, dry-run handling, result-size guards or the existing `SQL_MAX_ROW` clamp.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
| SQL outer limit | Request limit | Before | After |
| --- | --- | --- | --- |
| 5 | 10 | 10 | 5 |
| 10 | 5 | 5 | 5 |
| 5 | 5 | 5 | 5 |
| 5 | omitted | 5 | 5 |
| absent | 5 | 5 | 5 |
| 0 | 10 | 10 | 0 |
### TESTING INSTRUCTIONS
Local validation:
```bash
PYTHONPATH="$PWD:$PWD/superset-core/src" pytest -q tests/unit_tests/sql/execution tests/unit_tests/mcp_service/sql_lab --disable-warnings
PYTHONPATH="$PWD:$PWD/superset-core/src" pre-commit run
```
- **268 passed, 0 failed**, including 35 added cases.
- Coverage: both cap directions, equal/omitted/absent limits, LIMIT 0, OFFSET, CTE/subquery inner limits, multi-statement scope, SQL_MAX_ROW, DB2's non-default WRAP_SQL method, sync/async dry runs, six real in-memory SQLite row-count cases, and the published MCP tool schema.
- Pre-commit passed on the explicitly staged changed paths, including mypy, formatting, linting and Pylint.
Manual verification on a local test database: call `execute_sql` with a read-only SELECT containing `LIMIT 5` and request `limit: 10`; confirm `executed_sql` retains 5 and `row_count <= 5`. Reverse the limits and verify 5; omit the request limit and verify the SQL limit is unchanged. Repeat with `dry_run: true` to inspect transformed SQL without execution.
Staging reproduction and deployed revision remain **UNVERIFIED**. No Preset workspace was accessed or modified.
### ADDITIONAL INFORMATION
- [x] Has associated issue: SC-121813
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5843065942
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4160493677
##########
superset/sql/execution/executor.py:
##########
@@ -186,6 +187,65 @@ def build_statement_blocks(
return parsed_script, blocks
+class _LimitedCursor:
+ """Bound cumulative cursor reads without bypassing engine fetch processing."""
+
+ def __init__(self, cursor: Any, limit: int) -> None:
+ """Wrap a cursor with a shared budget for all row-reading methods."""
+ self._cursor = cursor
+ self._remaining = limit
+
+ def __getattr__(self, name: str) -> Any:
+ """Delegate metadata and driver-specific methods to the real cursor."""
+ return getattr(self._cursor, name)
+
+ @property
+ def arraysize(self) -> int:
+ """Expose the driver's default fetch batch size."""
+ return self._cursor.arraysize
+
+ @arraysize.setter
+ def arraysize(self, value: int) -> None:
+ """Preserve engine-specific cursor batch-size configuration."""
+ self._cursor.arraysize = value
+
+ def fetchmany(self, size: int | None = None) -> list[Any]:
+ """Read no more than the remaining budget, including across batches."""
+ size = self.arraysize if size is None else size
+ size = max(0, min(size, self._remaining))
+ # Some drivers interpret zero as unbounded, so do not call them at all.
+ if not size:
+ return []
+ rows = self._cursor.fetchmany(size)
+ self._remaining -= len(rows)
+ return rows
+
+ def check_truncated(self, db_engine_spec: type[BaseEngineSpec]) -> bool:
+ """Probe one extra row using the engine's fetch and error handling."""
+ return self._remaining == 0 and bool(
+ db_engine_spec.fetch_data(_LimitedCursor(self._cursor, 1))
+ )
Review Comment:
This block worries me a bit. The probe runs after `rows` has already been fully fetched, so a driver error on that one extra round trip discards a materialised result set and fails a query that had its data in hand.
I verified it on this branch with a cursor that serves the five requested rows and then raises on the next fetch:
- probe active: `QueryStatus.FAILED`, `sqlite error: transient read failure`, no statements returned, two driver fetches.
- same cursor with `check_truncated` stubbed to `False`: `QueryStatus.SUCCESS`, 5 rows, one driver fetch.
I can see `test_execute_truncation_probe_engine_errors` asserts exactly this for `OSError`, and the Drill `RuntimeError` case shows where you drew the line, so I read it as deliberate. What I am unsure about is the shape of the trade. The trigger is "the result reached the cap", which is the normal case for a capped query on a large table, so every such query against a network engine now carries a new failure window in exchange for a boolean. Would degrading be better here?
```python
def check_truncated(self, db_engine_spec: type[BaseEngineSpec]) -> bool:
"""Probe one extra row using the engine's fetch and error handling."""
if self._remaining != 0:
return False
try:
return bool(db_engine_spec.fetch_data(_LimitedCursor(self._cursor, 1)))
except Exception: # pylint: disable=broad-except
logger.warning("Truncation probe failed; reporting the result as partial")
return True
```
That keeps the honest answer (the caller is told the result may be partial) without losing rows the engine already handed over. An extra case in `test_execute_truncation_probe_engine_errors` asserting the rows survive an `OSError` with `truncated is True` would pin it. Or am I misunderstanding something, and a failed probe really should fail the query?
##########
superset/db_engine_specs/bigquery.py:
##########
@@ -488,16 +494,15 @@ def fetch_data(cls, cursor: Any, limit: int | None = None) -> list[tuple[Any, ..
if has_request_context():
g.bq_memory_limited = memory_limited
g.bq_memory_limited_row_count = len(data)
- return data
-
- except Exception: # pylint: disable=broad-except
- # Broad catch on purpose: any failure in the size-estimation /
- # progressive-fetch path (BigQuery DB-API errors, network or
- # auth timeouts mid-fetch, ``sys.getsizeof`` raising on an
- # unexpected cell type, or a future ``Row`` subclass we don't
- # know how to unwrap) must degrade gracefully to the parent's
- # straight fetch so the user still gets data.
- # Fallback to parent implementation
+ return FetchedRows(data, truncated=memory_limited)
+
+ except Exception as ex: # pylint: disable=broad-except
+ # A forward-only cursor cannot replay the consumed sample. Falling
+ # back after an estimation, second-batch, or EOF-probe failure would
+ # silently discard it and return only the remaining rows as success.
+ if first_batch:
+ raise cls.get_dbapi_mapped_exception(ex) from ex
Review Comment:
Just a small NIT, and I think this is the right call: falling back after a consumed sample would have returned rows 1001 onward as a success. It reaches further than the MCP contract though. `BigQueryEngineSpec.fetch_data` is also what legacy SQL Lab calls at `superset/sql_lab.py:351` and what dataset column discovery calls at `superset/connectors/sqla/utils.py:207`, so a BigQuery mid fetch error that used to degrade to a plain `fetchall()` now surfaces as a query error on those paths too. The `UPDATING.md` entry covers the limit contract and the SQL Lab limit change but not this one; a sentence there would reach operators who never open the MCP page.
##########
superset/sql/execution/executor.py:
##########
@@ -186,6 +187,65 @@ def build_statement_blocks(
return parsed_script, blocks
+class _LimitedCursor:
+ """Bound cumulative cursor reads without bypassing engine fetch processing."""
+
+ def __init__(self, cursor: Any, limit: int) -> None:
+ """Wrap a cursor with a shared budget for all row-reading methods."""
+ self._cursor = cursor
+ self._remaining = limit
+
+ def __getattr__(self, name: str) -> Any:
+ """Delegate metadata and driver-specific methods to the real cursor."""
+ return getattr(self._cursor, name)
+
+ @property
+ def arraysize(self) -> int:
+ """Expose the driver's default fetch batch size."""
+ return self._cursor.arraysize
+
+ @arraysize.setter
+ def arraysize(self, value: int) -> None:
+ """Preserve engine-specific cursor batch-size configuration."""
+ self._cursor.arraysize = value
+
+ def fetchmany(self, size: int | None = None) -> list[Any]:
+ """Read no more than the remaining budget, including across batches."""
+ size = self.arraysize if size is None else size
+ size = max(0, min(size, self._remaining))
+ # Some drivers interpret zero as unbounded, so do not call them at all.
+ if not size:
+ return []
+ rows = self._cursor.fetchmany(size)
+ self._remaining -= len(rows)
+ return rows
+
+ def check_truncated(self, db_engine_spec: type[BaseEngineSpec]) -> bool:
+ """Probe one extra row using the engine's fetch and error handling."""
+ return self._remaining == 0 and bool(
+ db_engine_spec.fetch_data(_LimitedCursor(self._cursor, 1))
+ )
+
+ def fetchall(self) -> list[Any]:
+ """Translate an unbounded read into a bounded driver fetch."""
+ return self.fetchmany(self._remaining)
Review Comment:
Not a blocker, and it is the note I dropped last time, but the new flag raises the stakes enough that I want to put it on the record. `fetchall()` issues a single `fetchmany(self._remaining)`, and PEP 249 lets a driver return fewer rows than `size` without being exhausted. When that happens the budget is not consumed, so `check_truncated` short circuits to `False` and the response now affirmatively reports a short result as complete.
Measured on this branch with a cursor that caps every batch at two rows, ten rows available and a budget of five: `BaseEngineSpec.fetch_data` returns 2 rows, `_remaining` stays at 3, and `check_truncated` reports `False`. Before this PR such a result was merely short; now it is short and labelled complete.
I still cannot name a driver in the tree that short batches, so this is cheap insurance rather than a known break. Looping in `fetchall()` until the budget is spent or a batch comes back empty would close it, and a `test_limited_cursor_shares_read_budget` case with a short batching mock would pin it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4141001725
##########
tests/unit_tests/db_engine_specs/test_bigquery.py:
##########
@@ -832,6 +869,43 @@ def test_fetch_data_fallback_on_exception(mocker: MockerFixture) -> None:
assert flask_g.bq_memory_limited_row_count == 2
+@pytest.mark.parametrize("bounded", [False, True])
+@pytest.mark.parametrize("failure_stage", ["probe", "second_batch", "estimate"])
+def test_fetch_data_does_not_discard_sample_after_error(
+ mocker: MockerFixture, bounded: bool, failure_stage: str
+) -> None:
+ """A failure after consuming rows must not return only the cursor remainder."""
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
+ from superset.sql.execution.executor import _LimitedCursor
+
+ _, app = _patch_bq_fetch_deps(mocker)
+ mocker.patch("superset.db_engine_specs.bigquery.has_app_context", return_value=True)
+ app.config = {"BQ_FETCH_MAX_MB": 1}
+ mocker.patch("superset.db_engine_specs.bigquery._BQ_INITIAL_SAMPLE_ROWS", 2)
+ mocker.patch.object(
+ BigQueryEngineSpec,
+ "get_dbapi_exception_mapping",
+ return_value={OSError: SupersetDBAPIConnectionError},
+ )
+ row = ("x" * (600_000 if failure_stage == "probe" else 300_000),)
Review Comment:
Fixed in 0c7cc2082c12c2c7019a6697cef5307316f87d01: named the probe and second-batch cell sizes and documented the 1 MiB budget math. Two roughly 600 KB rows exceed the budget and trigger the EOF probe; roughly 300 KB rows leave room for the second batch. All bounded/unbounded probe, second-batch, and estimation cases remain. The requested BigQuery and SQL execution suites pass (363 tests); all applicable pre-commit hooks pass, including mypy.
##########
tests/unit_tests/db_engine_specs/test_bigquery.py:
##########
@@ -832,6 +869,43 @@ def test_fetch_data_fallback_on_exception(mocker: MockerFixture) -> None:
assert flask_g.bq_memory_limited_row_count == 2
+@pytest.mark.parametrize("bounded", [False, True])
+@pytest.mark.parametrize("failure_stage", ["probe", "second_batch", "estimate"])
+def test_fetch_data_does_not_discard_sample_after_error(
+ mocker: MockerFixture, bounded: bool, failure_stage: str
+) -> None:
+ """A failure after consuming rows must not return only the cursor remainder."""
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
+ from superset.sql.execution.executor import _LimitedCursor
+
+ _, app = _patch_bq_fetch_deps(mocker)
+ mocker.patch("superset.db_engine_specs.bigquery.has_app_context", return_value=True)
+ app.config = {"BQ_FETCH_MAX_MB": 1}
+ mocker.patch("superset.db_engine_specs.bigquery._BQ_INITIAL_SAMPLE_ROWS", 2)
+ mocker.patch.object(
+ BigQueryEngineSpec,
+ "get_dbapi_exception_mapping",
+ return_value={OSError: SupersetDBAPIConnectionError},
+ )
+ row = ("x" * (600_000 if failure_stage == "probe" else 300_000),)
+ cursor = mock.MagicMock()
+ cursor.description = [("n", "STRING", None, None, None, None, None)]
+ cursor.fetchmany.side_effect = [[row, row], OSError("read failed"), [row]]
Review Comment:
Fixed in 0c7cc2082c12c2c7019a6697cef5307316f87d01: derive fetchmany side_effect from failure_stage. Estimation consumes only the initial sample before getsizeof raises; probe and second-batch cases consume the sample and then raise on the second fetch. Removed both unreachable entries while retaining all six error cases and their fetch-count and no-fetchall assertions. The requested BigQuery and SQL execution suites pass (363 tests); all applicable pre-commit hooks pass, including mypy.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4110242836
##########
superset/sql/execution/executor.py:
##########
@@ -186,6 +186,60 @@ def build_statement_blocks(
return parsed_script, blocks
+class _LimitedCursor:
+ """Bound cumulative cursor reads without bypassing engine fetch processing."""
+
+ def __init__(self, cursor: Any, limit: int) -> None:
+ """Wrap a cursor with a shared budget for all row-reading methods."""
+ self._cursor = cursor
+ self._remaining = limit
+
+ def __getattr__(self, name: str) -> Any:
+ """Delegate metadata and driver-specific methods to the real cursor."""
+ return getattr(self._cursor, name)
+
+ @property
+ def arraysize(self) -> int:
+ """Expose the driver's default fetch batch size."""
+ return self._cursor.arraysize
+
+ @arraysize.setter
+ def arraysize(self, value: int) -> None:
+ """Preserve engine-specific cursor batch-size configuration."""
+ self._cursor.arraysize = value
+
+ def fetchmany(self, size: int | None = None) -> list[Any]:
+ """Read no more than the remaining budget, including across batches."""
+ size = self.arraysize if size is None else size
+ size = max(0, min(size, self._remaining))
+ # Some drivers interpret zero as unbounded, so do not call them at all.
+ if not size:
+ return []
+ # Reserve before reading so an engine's error fallback cannot retry
+ # beyond the budget after a driver has partially consumed a batch.
+ self._remaining -= size
+ return self._cursor.fetchmany(size)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Budget reserved before driver error</b></div>
<div id="fix">
`_LimitedCursor.fetchmany` decrements `_remaining` before the driver call, so a mid-fetch driver error consumes budget for rows never returned. BigQuery's `fetch_data` fallback (`bigquery.py:501`) then calls `fetchall()`, which reads only post-consumption rows — silently dropping `first_batch` or returning `[]`. Decrement by `len(rows)` after the driver returns; returned rows still never exceed the limit.
</div>
</div>
<small><i>Code Review Run #d0eba2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4141002716
##########
tests/unit_tests/db_engine_specs/test_bigquery.py:
##########
@@ -832,6 +869,43 @@ def test_fetch_data_fallback_on_exception(mocker: MockerFixture) -> None:
assert flask_g.bq_memory_limited_row_count == 2
+@pytest.mark.parametrize("bounded", [False, True])
+@pytest.mark.parametrize("failure_stage", ["probe", "second_batch", "estimate"])
+def test_fetch_data_does_not_discard_sample_after_error(
+ mocker: MockerFixture, bounded: bool, failure_stage: str
+) -> None:
+ """A failure after consuming rows must not return only the cursor remainder."""
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
+ from superset.sql.execution.executor import _LimitedCursor
+
+ _, app = _patch_bq_fetch_deps(mocker)
+ mocker.patch("superset.db_engine_specs.bigquery.has_app_context", return_value=True)
+ app.config = {"BQ_FETCH_MAX_MB": 1}
+ mocker.patch("superset.db_engine_specs.bigquery._BQ_INITIAL_SAMPLE_ROWS", 2)
+ mocker.patch.object(
+ BigQueryEngineSpec,
+ "get_dbapi_exception_mapping",
+ return_value={OSError: SupersetDBAPIConnectionError},
+ )
+ row = ("x" * (600_000 if failure_stage == "probe" else 300_000),)
Review Comment:
<!-- Bito Reply -->
The suggestion to document the magic row sizes is valid and improves code maintainability by providing context for the 600,000 and 300,000 literals. Applying this comment ensures consistency with the existing documentation in the sibling test case.
**tests/unit_tests/db_engine_specs/test_bigquery.py**
```
row = ("x" * (600_000 if failure_stage == "probe" else 300_000),) # 600 KB rows exceed 1 MiB budget; 300 KB rows leave room
```
##########
tests/unit_tests/db_engine_specs/test_bigquery.py:
##########
@@ -832,6 +869,43 @@ def test_fetch_data_fallback_on_exception(mocker: MockerFixture) -> None:
assert flask_g.bq_memory_limited_row_count == 2
+@pytest.mark.parametrize("bounded", [False, True])
+@pytest.mark.parametrize("failure_stage", ["probe", "second_batch", "estimate"])
+def test_fetch_data_does_not_discard_sample_after_error(
+ mocker: MockerFixture, bounded: bool, failure_stage: str
+) -> None:
+ """A failure after consuming rows must not return only the cursor remainder."""
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
+ from superset.sql.execution.executor import _LimitedCursor
+
+ _, app = _patch_bq_fetch_deps(mocker)
+ mocker.patch("superset.db_engine_specs.bigquery.has_app_context", return_value=True)
+ app.config = {"BQ_FETCH_MAX_MB": 1}
+ mocker.patch("superset.db_engine_specs.bigquery._BQ_INITIAL_SAMPLE_ROWS", 2)
+ mocker.patch.object(
+ BigQueryEngineSpec,
+ "get_dbapi_exception_mapping",
+ return_value={OSError: SupersetDBAPIConnectionError},
+ )
+ row = ("x" * (600_000 if failure_stage == "probe" else 300_000),)
+ cursor = mock.MagicMock()
+ cursor.description = [("n", "STRING", None, None, None, None, None)]
+ cursor.fetchmany.side_effect = [[row, row], OSError("read failed"), [row]]
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is valid and improves the test code. It correctly identifies that the shared `side_effect` list in the parametrization does not accurately reflect the execution flow for all `failure_stage` scenarios, as some stages cause the test to fail before consuming all elements in the list. Applying this change ensures the test logic precisely matches the expected behavior for each failure scenario.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5902702087
Full-diff review completed at **a50d2b604b44746dbdcea3cf6ae4d2f24a82cc47**.
- **Fixed (P2):** BigQuery's EOF-probe error could trigger fallback after consuming the sample, silently returning only the remaining rows as success. Commit **a50d2b604b** propagates errors after a successful read while preserving initial-fetch fallback. Six regression cases failed before the fix and pass afterward; documentation updated.
- **Gabriel's Sept 29 review:** all three asks verified closed: truncation signaling through sync/async/cache/MCP responses, SQL Lab release notes, and last-statement schema wording.
- **Bito's Sept 30 review:** appending the probe remains correctly rejected—it exceeds the cap; existing consuming-cursor tests verify explicit truncation. The sample-size test suggestion is addressed in the new commit using the engine constant.
- No other correctness, regression, or security findings; no outstanding review asks.
Validation: affected SQL, MCP SQL Lab, model, connector, engine-spec, and SQL Lab command suites: **4,288 passed, 7 skipped**. All **12 applicable pre-commit hooks passed**, including mypy, across all PR-changed files. No live BigQuery/SQL Server validation; no services or containers started.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4110813658
##########
superset/sql/execution/executor.py:
##########
@@ -186,6 +186,60 @@ def build_statement_blocks(
return parsed_script, blocks
+class _LimitedCursor:
+ """Bound cumulative cursor reads without bypassing engine fetch processing."""
+
+ def __init__(self, cursor: Any, limit: int) -> None:
+ """Wrap a cursor with a shared budget for all row-reading methods."""
+ self._cursor = cursor
+ self._remaining = limit
+
+ def __getattr__(self, name: str) -> Any:
+ """Delegate metadata and driver-specific methods to the real cursor."""
+ return getattr(self._cursor, name)
+
+ @property
+ def arraysize(self) -> int:
+ """Expose the driver's default fetch batch size."""
+ return self._cursor.arraysize
+
+ @arraysize.setter
+ def arraysize(self, value: int) -> None:
+ """Preserve engine-specific cursor batch-size configuration."""
+ self._cursor.arraysize = value
+
+ def fetchmany(self, size: int | None = None) -> list[Any]:
+ """Read no more than the remaining budget, including across batches."""
+ size = self.arraysize if size is None else size
+ size = max(0, min(size, self._remaining))
+ # Some drivers interpret zero as unbounded, so do not call them at all.
+ if not size:
+ return []
+ # Reserve before reading so an engine's error fallback cannot retry
+ # beyond the budget after a driver has partially consumed a batch.
+ self._remaining -= size
+ return self._cursor.fetchmany(size)
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate and improves the code. Decrementing the budget after the driver returns ensures that the budget is only consumed by rows actually retrieved, preventing the silent data loss described in the review comment when a driver error occurs.
**superset/sql/execution/executor.py**
```
def fetchmany(self, size: int | None = None) -> list[Any]:
"""Read no more than the remaining budget, including across batches."""
size = self.arraysize if size is None else size
size = max(0, min(size, self._remaining))
if not size:
return []
rows = self._cursor.fetchmany(size)
self._remaining -= len(rows)
return rows
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5841850267
<!-- Bito Reply -->
The review comment correctly identifies a discrepancy between the schema documentation and the actual implementation behavior. The schema description in `superset/mcp_service/sql_lab/schemas.py` states that an absent SQL limit adds one row, but the implementation logic (as described in the updated documentation and the PR changes) is designed to cap the result set at the minimum of the SQL limit and the request limit, rather than simply adding a row.
To resolve this, the schema description should be updated to accurately reflect the capping behavior. The current change in the PR already attempts to clarify this, but the comment suggests the wording remains confusing or inconsistent with the actual execution logic. I recommend updating the `description` field in `ExecuteSqlRequest` to clearly state that it acts as a safety cap on the outer SQL limit, rather than implying it adds a row.
**superset/mcp_service/sql_lab/schemas.py**
```
description=(
"Maximum rows returned. "
"Omitted: respects SQL LIMIT. "
"If set, acts as a safety cap on the last statement's outer LIMIT, "
"taking the minimum of the SQL LIMIT and this value. "
"Never raises stricter SQL limits or changes inner limits. "
"Explicit limits also obey server SQL_MAX_ROW."
),
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5907014718
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5943769268
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5897716273
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4139857491
##########
superset/db_engine_specs/bigquery.py:
##########
@@ -465,13 +465,17 @@ def fetch_data(cls, cursor: Any, limit: int | None = None) -> list[tuple[Any, ..
# First batch already covers the budget or the result set
if rows_fetched < initial_batch_size or remaining_rows <= 0:
+ # A full sample can also be the entire result. Confirm an omitted
+ # row; a bounded cursor leaves its own EOF probe to the executor.
memory_limited = (
- remaining_rows <= 0 and rows_fetched == initial_batch_size
+ remaining_rows <= 0
+ and rows_fetched == initial_batch_size
+ and bool(cursor.fetchmany(1))
Review Comment:
Not applicable: the probe row is deliberately outside the return budget, not a missing in-budget row. A non-empty probe makes `memory_limited=True`, and `FetchedRows(first_batch, truncated=True)` explicitly reports the omission. An empty probe preserves the complete sample with `truncated=False`. Appending the probe would exceed the cap (including an explicit request limit).
Evidence committed in 071a1868eeab45cf9a2e8d14e96c0db2bb4d9b8f: `test_fetch_data_eof_probe_preserves_budgeted_rows` passes all 9 cases against the unchanged production implementation. It uses distinct row values and a consuming iterator, with an exactly four-row memory budget, 3/4/5 available rows, and request limits None/4/8. It asserts the entire returned sample, truncation metadata, and driver calls: 3 and 4 rows are returned completely with truncated=False; 5 rows return exactly the first four with truncated=True. The fifth row is only the truncation probe.
Validation: `pytest -q tests/unit_tests/db_engine_specs tests/unit_tests/sql/execution` — 2,093 passed, 7 skipped. Pre-commit passed on the new test and all branch-changed files, including mypy (12 applicable hooks for the branch).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5849950412
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4110812853
##########
superset/sql/execution/executor.py:
##########
@@ -186,6 +186,60 @@ def build_statement_blocks(
return parsed_script, blocks
+class _LimitedCursor:
+ """Bound cumulative cursor reads without bypassing engine fetch processing."""
+
+ def __init__(self, cursor: Any, limit: int) -> None:
+ """Wrap a cursor with a shared budget for all row-reading methods."""
+ self._cursor = cursor
+ self._remaining = limit
+
+ def __getattr__(self, name: str) -> Any:
+ """Delegate metadata and driver-specific methods to the real cursor."""
+ return getattr(self._cursor, name)
+
+ @property
+ def arraysize(self) -> int:
+ """Expose the driver's default fetch batch size."""
+ return self._cursor.arraysize
+
+ @arraysize.setter
+ def arraysize(self, value: int) -> None:
+ """Preserve engine-specific cursor batch-size configuration."""
+ self._cursor.arraysize = value
+
+ def fetchmany(self, size: int | None = None) -> list[Any]:
+ """Read no more than the remaining budget, including across batches."""
+ size = self.arraysize if size is None else size
+ size = max(0, min(size, self._remaining))
+ # Some drivers interpret zero as unbounded, so do not call them at all.
+ if not size:
+ return []
+ # Reserve before reading so an engine's error fallback cannot retry
+ # beyond the budget after a driver has partially consumed a batch.
+ self._remaining -= size
+ return self._cursor.fetchmany(size)
Review Comment:
Fixed in e781f0d7c7: the cursor budget is decremented only by rows successfully returned, while each driver fetch remains capped by the remaining budget. Regression tests cover fetchall/fetchmany retries and the BigQuery fallback.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5901495719
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5838983281
Fixed the derived-table regression in `dbf7abc17072cdf4e18be24465d0a6bb06e05c0e`.
**Approach:** SQL Server `TOP ... PERCENT`, `WITH TIES`, and nonliteral limits remain unchanged rather than being wrapped. This avoids introducing derived-table requirements for unique, named output columns—including unknown names behind `*`—without guessing schema or collation rules. These forms are still not treated as fixed row counts, and their original restrictions are never enlarged. Other dialects retain the outer-cap fallback.
**Returned-row cap:** The unified executor previously called `fetch_data(cursor)` without a downstream cap, so simply skipping the wrapper was insufficient. The shared sync/async path now truncates the last statement's rows to the explicit request limit, bounded by `SQL_MAX_ROW`, before constructing the result set: [`superset/sql/execution/executor.py:274–287`](https://github.com/apache/superset/blob/dbf7abc17072cdf4e18be24465d0a6bb06e05c0e/superset/sql/execution/executor.py#L274-L287). No request limit leaves existing behavior unchanged; earlier statements are not truncated.
**Validation:** Added failing regressions first for duplicate aliases, unnamed expressions, and joins with shared column names under both `PERCENT` and `WITH TIES`; also covered stars, case-variant aliases, nonliteral limits, and both limit methods. Real SQLite cursor tests verify downstream request/server caps, preservation of smaller results, and last-statement-only behavior independently of SQL rewriting. With pinned `sqlglot==30.18.0`, all 2,321 SQL executor/parser, MCP SQL Lab, and tool-inventory tests pass. Pre-commit passes on the touched files and all branch-changed files, including mypy.
**Tradeoff:** This caps returned results, not database work or rows fetched. SQL Server `dry_run` preserves the original complex restriction. Both points are documented. No live SQL Server was started or tested. The PR remains draft.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5846178172
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4161027088
##########
superset/sql/execution/executor.py:
##########
@@ -186,6 +187,65 @@ def build_statement_blocks(
return parsed_script, blocks
+class _LimitedCursor:
+ """Bound cumulative cursor reads without bypassing engine fetch processing."""
+
+ def __init__(self, cursor: Any, limit: int) -> None:
+ """Wrap a cursor with a shared budget for all row-reading methods."""
+ self._cursor = cursor
+ self._remaining = limit
+
+ def __getattr__(self, name: str) -> Any:
+ """Delegate metadata and driver-specific methods to the real cursor."""
+ return getattr(self._cursor, name)
+
+ @property
+ def arraysize(self) -> int:
+ """Expose the driver's default fetch batch size."""
+ return self._cursor.arraysize
+
+ @arraysize.setter
+ def arraysize(self, value: int) -> None:
+ """Preserve engine-specific cursor batch-size configuration."""
+ self._cursor.arraysize = value
+
+ def fetchmany(self, size: int | None = None) -> list[Any]:
+ """Read no more than the remaining budget, including across batches."""
+ size = self.arraysize if size is None else size
+ size = max(0, min(size, self._remaining))
+ # Some drivers interpret zero as unbounded, so do not call them at all.
+ if not size:
+ return []
+ rows = self._cursor.fetchmany(size)
+ self._remaining -= len(rows)
+ return rows
+
+ def check_truncated(self, db_engine_spec: type[BaseEngineSpec]) -> bool:
+ """Probe one extra row using the engine's fetch and error handling."""
+ return self._remaining == 0 and bool(
+ db_engine_spec.fetch_data(_LimitedCursor(self._cursor, 1))
+ )
Review Comment:
Agreed, degrading is the better trade. Done in 5eee71d8c7: `check_truncated` now catches a probe failure, logs a warning and returns `True`, so the rows already fetched within the budget survive and the caller is told the result may be partial. `test_execute_truncation_probe_engine_errors` now asserts the `OSError` case returns both rows with `truncated is True` (Drill EOF still returns `truncated is False`).
##########
superset/sql/execution/executor.py:
##########
@@ -186,6 +187,65 @@ def build_statement_blocks(
return parsed_script, blocks
+class _LimitedCursor:
+ """Bound cumulative cursor reads without bypassing engine fetch processing."""
+
+ def __init__(self, cursor: Any, limit: int) -> None:
+ """Wrap a cursor with a shared budget for all row-reading methods."""
+ self._cursor = cursor
+ self._remaining = limit
+
+ def __getattr__(self, name: str) -> Any:
+ """Delegate metadata and driver-specific methods to the real cursor."""
+ return getattr(self._cursor, name)
+
+ @property
+ def arraysize(self) -> int:
+ """Expose the driver's default fetch batch size."""
+ return self._cursor.arraysize
+
+ @arraysize.setter
+ def arraysize(self, value: int) -> None:
+ """Preserve engine-specific cursor batch-size configuration."""
+ self._cursor.arraysize = value
+
+ def fetchmany(self, size: int | None = None) -> list[Any]:
+ """Read no more than the remaining budget, including across batches."""
+ size = self.arraysize if size is None else size
+ size = max(0, min(size, self._remaining))
+ # Some drivers interpret zero as unbounded, so do not call them at all.
+ if not size:
+ return []
+ rows = self._cursor.fetchmany(size)
+ self._remaining -= len(rows)
+ return rows
+
+ def check_truncated(self, db_engine_spec: type[BaseEngineSpec]) -> bool:
+ """Probe one extra row using the engine's fetch and error handling."""
+ return self._remaining == 0 and bool(
+ db_engine_spec.fetch_data(_LimitedCursor(self._cursor, 1))
+ )
+
+ def fetchall(self) -> list[Any]:
+ """Translate an unbounded read into a bounded driver fetch."""
+ return self.fetchmany(self._remaining)
Review Comment:
Closed in 5eee71d8c7: `_LimitedCursor.fetchall()` now loops `fetchmany(remaining)` until the budget is spent or a batch comes back empty. New `test_limited_cursor_fetchall_reads_through_short_batches` uses a cursor that serves two rows per call with ten available and a budget of five: `fetch_data` returns 5 rows, the driver sees `fetchmany(5), (3), (1)` plus the one-row probe, and `check_truncated` reports `True`.
##########
superset/db_engine_specs/bigquery.py:
##########
@@ -488,16 +494,15 @@ def fetch_data(cls, cursor: Any, limit: int | None = None) -> list[tuple[Any, ..
if has_request_context():
g.bq_memory_limited = memory_limited
g.bq_memory_limited_row_count = len(data)
- return data
-
- except Exception: # pylint: disable=broad-except
- # Broad catch on purpose: any failure in the size-estimation /
- # progressive-fetch path (BigQuery DB-API errors, network or
- # auth timeouts mid-fetch, ``sys.getsizeof`` raising on an
- # unexpected cell type, or a future ``Row`` subclass we don't
- # know how to unwrap) must degrade gracefully to the parent's
- # straight fetch so the user still gets data.
- # Fallback to parent implementation
+ return FetchedRows(data, truncated=memory_limited)
+
+ except Exception as ex: # pylint: disable=broad-except
+ # A forward-only cursor cannot replay the consumed sample. Falling
+ # back after an estimation, second-batch, or EOF-probe failure would
+ # silently discard it and return only the remaining rows as success.
+ if first_batch:
+ raise cls.get_dbapi_mapped_exception(ex) from ex
Review Comment:
Added in 5eee71d8c7: `UPDATING.md` now says `BigQueryEngineSpec.fetch_data` no longer falls back to a plain `fetchall()` after its initial sample, so a mid-fetch driver error fails the query on every caller, including legacy SQL Lab execution and dataset column discovery, while an error on the initial read still falls back as before.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5903308318
Follow-up to the acceptance comment above: I checked the tracked acceptance criteria against that evidence.
- SQL limit smaller than the request, larger than it, or equal: PASS (5/8, 8/5, 5/5 rows above).
- Request limit omitted keeps the SQL limit; SQL without a limit gets the cap: PASS.
- LIMIT 0, OFFSET, nested/CTE limits left alone: PASS.
- Authorization and size guards unchanged; no regex rewriting: PASS. The diff touches no permission code, limits go through the sqlglot parser, and SQL_MAX_ROW still caps the result (9/10/11 → 9/10/10).
- Schema description and regression tests updated: PASS.
- Repro on a staging examples database: pending until this is deployed. It is read-only and will be recorded then.
Acceptance is complete for this head (a50d2b60) apart from that post-deploy check.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44604:
URL: https://github.com/apache/superset/pull/44604
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5811843772
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5811877578
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44604?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 81.10%. Comparing base ([`8141d66`](https://app.codecov.io/gh/apache/superset/commit/8141d666d6decade7de8406a0cc40c705289517d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`33dee87`](https://app.codecov.io/gh/apache/superset/commit/33dee877e35924263a67fc56be742d40ecb63644?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44604 +/- ##
==========================================
+ Coverage 81.07% 81.10% +0.03%
==========================================
Files 2955 2960 +5
Lines 178331 178958 +627
Branches 41313 41313
==========================================
+ Hits 144580 145152 +572
- Misses 31041 31096 +55
Partials 2710 2710
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44604/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset-extensions-cli](https://app.codecov.io/gh/apache/superset/pull/44604/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.24% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44604?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5903300402
### Acceptance at a50d2b604b44746dbdcea3cf6ae4d2f24a82cc47
**Executable PR/brief criteria: PASS. Overall acceptance remains incomplete:** the required linked-story criteria could not be read because its MCP is unavailable in this session, configured-server catalog, and reviewed catalog. No code changes or pushes.
Real Postgres 17; `SQL_MAX_ROW=10`; persisted local metadata and an Admin test user. No execution/parser/cursor mocks. Command: `SUPERSET_CONFIG_PATH=/tmp/r144/accept44604_config.py PYTHONPATH="$PWD:$PWD/superset-core/src" /tmp/ss-venv/bin/python /tmp/r144/accept44604.py` → **40 passed** (27 MCP, 13 SQL Lab API).
| Criterion / executed input | Result |
|---|---|
| MCP `Client(mcp).call_tool('execute_sql', {'request': …})`; `SELECT generate_series(1,20) AS n` with SQL/request limits 5/8, 8/5, 5/5, absent/5, 0/8 | PASS: 5, 5, 5, 5, 0 rows; stricter limits preserved. |
| Omitted request, SQL `LIMIT 15`; OFFSET, CTE/subquery, expression and FETCH FIRST variants; dry runs | PASS: omitted returns 15 unchanged; OFFSET values 3–7; inner-limit values 7–3; expression/FETCH return 5; dry runs retain `LIMIT 5`. |
| Request below/at/above server maximum: 9/10/11 | PASS: SELECT and unrewritable `INSERT … RETURNING` return 9/10/10. RETURNING reports `truncated=true`; exactly 10 available reports false. |
| Fetch truncation vs exact cap | PASS: `SHOW ALL`, cap 5 → 5/true; `SHOW server_version`, cap 1 → 1/false; RETURNING 6 vs 5 available, cap 5 → 5/true vs 5/false. SQL-rewritten SELECT reports false, as documented for fetch truncation. |
| Multi-statement last-only cap | PASS: SELECT script returns [20,5]; INSERT RETURNING plus COUNT returns [20,1], count=20, with cap 5. Earlier statement remains uncapped. |
| Flask client `POST /api/v1/sqllab/execute/`, `queryLimit=9/10/11` | PASS: HTTP 200; 9/10/10 rows; SQL overfetch limits 10/11/11. At cap 5, 4/5/6 available → 4/5/5 rows, `NOT_LIMITED`/`NOT_LIMITED`/`DROPDOWN`. Smaller/equal SQL limits, FETCH FIRST, parenthesized limits and multi-statement execution also pass. SQL Lab retains its separate per-statement behavior. |
Unit command: `pytest -q tests/unit_tests/{sql,mcp_service/sql_lab,models/core_test.py,connectors/sqla/utils_test.py,db_engine_specs,commands/sql_lab}` → **4,288 passed, 7 skipped**. BigQuery memory/EOF/error handling and Drill EOF/error mapping are **unit-test evidence only**, not live-service validation.
Existing findings rechecked: truncation/docs/schema fixed in f06ce97e7b, exact-sample detection in 53718267, buffered-row errors/sample-size nit in a50d2b60; probe-appending suggestion rebutted by 071a1868 tests. Earlier expression-limit and budget findings remain fixed in cad654d4/e781f0d7. All review threads already resolved; no reviewer requests changed.
`pre-commit run --files <all PR-changed paths>` → **12 hooks passed**. Working tree clean. Postgres container stopped/removed and its created volume verified removed. Remaining item: linked-story criteria verification.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4109802036
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `cad654d`.
`cap_limit_value` now detects non-fixed limits returned as `None` and wraps the query instead of replacing the existing expression, preserving potentially smaller limits.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4109685879
##########
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:
**Suggestion:** The schema says an absent SQL limit adds one row, but execution returns at most the requested limit, so clients receive inconsistent limiting behavior.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e3ea4ffe0afa49b4a2a73cc4853c9d0e&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=e3ea4ffe0afa49b4a2a73cc4853c9d0e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/sql_lab/schemas.py
**Line:** 77:78
**Comment:**
*Api Mismatch: The schema says an absent SQL limit adds one row, but execution returns at most the requested limit, so clients receive inconsistent limiting behavior.
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%2F44604&comment_hash=0717c219173504c7d2cb241bf2596218782a79f6900abb2948447e0aa84e16dc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44604&comment_hash=0717c219173504c7d2cb241bf2596218782a79f6900abb2948447e0aa84e16dc&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** Returning `None` for expression limits makes `Database.apply_limit_to_sql` treat them as unlimited and replace a potentially smaller existing limit.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=07017ff595e74a948770eddb89182833&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=07017ff595e74a948770eddb89182833&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/sql/parse.py
**Line:** 1805:1808
**Comment:**
*Api Mismatch: Returning `None` for expression limits makes `Database.apply_limit_to_sql` treat them as unlimited and replace a potentially smaller existing limit.
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%2F44604&comment_hash=990bd6b9970858a1a5ca40a2f2bfc97e7fac12209304309fdf276ca3b266dabe&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44604&comment_hash=990bd6b9970858a1a5ca40a2f2bfc97e7fac12209304309fdf276ca3b266dabe&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4138364288
##########
superset/sql/execution/executor.py:
##########
@@ -186,6 +186,60 @@ def build_statement_blocks(
return parsed_script, blocks
+class _LimitedCursor:
+ """Bound cumulative cursor reads without bypassing engine fetch processing."""
+
+ def __init__(self, cursor: Any, limit: int) -> None:
+ """Wrap a cursor with a shared budget for all row-reading methods."""
+ self._cursor = cursor
+ self._remaining = limit
+
+ def __getattr__(self, name: str) -> Any:
+ """Delegate metadata and driver-specific methods to the real cursor."""
+ return getattr(self._cursor, name)
+
+ @property
+ def arraysize(self) -> int:
+ """Expose the driver's default fetch batch size."""
+ return self._cursor.arraysize
+
+ @arraysize.setter
+ def arraysize(self, value: int) -> None:
+ """Preserve engine-specific cursor batch-size configuration."""
+ self._cursor.arraysize = value
+
+ def fetchmany(self, size: int | None = None) -> list[Any]:
+ """Read no more than the remaining budget, including across batches."""
+ size = self.arraysize if size is None else size
+ size = max(0, min(size, self._remaining))
+ # Some drivers interpret zero as unbounded, so do not call them at all.
+ if not size:
+ return []
+ # Reserve before reading so an engine's error fallback cannot retry
+ # beyond the budget after a driver has partially consumed a batch.
+ self._remaining -= size
+ return self._cursor.fetchmany(size)
Review Comment:
<!-- Bito Reply -->
The suggestion to decrement the budget after the driver returns is correct and should be applied. Decrementing the budget before the driver call can lead to silent data loss if a mid-fetch driver error occurs, as the budget is consumed for rows that were never actually returned. Applying this change ensures the budget accurately reflects only the rows successfully retrieved.
**superset/sql/execution/executor.py**
```
def fetchmany(self, size: int | None = None) -> list[Any]:
"""Read no more than the remaining budget, including across batches."""
size = self.arraysize if size is None else size
size = max(0, min(size, self._remaining))
if not size:
return []
rows = self._cursor.fetchmany(size)
self._remaining -= len(rows)
return rows
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4139575061
##########
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:
Follow-up fixed in 53718267d279b0d6c8c0e112d6aa988d26a6e2bc: BigQuery probes for one omitted row before marking a full memory-limited initial sample as truncated. An exactly 1,000-row result with a 1 MB cap now reports truncated=false. Six regression cases cover 999, 1,000 and 1,001 rows, with and without a request limit, including the bounded cursor probe. Before the fix: 4 failed, 2 passed; after: all 6 passed. The affected suites pass with 4,273 passed and 7 skipped; the SQL coverage gate passes with 2,208 tests and 100% coverage, and all 12 applicable pre-commit hooks pass. Apache master was merged normally in b6c7f041e8e582efeca3ec2376d9e2688d156f30, preserving both release notes.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5903898320
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44604:
URL: https://github.com/apache/superset/pull/44604#issuecomment-5811872945
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 33dee877e35924263a67fc56be742d40ecb63644 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ab4f2a4102ab50009aee939 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44604--superset-docs-preview.netlify.app](https://deploy-preview-44604--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8y6AT8V2Q06xmgTCVpmBkV099y9xXsh5T2mAiNAAzbT5tbq06FUcpAa-WZ3FcbzCBh89AWj32qdOYX9meaae7LtoqiVQpHthBCGmI028P0) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44604--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4139859172
##########
superset/db_engine_specs/bigquery.py:
##########
@@ -465,13 +465,17 @@ def fetch_data(cls, cursor: Any, limit: int | None = None) -> list[tuple[Any, ..
# First batch already covers the budget or the result set
if rows_fetched < initial_batch_size or remaining_rows <= 0:
+ # A full sample can also be the entire result. Confirm an omitted
+ # row; a bounded cursor leaves its own EOF probe to the executor.
memory_limited = (
- remaining_rows <= 0 and rows_fetched == initial_batch_size
+ remaining_rows <= 0
+ and rows_fetched == initial_batch_size
+ and bool(cursor.fetchmany(1))
Review Comment:
<!-- Bito Reply -->
The suggestion is valid and addresses a potential data loss issue. When the first batch of rows exactly matches the budget, the EOF probe `cursor.fetchmany(1)` consumes an additional row that is currently discarded because the function returns `first_batch` directly. Capturing this probe result and appending it to `first_batch` when non-empty ensures that the truncation status is correctly identified and no rows are silently dropped.
**superset/db_engine_specs/bigquery.py**
```
# First batch already covers the budget or the result set
if rows_fetched < initial_batch_size or remaining_rows <= 0:
# A full sample can also be the entire result. Confirm an omitted
# row; a bounded cursor leaves its own EOF probe to the executor.
memory_limited = (
remaining_rows <= 0
and rows_fetched == initial_batch_size
and bool(cursor.fetchmany(1))
)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4140859362
##########
tests/unit_tests/db_engine_specs/test_bigquery.py:
##########
@@ -832,6 +869,43 @@ def test_fetch_data_fallback_on_exception(mocker: MockerFixture) -> None:
assert flask_g.bq_memory_limited_row_count == 2
+@pytest.mark.parametrize("bounded", [False, True])
+@pytest.mark.parametrize("failure_stage", ["probe", "second_batch", "estimate"])
+def test_fetch_data_does_not_discard_sample_after_error(
+ mocker: MockerFixture, bounded: bool, failure_stage: str
+) -> None:
+ """A failure after consuming rows must not return only the cursor remainder."""
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
+ from superset.sql.execution.executor import _LimitedCursor
+
+ _, app = _patch_bq_fetch_deps(mocker)
+ mocker.patch("superset.db_engine_specs.bigquery.has_app_context", return_value=True)
+ app.config = {"BQ_FETCH_MAX_MB": 1}
+ mocker.patch("superset.db_engine_specs.bigquery._BQ_INITIAL_SAMPLE_ROWS", 2)
+ mocker.patch.object(
+ BigQueryEngineSpec,
+ "get_dbapi_exception_mapping",
+ return_value={OSError: SupersetDBAPIConnectionError},
+ )
+ row = ("x" * (600_000 if failure_stage == "probe" else 300_000),)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Undocumented magic row sizes</b></div>
<div id="fix">
The 600_000/300_000 literals encode the 1 MiB `BQ_FETCH_MAX_MB` budget math — two ~600 KB rows overflow it (EOF-probe path), ~300 KB rows keep `remaining_rows > 0` (second-batch path) — but are undocumented. Sibling `test_fetch_data_eof_probe_preserves_budgeted_rows` documents identical sizing at line 809; add the same brief comment.
</div>
</div>
<small><i>Code Review Run #59108b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/db_engine_specs/test_bigquery.py:
##########
@@ -832,6 +869,43 @@ def test_fetch_data_fallback_on_exception(mocker: MockerFixture) -> None:
assert flask_g.bq_memory_limited_row_count == 2
+@pytest.mark.parametrize("bounded", [False, True])
+@pytest.mark.parametrize("failure_stage", ["probe", "second_batch", "estimate"])
+def test_fetch_data_does_not_discard_sample_after_error(
+ mocker: MockerFixture, bounded: bool, failure_stage: str
+) -> None:
+ """A failure after consuming rows must not return only the cursor remainder."""
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
+ from superset.sql.execution.executor import _LimitedCursor
+
+ _, app = _patch_bq_fetch_deps(mocker)
+ mocker.patch("superset.db_engine_specs.bigquery.has_app_context", return_value=True)
+ app.config = {"BQ_FETCH_MAX_MB": 1}
+ mocker.patch("superset.db_engine_specs.bigquery._BQ_INITIAL_SAMPLE_ROWS", 2)
+ mocker.patch.object(
+ BigQueryEngineSpec,
+ "get_dbapi_exception_mapping",
+ return_value={OSError: SupersetDBAPIConnectionError},
+ )
+ row = ("x" * (600_000 if failure_stage == "probe" else 300_000),)
+ cursor = mock.MagicMock()
+ cursor.description = [("n", "STRING", None, None, None, None, None)]
+ cursor.fetchmany.side_effect = [[row, row], OSError("read failed"), [row]]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead side_effect entries</b></div>
<div id="fix">
In the `estimate` parametrization, the patched `sys.getsizeof` raises before the second `fetchmany`, so the `OSError` entry here is never consumed; the trailing `[row]` is consumed in no stage (the EOF probe raises at call 2). The shared `side_effect` list misstates each scenario's call sequence; derive it from `failure_stage`.
</div>
</div>
<small><i>Code Review Run #59108b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4138361829
##########
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:
Fixed in f06ce97e7b: StatementResult and StatementInfo expose fetch truncation using one probe row, distinguishing exact caps from omitted rows, including metadata and RETURNING results. The flag survives async execution and caching.
##########
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:
Documented in f06ce97e7b: SQL Lab intentionally treats PERCENT and WITH TIES as non-fixed counts and uses the dropdown cap, so TOP 5 PERCENT with a 1000-row dropdown can return up to 1000 rows rather than five. UPDATING.md also covers literal FETCH FIRST and parenthesized limits.
##########
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:
Fixed in f06ce97e7b: the limit description says “Maximum rows returned by the last statement” and explicitly describes adding a LIMIT clause when absent.
##########
superset/sql/execution/executor.py:
##########
@@ -186,6 +186,60 @@ def build_statement_blocks(
return parsed_script, blocks
+class _LimitedCursor:
+ """Bound cumulative cursor reads without bypassing engine fetch processing."""
+
+ def __init__(self, cursor: Any, limit: int) -> None:
+ """Wrap a cursor with a shared budget for all row-reading methods."""
+ self._cursor = cursor
+ self._remaining = limit
+
+ def __getattr__(self, name: str) -> Any:
+ """Delegate metadata and driver-specific methods to the real cursor."""
+ return getattr(self._cursor, name)
+
+ @property
+ def arraysize(self) -> int:
+ """Expose the driver's default fetch batch size."""
+ return self._cursor.arraysize
+
+ @arraysize.setter
+ def arraysize(self, value: int) -> None:
+ """Preserve engine-specific cursor batch-size configuration."""
+ self._cursor.arraysize = value
+
+ def fetchmany(self, size: int | None = None) -> list[Any]:
+ """Read no more than the remaining budget, including across batches."""
+ size = self.arraysize if size is None else size
+ size = max(0, min(size, self._remaining))
+ # Some drivers interpret zero as unbounded, so do not call them at all.
+ if not size:
+ return []
+ # Reserve before reading so an engine's error fallback cannot retry
+ # beyond the budget after a driver has partially consumed a batch.
+ self._remaining -= size
+ return self._cursor.fetchmany(size)
Review Comment:
Budget accounting was fixed in e781f0d7c7: only successful driver reads consume the budget, preserving it for retries and BigQuery fallback. f06ce97e7b retains that behavior and exposes fetch truncation, including for RETURNING, while documenting that returned-row caps do not limit affected rows.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): execute_sql request limit caps, never raises, an explicit SQL LIMIT [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4139728216
##########
superset/db_engine_specs/bigquery.py:
##########
@@ -465,13 +465,17 @@ def fetch_data(cls, cursor: Any, limit: int | None = None) -> list[tuple[Any, ..
# First batch already covers the budget or the result set
if rows_fetched < initial_batch_size or remaining_rows <= 0:
+ # A full sample can also be the entire result. Confirm an omitted
+ # row; a bounded cursor leaves its own EOF probe to the executor.
memory_limited = (
- remaining_rows <= 0 and rows_fetched == initial_batch_size
+ remaining_rows <= 0
+ and rows_fetched == initial_batch_size
+ and bool(cursor.fetchmany(1))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>EOF probe drops row</b></div>
<div id="fix">
The new EOF probe `cursor.fetchmany(1)` consumes a row from the cursor, but this branch returns only `first_batch` (line 478) — `FetchedRows` (superset_typing.py:165) is a plain list with no cursor back-reference, so the probed row is silently dropped whenever a full first sample exactly exhausts the budget. Capture the probe result and append the row to `first_batch` when non-empty before returning.
</div>
</div>
<small><i>Code Review Run #021104</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org