You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "atrsa (via GitHub)" <gi...@apache.org> on 2026/04/21 06:51:43 UTC
[PR] fix(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
atrsa opened a new pull request, #39509:
URL: https://github.com/apache/superset/pull/39509
### SUMMARY
Drill to Detail paginates sample rows with LIMIT / OFFSET, which is unsupported by the Elasticsearch and OpenDistro/OpenSearch SQL APIs. Page 1 loads, but any click beyond that throws parsing_exception: mismatched input 'OFFSET' and the drill-detail modal errors out.
This PR introduces a small engine-spec capability (allows_offset_fetch, default True) and an optional cursor-based pagination hook (fetch_data_with_cursor) on BaseEngineSpec. The changes are scoped:
- Query builder (superset/models/helpers.py): skip qry.offset(...) when the backing engine opts out. Page 1 behaviour is unchanged because row_offset == 0.
- Elasticsearch / OpenDistro specs (superset/db_engine_specs/elasticsearch.py): opt out of OFFSET, and implement fetch_data_with_cursor against the native ES SQL cursor (/_sql + /_sql/close, or /_opendistro/_sql + /_opendistro/_sql/close). The helper strips trailing ; and a trailing LIMIT N from the submitted SQL (both break ES cursor semantics) and sets Content-Type: application/json on the raw transport.
- Samples endpoint (superset/views/datasource/utils.py): for engines that can't do OFFSET, pages > 1 delegate to the engine-spec cursor helper. Colnames/coltypes are sourced from the normal samples payload so the frontend grid renders identically to page 1. Count-star cache is evicted on failure, matching the existing FAILED path.
- API schema (superset/databases/schemas.py): expose allows_offset_fetch on EngineInformationSchema for completeness.
No other engine specs are touched; no behaviour change for Postgres, MySQL,
BigQuery, etc.
**PERFORMANCE NOTE**
The ES SQL cursor API is forward-only, so for engines on the cursor path (Elasticsearch, OpenDistro/OpenSearch) reaching page N of drill-to-detail issues N round trips to the cluster. Deep pagination cost is therefore linear in page number, not constant like OFFSET-capable engines. In practice drill-to-detail is used to skim the first handful of pages, so this is a reasonable trade-off — but if a user paginates into the hundreds or thousands of pages, latency will grow proportionally. Documented in UPDATING.md and in the _fetch_page_via_cursor docstring so forks can reason about the cost.
Fixes #24563
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<img width="884" height="699" alt="Before" src="https://github.com/user-attachments/assets/204f6cd3-0287-4725-8c8c-a83043d607f5" />
<img width="850" height="669" alt="After" src="https://github.com/user-attachments/assets/ee1c0ff3-655d-4106-ab6f-d777467f18c1" />
### TESTING INSTRUCTIONS
1. Stand up Superset 5.0.0 with elasticsearch-dbapi installed and an ES 7.17
side-container.
2. Seed an ES index with >100 rows (e.g. drill_demo with 180 orders).
3. Register an elasticsearch+http://… database and a dataset on the seeded index.
4. POST /datasource/samples?datasource_type=table&datasource_id=<id>&force=true&pa
ge=N&per_page=50 for N = 1, 2, 3.
**Before this PR (stock 5.0.0):**
- Page 1 → 200 OK, 50 rows.
- Pages 2 and 3 → "error": "Error (parsing_exception): … mismatched input 'OFFSET'
expecting <EOF>".
**After this PR:**
- Page 1 → 50 rows (ids 1–50).
- Page 2 → 50 rows (ids 51–100).
- Page 3 → 50 rows (ids 101–150).
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: #24563
- [ ] 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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "atrsa (via GitHub)" <gi...@apache.org>.
atrsa commented on code in PR #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3347469520
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
Review Comment:
This is a unit-test module, not production code. `assert` is the standard pytest idiom and pytest never runs under `-O`, so the statement can't be optimized away here. Rewriting to `pytest.fail()` would be non-idiomatic. False positive — not changing.
##########
tests/unit_tests/db_engine_specs/test_elasticsearch.py:
##########
@@ -92,3 +93,325 @@ def test_opendistro_sqla_column_label(original: str, expected: str) -> None:
from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
assert OpenDistroEngineSpec.make_label_compatible(original) == expected
+
+
+def test_elasticsearch_spec_opts_out_of_offset_fetch() -> None:
+ """
+ Elasticsearch SQL does not support OFFSET. The spec must opt out so the
+ query builder does not emit OFFSET clauses that crash the parser.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ assert ElasticSearchEngineSpec.supports_offset is False
+
+
+def test_opendistro_spec_opts_out_of_offset_fetch() -> None:
+ """
+ OpenDistro/OpenSearch SQL also does not support OFFSET.
+ """
+ from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
+
+ assert OpenDistroEngineSpec.supports_offset is False
+
+
+def _build_fake_database(transport_responses: list[dict[str, Any]]) -> MagicMock:
+ """
+ Build a mocked Database whose get_raw_connection() yields a connection
+ whose es.transport.perform_request returns transport_responses sequentially.
+ """
+ database = MagicMock(name="Database")
+
+ responses_iter = iter(transport_responses)
+
+ def perform_request(method, path, body=None, **_kwargs):
+ return next(responses_iter)
+
+ transport = MagicMock()
+ transport.perform_request.side_effect = perform_request
+ conn = MagicMock()
+ conn.es.transport = transport
+
+ ctx = MagicMock()
+ ctx.__enter__ = MagicMock(return_value=conn)
+ ctx.__exit__ = MagicMock(return_value=False)
+ database.get_raw_connection.return_value = ctx
+ database._transport = transport # expose for assertions
+ return database
+
+
+def test_fetch_data_with_cursor_returns_first_page_when_page_index_zero() -> None:
+ """
+ Page index 0 = return the rows from the initial query, no cursor
+ iteration needed. The cursor must still be closed if present.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {
+ "columns": [{"name": "a"}, {"name": "b"}],
+ "rows": [[1, "x"], [2, "y"]],
+ "cursor": "CUR-1",
+ },
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a, b FROM idx",
+ page_index=0,
+ page_size=2,
+ )
+
+ assert rows == [[1, "x"], [2, "y"]]
+ assert cols == ["a", "b"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 2
+ assert calls[0][0][0] == "POST"
+ assert calls[0][0][1] == "/_sql"
+ assert calls[0].kwargs["body"] == {"query": "SELECT a, b FROM idx", "fetch_size": 2}
+ assert calls[1][0][1] == "/_sql/close"
+ assert calls[1].kwargs["body"] == {"cursor": "CUR-1"}
+
+
+def test_fetch_data_with_cursor_iterates_to_target_page() -> None:
+ """
+ For page_index=2, the code executes the initial query, then sends the
+ cursor twice. The rows returned belong to the third page.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]], "cursor": "C2"},
+ {"rows": [[2]], "cursor": "C3"},
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=2,
+ page_size=1,
+ )
+
+ assert rows == [[2]]
+ assert cols == ["a"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 4
+ assert calls[1].kwargs["body"] == {"cursor": "C1"}
+ assert calls[2].kwargs["body"] == {"cursor": "C2"}
+ assert calls[3][0][1] == "/_sql/close"
+ assert calls[3].kwargs["body"] == {"cursor": "C3"}
+
+
+def test_fetch_data_with_cursor_returns_empty_when_dataset_exhausted() -> None:
+ """
+ If the dataset has fewer pages than the requested page_index, the
+ cursor becomes falsy mid-iteration. Return empty rows, do not call
+ close, do not raise.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]]}, # no cursor → dataset ends here
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=5,
+ page_size=1,
+ )
+
+ assert rows == []
+ assert cols == ["a"]
+ assert len(database._transport.perform_request.call_args_list) == 2
+
+
+def test_fetch_data_with_cursor_does_not_close_when_no_cursor_present() -> None:
+ """
+ Some responses (tiny result sets) come back without a cursor token.
+ The code must not send a close request with a missing cursor.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0], [1]]},
+ ]
+ )
+
+ rows, _ = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=0,
+ page_size=50,
+ )
+
+ assert rows == [[0], [1]]
+ assert len(database._transport.perform_request.call_args_list) == 1
+
+
+def test_fetch_data_with_cursor_closes_cursor_even_if_iteration_raises() -> None:
+ """
+ If an intermediate cursor request raises, the cursor from the most
+ recent successful response must still be closed. Prevents server-side
+ cursor leaks on transport errors.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ class BoomError(RuntimeError):
+ pass
+
+ responses = [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ ]
+
+ call_count = {"n": 0}
+ recorded_close = {}
+
+ def perform_request(method, path, body=None, **_kwargs):
+ call_count["n"] += 1
+ # calls: 1=initial query, 2=cursor follow-up (raises), 3=close
+ if call_count["n"] == 2:
+ raise BoomError("transport blew up")
+ if path.endswith("/close"):
+ recorded_close["body"] = body
+ return {}
+ return responses[call_count["n"] - 1]
+
+ transport = MagicMock()
+ transport.perform_request.side_effect = perform_request
+ conn = MagicMock()
+ conn.es.transport = transport
+ ctx = MagicMock()
+ ctx.__enter__ = MagicMock(return_value=conn)
+ ctx.__exit__ = MagicMock(return_value=False)
+ database = MagicMock()
+ database.get_raw_connection.return_value = ctx
+
+ with pytest.raises(BoomError):
+ ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=3,
+ page_size=1,
+ )
+
+ assert recorded_close.get("body") == {"cursor": "C1"}, (
+ "The cursor from the last successful response must be closed "
+ "even when a later iteration raises."
+ )
+
+
+@pytest.mark.parametrize(
+ "sql_in,expected_query",
Review Comment:
`@pytest.mark.parametrize("sql_in,expected_query", ...)` with a comma-separated string is fully valid and the most common pytest idiom — pytest splits the names internally. A tuple of names is equivalent, not required. False positive — not changing.
##########
superset/db_engine_specs/elasticsearch.py:
##########
@@ -29,16 +32,108 @@
SupersetDBAPIProgrammingError,
)
+if TYPE_CHECKING:
+ from superset.models.core import Database
+
logger = logging.getLogger()
+def _fetch_page_via_cursor(
+ database: Database,
+ sql: str,
+ page_index: int,
+ page_size: int,
+ sql_path: str,
+ close_path: str,
+) -> tuple[list[list[Any]], list[str]]:
+ """
+ Iterate Elasticsearch/OpenSearch SQL cursor pagination to return a single
+ page of results.
+
+ Executes ``sql`` with ``fetch_size = page_size``, then sends cursor
+ follow-up requests ``page_index`` times to skip earlier pages. Closes the
+ cursor when done to release server-side state. Returns
+ ``(rows, columns)``.
+
+ If the dataset is exhausted before reaching ``page_index``, returns an
+ empty rows list with the column names from the initial request.
+
+ Note: the Elasticsearch SQL cursor is forward-only, so cost is linear in
+ ``page_index`` — reaching page N issues N round trips to the cluster.
+ Deep pagination (hundreds of pages) will therefore be noticeably slower
+ than on ``OFFSET``-capable engines. This is a protocol limitation, not
+ an implementation choice.
+ """
+ # The Elasticsearch SQL API rejects trailing semicolons, and any LIMIT
+ # in the submitted statement caps the result set before the cursor can
+ # page through it. ``fetch_size`` drives pagination instead.
+ # Assumption: Superset only appends a trailing ``LIMIT N`` for engines
+ # with ``supports_offset=False``. If that ever changes (e.g.
+ # ``FETCH FIRST N ROWS`` or ``TOP N``), extend this sanitizer to match.
+ sanitized_sql = sql.strip().rstrip(";").strip()
+ sanitized_sql = re.sub(
+ r"\s+LIMIT\s+\d+\s*$", "", sanitized_sql, flags=re.IGNORECASE
+ )
+
+ # The raw transport does not auto-set Content-Type the way the Python
+ # DB-API driver does; ES rejects POSTs without a JSON content type.
+ json_headers = {"Content-Type": "application/json"}
+ with database.get_raw_connection() as conn:
+ transport = conn.es.transport
+ response = transport.perform_request(
+ "POST",
+ sql_path,
+ headers=json_headers,
+ body={"query": sanitized_sql, "fetch_size": page_size},
+ )
+ columns = [col["name"] for col in response.get("columns", [])]
+ rows = response.get("rows", [])
+ cursor = response.get("cursor")
+
+ try:
+ for _ in range(page_index):
+ if not cursor:
+ # Dataset exhausted before reaching the target page —
+ # no cursor to close (ES returns no cursor on the final
+ # page). Return immediately with empty rows.
+ return [], columns
+ response = transport.perform_request(
+ "POST",
+ sql_path,
+ headers=json_headers,
+ body={"cursor": cursor},
+ )
+ rows = response.get("rows", [])
+ cursor = response.get("cursor")
+
+ return rows, columns
+ finally:
+ if cursor:
+ # Best-effort cleanup. If close itself fails we don't want
+ # to mask the original error (if any) — swallow and log.
+ try:
+ transport.perform_request(
+ "POST",
+ close_path,
+ headers=json_headers,
+ body={"cursor": cursor},
+ )
+ except Exception: # pylint: disable=broad-except
Review Comment:
This is a best-effort cursor close in a `finally` block; a failure here must not mask the original error, so the broad catch is intentional and already annotated with `# pylint: disable=broad-except` plus an explanatory comment. The suggested `except (ConnectionError, TimeoutError, Exception)` is redundant since `Exception` already subsumes the other two. False positive — keeping as-is.
--
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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "atrsa (via GitHub)" <gi...@apache.org>.
atrsa commented on code in PR #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3348000541
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
+ "helpers.py no longer references supports_offset; the OFFSET "
+ "guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
+ )
+
+ tree = ast.parse(source)
+ unguarded: list[int] = []
+
+ class Visitor(ast.NodeVisitor):
+ def __init__(self) -> None:
+ self._in_guarded_if = 0
+
+ def visit_If(self, node: ast.If) -> None: # noqa: N802
+ if _uses_supports_offset(node.test):
Review Comment:
Added a docstring to the `visit_If` method.
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
+ "helpers.py no longer references supports_offset; the OFFSET "
+ "guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
+ )
+
+ tree = ast.parse(source)
+ unguarded: list[int] = []
+
+ class Visitor(ast.NodeVisitor):
+ def __init__(self) -> None:
+ self._in_guarded_if = 0
+
+ def visit_If(self, node: ast.If) -> None: # noqa: N802
+ if _uses_supports_offset(node.test):
+ self._in_guarded_if += 1
+ for child in node.body:
+ self.visit(child)
+ self._in_guarded_if -= 1
+ for child in node.orelse:
+ self.visit(child)
+ else:
+ self.generic_visit(node)
+
+ def visit_Assign(self, node: ast.Assign) -> None: # noqa: N802
+ if _is_qry_offset_assignment(node) and self._in_guarded_if == 0:
Review Comment:
Added a docstring to the `visit_Assign` method.
--
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(drill-detail): paginate Elasticsearch samples via engine cursor [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 #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3559520352
##########
superset/db_engine_specs/elasticsearch.py:
##########
@@ -136,6 +231,30 @@ class ElasticSearchEngineSpec(BaseEngineSpec): # pylint: disable=abstract-metho
type_code_map: dict[int, str] = {} # loaded from get_datatype only if needed
+ SQL_ENDPOINT = "/_sql"
Review Comment:
**Suggestion:** Add explicit type annotations for the newly introduced class-level endpoint constants to satisfy the type-hint requirement for relevant variables. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The custom rule requires type hints for relevant variables that can be annotated. These new class-level constants are plain string assignments without annotations, so the suggestion correctly identifies a real type-hint omission.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1e21e78d44a44c01826a4046030ad26b&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=1e21e78d44a44c01826a4046030ad26b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/elasticsearch.py
**Line:** 233:234
**Comment:**
*Custom Rule: Add explicit type annotations for the newly introduced class-level endpoint constants to satisfy the type-hint requirement for relevant variables.
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%2F39509&comment_hash=effd365546992f8532d9a128586d459a537549fe9f151f8ae5c8d218c30c735e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=effd365546992f8532d9a128586d459a537549fe9f151f8ae5c8d218c30c735e&reaction=dislike'>👎</a>
##########
superset/db_engine_specs/elasticsearch.py:
##########
@@ -204,6 +324,30 @@ class OpenDistroEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
engine = "odelasticsearch"
engine_name = "OpenSearch (OpenDistro)"
+ SQL_ENDPOINT = "/_opendistro/_sql"
+ SQL_CLOSE_ENDPOINT = "/_opendistro/_sql/close"
Review Comment:
**Suggestion:** Add explicit type annotations for the newly introduced OpenDistro endpoint constants so these relevant class variables are type-hinted. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The custom rule requires type hints for relevant variables that can be annotated. These newly introduced class constants are unannotated string variables, so this is a genuine violation.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ba73d3668b3d4d3e909ca88dc24908f0&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=ba73d3668b3d4d3e909ca88dc24908f0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/elasticsearch.py
**Line:** 327:328
**Comment:**
*Custom Rule: Add explicit type annotations for the newly introduced OpenDistro endpoint constants so these relevant class variables are type-hinted.
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%2F39509&comment_hash=68ab7b4c410e0046f99e1aa70fb98c1dc3466267364d06ea099b1f211187ea81&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=68ab7b4c410e0046f99e1aa70fb98c1dc3466267364d06ea099b1f211187ea81&reaction=dislike'>👎</a>
##########
superset/views/datasource/utils.py:
##########
@@ -176,15 +178,88 @@ def get_samples( # pylint: disable=too-many-arguments
if count_star_data.get("status") == QueryStatus.FAILED:
raise DatasetSamplesFailedError(count_star_data.get("error"))
- sample_data = samples_instance.get_payload()["queries"][0]
+ engine_spec = datasource.database.db_engine_spec
Review Comment:
**Suggestion:** Add an explicit local type annotation for this newly introduced variable to satisfy the project’s type-hinting requirement for relevant variables. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is newly added Python code and the local variable `engine_spec` is introduced without an explicit type annotation, which matches the project rule requiring type hints for relevant new or modified Python variables when they can be annotated.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=009b1ee022434d23a3c7f77bccb537aa&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=009b1ee022434d23a3c7f77bccb537aa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/datasource/utils.py
**Line:** 181:181
**Comment:**
*Custom Rule: Add an explicit local type annotation for this newly introduced variable to satisfy the project’s type-hinting requirement for relevant variables.
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%2F39509&comment_hash=3bdbaaf17cdc3364fc48545128879ea5f49fac092e40b869b55d92b81ba3f1ed&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=3bdbaaf17cdc3364fc48545128879ea5f49fac092e40b869b55d92b81ba3f1ed&reaction=dislike'>👎</a>
##########
superset/views/datasource/utils.py:
##########
@@ -176,15 +178,88 @@ def get_samples( # pylint: disable=too-many-arguments
if count_star_data.get("status") == QueryStatus.FAILED:
raise DatasetSamplesFailedError(count_star_data.get("error"))
- sample_data = samples_instance.get_payload()["queries"][0]
+ engine_spec = datasource.database.db_engine_spec
+ row_offset = limit_clause["row_offset"]
+ row_limit = limit_clause["row_limit"]
- if sample_data.get("status") == QueryStatus.FAILED:
- QueryCacheManager.delete(count_star_data.get("cache_key"), CacheRegion.DATA)
- raise DatasetSamplesFailedError(sample_data.get("error"))
+ if not engine_spec.supports_offset and row_offset > 0:
+ try:
+ sample_data = _fetch_samples_via_cursor(
+ datasource=datasource,
+ samples_instance=samples_instance,
+ count_star_data=count_star_data,
+ page_index=row_offset // row_limit,
+ page_size=row_limit,
+ )
+ except DatasetSamplesFailedError:
+ raise
+ except Exception as exc:
+ QueryCacheManager.delete(
+ count_star_data.get("cache_key"), CacheRegion.DATA
+ )
+ logger.exception("Cursor-based samples pagination failed")
+ raise DatasetSamplesFailedError(
+ "Failed to fetch samples via cursor pagination"
+ ) from exc
+ else:
+ sample_data = samples_instance.get_payload()["queries"][0]
+ if sample_data.get("status") == QueryStatus.FAILED:
+ QueryCacheManager.delete(
+ count_star_data.get("cache_key"), CacheRegion.DATA
+ )
+ raise DatasetSamplesFailedError(sample_data.get("error") or "")
sample_data["page"] = page
sample_data["per_page"] = per_page
sample_data["total_count"] = count_star_data["data"][0]["COUNT(*)"]
return sample_data
except (IndexError, KeyError) as exc:
raise DatasetSamplesFailedError from exc
+
+
+def _fetch_samples_via_cursor(
+ datasource: Datasource,
+ samples_instance: QueryContext,
+ count_star_data: dict[str, Any],
+ page_index: int,
+ page_size: int,
+) -> dict[str, Any]:
+ """
+ Fetch a single page of samples via engine-spec cursor pagination.
+
+ Used when ``datasource.database.db_engine_spec.supports_offset`` is
+ False and a non-first page is requested. Compiles the same SQL Superset
+ would run for the normal samples payload — without executing it — and
+ delegates cursor iteration to the engine spec. The engine spec is
+ responsible for stripping any trailing ``LIMIT`` from the SQL so the
+ cursor is not capped to a single page.
+
+ ``coltypes`` are inferred from the returned rows with
+ ``extract_dataframe_dtypes``, the same function the non-cursor path
+ uses to type page 1 — it works off the actual returned values, not
+ ``cursor.description``, so no ES-type-to-coltype translator is needed
+ and no extra query is required to source them.
+ """
+ query_obj = samples_instance.queries[0]
Review Comment:
**Suggestion:** Add an explicit local type annotation for this newly introduced variable in the helper function to comply with required type hints on relevant variables. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is newly added Python code and the local variable `query_obj` is introduced without an explicit type annotation, which is a real instance of the repository’s type-hinting rule for new or modified annotatable variables.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0e34d38f274145ada6ce2945344f45ce&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=0e34d38f274145ada6ce2945344f45ce&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/datasource/utils.py
**Line:** 243:243
**Comment:**
*Custom Rule: Add an explicit local type annotation for this newly introduced variable in the helper function to comply with required type hints on relevant variables.
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%2F39509&comment_hash=4236d67d3cb688cd6eada838ebeac8f6a0b8f16090698ae922447805d30255ba&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=4236d67d3cb688cd6eada838ebeac8f6a0b8f16090698ae922447805d30255ba&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(drill-detail): paginate Elasticsearch samples via engine cursor [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 #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3559521599
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,93 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
Review Comment:
**Suggestion:** Add an explicit type annotation for the module-level path constant so this new variable complies with the project’s type-hint requirement. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The new module-level constant `HELPERS_PATH` is assigned without any type annotation, and it is a clearly annotatable Python variable. This matches the rule requiring type hints on new or modified Python code for relevant variables.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf88fca6f43242a6b7d87cee8dc77fd7&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=bf88fca6f43242a6b7d87cee8dc77fd7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/models/test_helpers_offset.py
**Line:** 20:22
**Comment:**
*Custom Rule: Add an explicit type annotation for the module-level path constant so this new variable complies with the project’s type-hint requirement.
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%2F39509&comment_hash=f22e33ec29431b8a873c0f08e5988c2f15d27294f081f5dbea11f5fc4b84d815&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=f22e33ec29431b8a873c0f08e5988c2f15d27294f081f5dbea11f5fc4b84d815&reaction=dislike'>👎</a>
##########
tests/unit_tests/views/datasource/test_utils.py:
##########
@@ -0,0 +1,389 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+
+@pytest.fixture
+def fake_datasource_factory():
+ """Builds a MagicMock datasource whose db_engine_spec is configurable."""
+
+ def _build(supports_offset: bool) -> MagicMock:
+ datasource = MagicMock(name="SqlaTable")
+ datasource.type = "table"
+ datasource.id = 1
+ datasource.columns = []
+ datasource.database.db_engine_spec.supports_offset = supports_offset
+ return datasource
+
+ return _build
+
+
+def test_get_samples_uses_normal_path_when_engine_supports_offset(
+ fake_datasource_factory,
+):
Review Comment:
**Suggestion:** Add type hints for the fixture parameter and an explicit return type for this test function. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This test function omits both a type annotation for the fixture parameter and a return type annotation, which matches the stated Python type-hint rule violation.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0ef35284c0bd414ba824410358ea944e&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=0ef35284c0bd414ba824410358ea944e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/datasource/test_utils.py
**Line:** 37:39
**Comment:**
*Custom Rule: Add type hints for the fixture parameter and an explicit return type for this test function.
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%2F39509&comment_hash=37131e9fd214cf940601bea6784f15c2caed6e373435ea31a9adad1bed51b92d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=37131e9fd214cf940601bea6784f15c2caed6e373435ea31a9adad1bed51b92d&reaction=dislike'>👎</a>
##########
tests/unit_tests/views/datasource/test_utils.py:
##########
@@ -0,0 +1,389 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+
+@pytest.fixture
+def fake_datasource_factory():
Review Comment:
**Suggestion:** Add a return type annotation to this fixture function to satisfy the required type-hint rule. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The fixture function is newly added and has no return type annotation, which violates the type-hint rule for Python functions that can be annotated.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cbbc5cfb078b4f048e2c1e7ad1099529&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=cbbc5cfb078b4f048e2c1e7ad1099529&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/datasource/test_utils.py
**Line:** 23:23
**Comment:**
*Custom Rule: Add a return type annotation to this fixture function to satisfy the required type-hint rule.
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%2F39509&comment_hash=61889d7fffe0a935209a702886a35ef86ac5f9958e78e0063a3a3e02e0aa3815&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=61889d7fffe0a935209a702886a35ef86ac5f9958e78e0063a3a3e02e0aa3815&reaction=dislike'>👎</a>
##########
tests/unit_tests/views/datasource/test_utils.py:
##########
@@ -0,0 +1,389 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+
+@pytest.fixture
+def fake_datasource_factory():
+ """Builds a MagicMock datasource whose db_engine_spec is configurable."""
+
+ def _build(supports_offset: bool) -> MagicMock:
+ datasource = MagicMock(name="SqlaTable")
+ datasource.type = "table"
+ datasource.id = 1
+ datasource.columns = []
+ datasource.database.db_engine_spec.supports_offset = supports_offset
+ return datasource
+
+ return _build
+
+
+def test_get_samples_uses_normal_path_when_engine_supports_offset(
+ fake_datasource_factory,
+):
+ """
+ Engines with supports_offset=True continue to use the existing
+ QueryContext/get_payload path. No cursor-method calls.
+ """
+ from superset.views.datasource import utils
+
+ datasource = fake_datasource_factory(supports_offset=True)
+ datasource.database.db_engine_spec.fetch_data_with_cursor = MagicMock()
+
+ with (
+ patch.object(
+ utils, "DatasourceDAO", MagicMock(get_datasource=lambda **kw: datasource)
+ ),
+ patch.object(utils, "QueryContextFactory") as qcf,
+ ):
+ samples_ctx = MagicMock()
+ samples_ctx.get_payload.return_value = {
+ "queries": [
+ {
+ "data": [{"a": 1}],
+ "colnames": ["a"],
+ "coltypes": [],
+ "status": "success",
+ }
+ ]
+ }
+ count_ctx = MagicMock()
+ count_ctx.get_payload.return_value = {
+ "queries": [{"data": [{"COUNT(*)": 42}], "status": "success"}]
+ }
+ qcf.return_value.create.side_effect = [samples_ctx, count_ctx]
+
+ result = utils.get_samples(
+ datasource_type="table",
+ datasource_id=1,
+ page=2,
+ per_page=50,
+ )
+
+ assert result["data"] == [{"a": 1}]
+ assert result["page"] == 2
+ assert result["per_page"] == 50
+ assert result["total_count"] == 42
+ datasource.database.db_engine_spec.fetch_data_with_cursor.assert_not_called()
+
+
+def test_get_samples_normal_path_cleans_count_cache_when_sample_data_failed(
+ fake_datasource_factory,
+):
Review Comment:
**Suggestion:** Annotate the injected fixture argument and add an explicit return type annotation to this function. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The function signature lacks annotations for the injected fixture argument and the return type, so it is a real omission under the required type-hint rule.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ceb841e44a2142c388fbd7cf7839f27b&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=ceb841e44a2142c388fbd7cf7839f27b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/datasource/test_utils.py
**Line:** 86:88
**Comment:**
*Custom Rule: Annotate the injected fixture argument and add an explicit return type annotation to this function.
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%2F39509&comment_hash=7b243394b430d7a32229da87045a85b3ba9278a0b3ef732fdf337ac96e020a2b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=7b243394b430d7a32229da87045a85b3ba9278a0b3ef732fdf337ac96e020a2b&reaction=dislike'>👎</a>
##########
tests/unit_tests/views/datasource/test_utils.py:
##########
@@ -0,0 +1,389 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+
+@pytest.fixture
+def fake_datasource_factory():
+ """Builds a MagicMock datasource whose db_engine_spec is configurable."""
+
+ def _build(supports_offset: bool) -> MagicMock:
+ datasource = MagicMock(name="SqlaTable")
+ datasource.type = "table"
+ datasource.id = 1
+ datasource.columns = []
+ datasource.database.db_engine_spec.supports_offset = supports_offset
+ return datasource
+
+ return _build
+
+
+def test_get_samples_uses_normal_path_when_engine_supports_offset(
+ fake_datasource_factory,
+):
+ """
+ Engines with supports_offset=True continue to use the existing
+ QueryContext/get_payload path. No cursor-method calls.
+ """
+ from superset.views.datasource import utils
+
+ datasource = fake_datasource_factory(supports_offset=True)
+ datasource.database.db_engine_spec.fetch_data_with_cursor = MagicMock()
+
+ with (
+ patch.object(
+ utils, "DatasourceDAO", MagicMock(get_datasource=lambda **kw: datasource)
+ ),
+ patch.object(utils, "QueryContextFactory") as qcf,
+ ):
+ samples_ctx = MagicMock()
+ samples_ctx.get_payload.return_value = {
+ "queries": [
+ {
+ "data": [{"a": 1}],
+ "colnames": ["a"],
+ "coltypes": [],
+ "status": "success",
+ }
+ ]
+ }
+ count_ctx = MagicMock()
+ count_ctx.get_payload.return_value = {
+ "queries": [{"data": [{"COUNT(*)": 42}], "status": "success"}]
+ }
+ qcf.return_value.create.side_effect = [samples_ctx, count_ctx]
+
+ result = utils.get_samples(
+ datasource_type="table",
+ datasource_id=1,
+ page=2,
+ per_page=50,
+ )
+
+ assert result["data"] == [{"a": 1}]
+ assert result["page"] == 2
+ assert result["per_page"] == 50
+ assert result["total_count"] == 42
+ datasource.database.db_engine_spec.fetch_data_with_cursor.assert_not_called()
+
+
+def test_get_samples_normal_path_cleans_count_cache_when_sample_data_failed(
+ fake_datasource_factory,
+):
+ """
+ On the normal (non-cursor) path, a FAILED samples payload must evict the
+ count-star cache and raise DatasetSamplesFailedError — mirroring the
+ cursor path's failure handling.
+ """
+ from superset.commands.dataset.exceptions import DatasetSamplesFailedError
+ from superset.constants import CacheRegion
+ from superset.views.datasource import utils
+
+ datasource = fake_datasource_factory(supports_offset=True)
+
+ samples_ctx = MagicMock()
+ samples_ctx.get_payload.return_value = {
+ "queries": [
+ {
+ "status": "failed",
+ "error": "backend query failed",
+ }
+ ]
+ }
+ count_ctx = MagicMock()
+ count_ctx.get_payload.return_value = {
+ "queries": [
+ {
+ "data": [{"COUNT(*)": 1}],
+ "status": "success",
+ "cache_key": "count-cache-key",
+ }
+ ]
+ }
+
+ with (
+ patch.object(
+ utils, "DatasourceDAO", MagicMock(get_datasource=lambda **kw: datasource)
+ ),
+ patch.object(utils, "QueryContextFactory") as qcf,
+ patch.object(utils, "QueryCacheManager") as cache_mgr,
+ ):
+ qcf.return_value.create.side_effect = [samples_ctx, count_ctx]
+
+ with pytest.raises(DatasetSamplesFailedError) as excinfo:
+ utils.get_samples(
+ datasource_type="table",
+ datasource_id=1,
+ page=1,
+ per_page=50,
+ )
+
+ cache_mgr.delete.assert_called_once_with("count-cache-key", CacheRegion.DATA)
+ assert "backend query failed" in str(excinfo.value)
+
+
+def test_get_samples_uses_cursor_path_when_engine_disallows_offset(
+ fake_datasource_factory,
+):
+ """
+ When the engine reports supports_offset=False and the requested
+ page is > 1, get_samples delegates to fetch_data_with_cursor with SQL
+ compiled statically (without executing the normal samples payload).
+ """
+ from superset.views.datasource import utils
+
+ datasource = fake_datasource_factory(supports_offset=False)
+ datasource.database.db_engine_spec.fetch_data_with_cursor.return_value = (
+ [[99]],
+ ["a"],
+ )
+
+ samples_ctx = MagicMock()
+ samples_ctx.queries = [MagicMock()]
+ samples_ctx.datasource.get_query_str.return_value = "SELECT a FROM idx LIMIT 50"
+ count_ctx = MagicMock()
+ count_ctx.get_payload.return_value = {
+ "queries": [{"data": [{"COUNT(*)": 200}], "status": "success"}]
+ }
+
+ with (
+ patch.object(
+ utils, "DatasourceDAO", MagicMock(get_datasource=lambda **kw: datasource)
+ ),
+ patch.object(utils, "QueryContextFactory") as qcf,
+ ):
+ qcf.return_value.create.side_effect = [samples_ctx, count_ctx]
+
+ result = utils.get_samples(
+ datasource_type="table",
+ datasource_id=1,
+ page=3,
+ per_page=50,
+ )
+
+ datasource.database.db_engine_spec.fetch_data_with_cursor.assert_called_once()
+ kwargs = datasource.database.db_engine_spec.fetch_data_with_cursor.call_args.kwargs
+ assert kwargs["page_index"] == 2
+ assert kwargs["page_size"] == 50
+ # The cursor path compiles SQL statically via get_query_str, without
+ # executing the normal samples payload; the engine spec is responsible
+ # for any sanitation (strip ``;``/``LIMIT``).
+ assert kwargs["sql"] == "SELECT a FROM idx LIMIT 50"
+ samples_ctx.get_payload.assert_not_called()
+
+ assert result["data"] == [{"a": 99}]
+ assert result["colnames"] == ["a"]
+ assert result["page"] == 3
+ assert result["per_page"] == 50
+ assert result["total_count"] == 200
+
+
+def test_get_samples_cursor_path_infers_coltypes_from_cursor_rows(
+ fake_datasource_factory,
+):
+ """
+ coltypes on the cursor path are inferred from the returned rows via
+ extract_dataframe_dtypes — the same function the normal (non-cursor)
+ path uses to type page 1 — so SamplesPane's useGridColumns() picks up
+ type-based cell renderers on page 2+ without an extra query.
+ """
+ from superset.utils.core import GenericDataType
+ from superset.views.datasource import utils
+
+ datasource = fake_datasource_factory(supports_offset=False)
+ datasource.database.db_engine_spec.fetch_data_with_cursor.return_value = (
+ [["x", 1]],
+ ["a", "b"],
+ )
+
+ samples_ctx = MagicMock()
+ samples_ctx.queries = [MagicMock()]
+ samples_ctx.datasource.get_query_str.return_value = "SELECT a, b FROM idx LIMIT 50"
+ count_ctx = MagicMock()
+ count_ctx.get_payload.return_value = {
+ "queries": [{"data": [{"COUNT(*)": 2000}], "status": "success"}]
+ }
+
+ with (
+ patch.object(
+ utils, "DatasourceDAO", MagicMock(get_datasource=lambda **kw: datasource)
+ ),
+ patch.object(utils, "QueryContextFactory") as qcf,
+ ):
+ qcf.return_value.create.side_effect = [samples_ctx, count_ctx]
+
+ result = utils.get_samples(
+ datasource_type="table",
+ datasource_id=1,
+ page=2,
+ per_page=50,
+ )
+
+ assert result["coltypes"] == [GenericDataType.STRING, GenericDataType.NUMERIC]
+ assert result["colnames"] == ["a", "b"]
+ assert result["data"] == [{"a": "x", "b": 1}]
+
+
+def test_get_samples_cursor_path_cleans_count_cache_on_failure(
+ fake_datasource_factory,
+):
+ """
+ Issue 2: if fetch_data_with_cursor raises, the count-star cache must be
+ evicted (mirroring the normal FAILED path) and the error re-raised as
+ DatasetSamplesFailedError.
+ """
+ from superset.commands.dataset.exceptions import DatasetSamplesFailedError
+ from superset.constants import CacheRegion
+ from superset.views.datasource import utils
+
+ datasource = fake_datasource_factory(supports_offset=False)
+ datasource.database.db_engine_spec.fetch_data_with_cursor.side_effect = (
+ RuntimeError("boom: internal es stack trace details")
+ )
+
+ samples_ctx = MagicMock()
+ samples_ctx.queries = [MagicMock()]
+ samples_ctx.datasource.get_query_str.return_value = "SELECT a FROM idx LIMIT 50"
+ count_ctx = MagicMock()
+ count_ctx.get_payload.return_value = {
+ "queries": [
+ {
+ "data": [{"COUNT(*)": 200}],
+ "status": "success",
+ "cache_key": "count-cache-key",
+ }
+ ]
+ }
+
+ with (
+ patch.object(
+ utils, "DatasourceDAO", MagicMock(get_datasource=lambda **kw: datasource)
+ ),
+ patch.object(utils, "QueryContextFactory") as qcf,
+ patch.object(utils, "QueryCacheManager") as cache_mgr,
+ ):
+ qcf.return_value.create.side_effect = [samples_ctx, count_ctx]
+
+ with pytest.raises(DatasetSamplesFailedError) as excinfo:
+ utils.get_samples(
+ datasource_type="table",
+ datasource_id=1,
+ page=3,
+ per_page=50,
+ )
+
+ cache_mgr.delete.assert_called_once_with("count-cache-key", CacheRegion.DATA)
+ # Backend-internal error text must not leak into the user-facing message;
+ # only the original exception (chained via ``from exc``) retains the detail.
+ assert "internal es stack trace details" not in str(excinfo.value)
+ assert isinstance(excinfo.value.__cause__, RuntimeError)
+
+
+def test_get_samples_cursor_path_raises_when_compiled_sql_is_empty(
+ fake_datasource_factory,
+):
+ """
+ If get_query_str compiles to an empty string, the cursor path has
+ nothing to submit. Fail fast with a descriptive error and evict the
+ count cache, instead of handing an empty statement to the engine driver.
+ """
+ from superset.commands.dataset.exceptions import DatasetSamplesFailedError
+ from superset.constants import CacheRegion
+ from superset.views.datasource import utils
+
+ datasource = fake_datasource_factory(supports_offset=False)
+
+ samples_ctx = MagicMock()
+ samples_ctx.queries = [MagicMock()]
+ samples_ctx.datasource.get_query_str.return_value = ""
+ count_ctx = MagicMock()
+ count_ctx.get_payload.return_value = {
+ "queries": [
+ {
+ "data": [{"COUNT(*)": 200}],
+ "status": "success",
+ "cache_key": "count-cache-key",
+ }
+ ]
+ }
+
+ with (
+ patch.object(
+ utils, "DatasourceDAO", MagicMock(get_datasource=lambda **kw: datasource)
+ ),
+ patch.object(utils, "QueryContextFactory") as qcf,
+ patch.object(utils, "QueryCacheManager") as cache_mgr,
+ ):
+ qcf.return_value.create.side_effect = [samples_ctx, count_ctx]
+
+ with pytest.raises(DatasetSamplesFailedError):
+ utils.get_samples(
+ datasource_type="table",
+ datasource_id=1,
+ page=2,
+ per_page=50,
+ )
+
+ cache_mgr.delete.assert_called_once_with("count-cache-key", CacheRegion.DATA)
+ datasource.database.db_engine_spec.fetch_data_with_cursor.assert_not_called()
+
+
+def test_get_samples_cursor_path_unused_for_page_one(fake_datasource_factory):
Review Comment:
**Suggestion:** Provide a type hint for the fixture argument and annotate the function as returning no value. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The test function has an unannotated fixture argument and no explicit return type, which is a direct match for the type-hint requirement.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1142097dfde64434ade2bbe9f55eabfb&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=1142097dfde64434ade2bbe9f55eabfb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/datasource/test_utils.py
**Line:** 347:347
**Comment:**
*Custom Rule: Provide a type hint for the fixture argument and annotate the function as returning no value.
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%2F39509&comment_hash=378285f31d2e8bc775c4a55f48950a8c8b41be28159f23269d37811b24fe6846&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=378285f31d2e8bc775c4a55f48950a8c8b41be28159f23269d37811b24fe6846&reaction=dislike'>👎</a>
##########
tests/unit_tests/views/datasource/test_utils.py:
##########
@@ -0,0 +1,389 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+
+@pytest.fixture
+def fake_datasource_factory():
+ """Builds a MagicMock datasource whose db_engine_spec is configurable."""
+
+ def _build(supports_offset: bool) -> MagicMock:
+ datasource = MagicMock(name="SqlaTable")
+ datasource.type = "table"
+ datasource.id = 1
+ datasource.columns = []
+ datasource.database.db_engine_spec.supports_offset = supports_offset
+ return datasource
+
+ return _build
+
+
+def test_get_samples_uses_normal_path_when_engine_supports_offset(
+ fake_datasource_factory,
+):
+ """
+ Engines with supports_offset=True continue to use the existing
+ QueryContext/get_payload path. No cursor-method calls.
+ """
+ from superset.views.datasource import utils
+
+ datasource = fake_datasource_factory(supports_offset=True)
+ datasource.database.db_engine_spec.fetch_data_with_cursor = MagicMock()
+
+ with (
+ patch.object(
+ utils, "DatasourceDAO", MagicMock(get_datasource=lambda **kw: datasource)
+ ),
+ patch.object(utils, "QueryContextFactory") as qcf,
+ ):
+ samples_ctx = MagicMock()
+ samples_ctx.get_payload.return_value = {
+ "queries": [
+ {
+ "data": [{"a": 1}],
+ "colnames": ["a"],
+ "coltypes": [],
+ "status": "success",
+ }
+ ]
+ }
+ count_ctx = MagicMock()
+ count_ctx.get_payload.return_value = {
+ "queries": [{"data": [{"COUNT(*)": 42}], "status": "success"}]
+ }
+ qcf.return_value.create.side_effect = [samples_ctx, count_ctx]
+
+ result = utils.get_samples(
+ datasource_type="table",
+ datasource_id=1,
+ page=2,
+ per_page=50,
+ )
+
+ assert result["data"] == [{"a": 1}]
+ assert result["page"] == 2
+ assert result["per_page"] == 50
+ assert result["total_count"] == 42
+ datasource.database.db_engine_spec.fetch_data_with_cursor.assert_not_called()
+
+
+def test_get_samples_normal_path_cleans_count_cache_when_sample_data_failed(
+ fake_datasource_factory,
+):
+ """
+ On the normal (non-cursor) path, a FAILED samples payload must evict the
+ count-star cache and raise DatasetSamplesFailedError — mirroring the
+ cursor path's failure handling.
+ """
+ from superset.commands.dataset.exceptions import DatasetSamplesFailedError
+ from superset.constants import CacheRegion
+ from superset.views.datasource import utils
+
+ datasource = fake_datasource_factory(supports_offset=True)
+
+ samples_ctx = MagicMock()
+ samples_ctx.get_payload.return_value = {
+ "queries": [
+ {
+ "status": "failed",
+ "error": "backend query failed",
+ }
+ ]
+ }
+ count_ctx = MagicMock()
+ count_ctx.get_payload.return_value = {
+ "queries": [
+ {
+ "data": [{"COUNT(*)": 1}],
+ "status": "success",
+ "cache_key": "count-cache-key",
+ }
+ ]
+ }
+
+ with (
+ patch.object(
+ utils, "DatasourceDAO", MagicMock(get_datasource=lambda **kw: datasource)
+ ),
+ patch.object(utils, "QueryContextFactory") as qcf,
+ patch.object(utils, "QueryCacheManager") as cache_mgr,
+ ):
+ qcf.return_value.create.side_effect = [samples_ctx, count_ctx]
+
+ with pytest.raises(DatasetSamplesFailedError) as excinfo:
+ utils.get_samples(
+ datasource_type="table",
+ datasource_id=1,
+ page=1,
+ per_page=50,
+ )
+
+ cache_mgr.delete.assert_called_once_with("count-cache-key", CacheRegion.DATA)
+ assert "backend query failed" in str(excinfo.value)
+
+
+def test_get_samples_uses_cursor_path_when_engine_disallows_offset(
+ fake_datasource_factory,
+):
Review Comment:
**Suggestion:** Add missing type annotations for the function parameter and the return type on this test definition. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This test definition also omits type hints for its parameter and return value, so the rule violation is present in the existing code.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=57f8cddce4c946a4af18c03b1ac990c3&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=57f8cddce4c946a4af18c03b1ac990c3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/datasource/test_utils.py
**Line:** 141:143
**Comment:**
*Custom Rule: Add missing type annotations for the function parameter and the return type on this test definition.
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%2F39509&comment_hash=6ac572df966444a70e91e0d357cd661c69db04c61e0154355d3ad55cbcf81a8b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=6ac572df966444a70e91e0d357cd661c69db04c61e0154355d3ad55cbcf81a8b&reaction=dislike'>👎</a>
##########
superset/db_engine_specs/base.py:
##########
@@ -500,6 +500,12 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
allows_sql_comments = True
allows_escaped_colons = True
+ # Whether the engine supports OFFSET in SQL queries. Defaults to True;
+ # engines like Elasticsearch SQL that do not support OFFSET set this to
+ # False and are expected to implement `fetch_data_with_cursor` for
+ # pagination via another mechanism (e.g. Elasticsearch's cursor API).
+ supports_offset = True
Review Comment:
**Suggestion:** Add an explicit type annotation to this new class attribute so it complies with the type-hint requirement for newly introduced variables. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The new class attribute is a Python variable that can be annotated, but it is defined without a type hint. This matches the type-hint requirement for newly introduced or modified Python code.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=25ca602c80ae47eeb601042f85fda4e3&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=25ca602c80ae47eeb601042f85fda4e3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/base.py
**Line:** 507:507
**Comment:**
*Custom Rule: Add an explicit type annotation to this new class attribute so it complies with the type-hint requirement for newly introduced variables.
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%2F39509&comment_hash=3f107cd267b4deebc8d15926363405b51d9109c59dfe9ecc2a063672d32bd526&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=3f107cd267b4deebc8d15926363405b51d9109c59dfe9ecc2a063672d32bd526&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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #39509:
URL: https://github.com/apache/superset/pull/39509
--
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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "atrsa (via GitHub)" <gi...@apache.org>.
atrsa commented on code in PR #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3347999153
##########
tests/unit_tests/db_engine_specs/test_elasticsearch.py:
##########
@@ -92,3 +93,325 @@ def test_opendistro_sqla_column_label(original: str, expected: str) -> None:
from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
assert OpenDistroEngineSpec.make_label_compatible(original) == expected
+
+
+def test_elasticsearch_spec_opts_out_of_offset_fetch() -> None:
+ """
+ Elasticsearch SQL does not support OFFSET. The spec must opt out so the
+ query builder does not emit OFFSET clauses that crash the parser.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ assert ElasticSearchEngineSpec.supports_offset is False
+
+
+def test_opendistro_spec_opts_out_of_offset_fetch() -> None:
+ """
+ OpenDistro/OpenSearch SQL also does not support OFFSET.
+ """
+ from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
+
+ assert OpenDistroEngineSpec.supports_offset is False
+
+
+def _build_fake_database(transport_responses: list[dict[str, Any]]) -> MagicMock:
+ """
+ Build a mocked Database whose get_raw_connection() yields a connection
+ whose es.transport.perform_request returns transport_responses sequentially.
+ """
+ database = MagicMock(name="Database")
+
+ responses_iter = iter(transport_responses)
+
+ def perform_request(method, path, body=None, **_kwargs):
Review Comment:
Added explicit parameter and return type annotations to this `perform_request` mock callback.
##########
tests/unit_tests/db_engine_specs/test_elasticsearch.py:
##########
@@ -92,3 +93,325 @@ def test_opendistro_sqla_column_label(original: str, expected: str) -> None:
from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
assert OpenDistroEngineSpec.make_label_compatible(original) == expected
+
+
+def test_elasticsearch_spec_opts_out_of_offset_fetch() -> None:
+ """
+ Elasticsearch SQL does not support OFFSET. The spec must opt out so the
+ query builder does not emit OFFSET clauses that crash the parser.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ assert ElasticSearchEngineSpec.supports_offset is False
+
+
+def test_opendistro_spec_opts_out_of_offset_fetch() -> None:
+ """
+ OpenDistro/OpenSearch SQL also does not support OFFSET.
+ """
+ from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
+
+ assert OpenDistroEngineSpec.supports_offset is False
+
+
+def _build_fake_database(transport_responses: list[dict[str, Any]]) -> MagicMock:
+ """
+ Build a mocked Database whose get_raw_connection() yields a connection
+ whose es.transport.perform_request returns transport_responses sequentially.
+ """
+ database = MagicMock(name="Database")
+
+ responses_iter = iter(transport_responses)
+
+ def perform_request(method, path, body=None, **_kwargs):
+ return next(responses_iter)
+
+ transport = MagicMock()
+ transport.perform_request.side_effect = perform_request
+ conn = MagicMock()
+ conn.es.transport = transport
+
+ ctx = MagicMock()
+ ctx.__enter__ = MagicMock(return_value=conn)
+ ctx.__exit__ = MagicMock(return_value=False)
+ database.get_raw_connection.return_value = ctx
+ database._transport = transport # expose for assertions
+ return database
+
+
+def test_fetch_data_with_cursor_returns_first_page_when_page_index_zero() -> None:
+ """
+ Page index 0 = return the rows from the initial query, no cursor
+ iteration needed. The cursor must still be closed if present.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {
+ "columns": [{"name": "a"}, {"name": "b"}],
+ "rows": [[1, "x"], [2, "y"]],
+ "cursor": "CUR-1",
+ },
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a, b FROM idx",
+ page_index=0,
+ page_size=2,
+ )
+
+ assert rows == [[1, "x"], [2, "y"]]
+ assert cols == ["a", "b"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 2
+ assert calls[0][0][0] == "POST"
+ assert calls[0][0][1] == "/_sql"
+ assert calls[0].kwargs["body"] == {"query": "SELECT a, b FROM idx", "fetch_size": 2}
+ assert calls[1][0][1] == "/_sql/close"
+ assert calls[1].kwargs["body"] == {"cursor": "CUR-1"}
+
+
+def test_fetch_data_with_cursor_iterates_to_target_page() -> None:
+ """
+ For page_index=2, the code executes the initial query, then sends the
+ cursor twice. The rows returned belong to the third page.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]], "cursor": "C2"},
+ {"rows": [[2]], "cursor": "C3"},
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=2,
+ page_size=1,
+ )
+
+ assert rows == [[2]]
+ assert cols == ["a"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 4
+ assert calls[1].kwargs["body"] == {"cursor": "C1"}
+ assert calls[2].kwargs["body"] == {"cursor": "C2"}
+ assert calls[3][0][1] == "/_sql/close"
+ assert calls[3].kwargs["body"] == {"cursor": "C3"}
+
+
+def test_fetch_data_with_cursor_returns_empty_when_dataset_exhausted() -> None:
+ """
+ If the dataset has fewer pages than the requested page_index, the
+ cursor becomes falsy mid-iteration. Return empty rows, do not call
+ close, do not raise.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]]}, # no cursor → dataset ends here
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=5,
+ page_size=1,
+ )
+
+ assert rows == []
+ assert cols == ["a"]
+ assert len(database._transport.perform_request.call_args_list) == 2
+
+
+def test_fetch_data_with_cursor_does_not_close_when_no_cursor_present() -> None:
+ """
+ Some responses (tiny result sets) come back without a cursor token.
+ The code must not send a close request with a missing cursor.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0], [1]]},
+ ]
+ )
+
+ rows, _ = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=0,
+ page_size=50,
+ )
+
+ assert rows == [[0], [1]]
+ assert len(database._transport.perform_request.call_args_list) == 1
+
+
+def test_fetch_data_with_cursor_closes_cursor_even_if_iteration_raises() -> None:
+ """
+ If an intermediate cursor request raises, the cursor from the most
+ recent successful response must still be closed. Prevents server-side
+ cursor leaks on transport errors.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ class BoomError(RuntimeError):
+ pass
Review Comment:
Added a docstring to the `BoomError` test helper.
##########
tests/unit_tests/db_engine_specs/test_elasticsearch.py:
##########
@@ -92,3 +93,325 @@ def test_opendistro_sqla_column_label(original: str, expected: str) -> None:
from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
assert OpenDistroEngineSpec.make_label_compatible(original) == expected
+
+
+def test_elasticsearch_spec_opts_out_of_offset_fetch() -> None:
+ """
+ Elasticsearch SQL does not support OFFSET. The spec must opt out so the
+ query builder does not emit OFFSET clauses that crash the parser.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ assert ElasticSearchEngineSpec.supports_offset is False
+
+
+def test_opendistro_spec_opts_out_of_offset_fetch() -> None:
+ """
+ OpenDistro/OpenSearch SQL also does not support OFFSET.
+ """
+ from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
+
+ assert OpenDistroEngineSpec.supports_offset is False
+
+
+def _build_fake_database(transport_responses: list[dict[str, Any]]) -> MagicMock:
+ """
+ Build a mocked Database whose get_raw_connection() yields a connection
+ whose es.transport.perform_request returns transport_responses sequentially.
+ """
+ database = MagicMock(name="Database")
+
+ responses_iter = iter(transport_responses)
+
+ def perform_request(method, path, body=None, **_kwargs):
+ return next(responses_iter)
+
+ transport = MagicMock()
+ transport.perform_request.side_effect = perform_request
+ conn = MagicMock()
+ conn.es.transport = transport
+
+ ctx = MagicMock()
+ ctx.__enter__ = MagicMock(return_value=conn)
+ ctx.__exit__ = MagicMock(return_value=False)
+ database.get_raw_connection.return_value = ctx
+ database._transport = transport # expose for assertions
+ return database
+
+
+def test_fetch_data_with_cursor_returns_first_page_when_page_index_zero() -> None:
+ """
+ Page index 0 = return the rows from the initial query, no cursor
+ iteration needed. The cursor must still be closed if present.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {
+ "columns": [{"name": "a"}, {"name": "b"}],
+ "rows": [[1, "x"], [2, "y"]],
+ "cursor": "CUR-1",
+ },
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a, b FROM idx",
+ page_index=0,
+ page_size=2,
+ )
+
+ assert rows == [[1, "x"], [2, "y"]]
+ assert cols == ["a", "b"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 2
+ assert calls[0][0][0] == "POST"
+ assert calls[0][0][1] == "/_sql"
+ assert calls[0].kwargs["body"] == {"query": "SELECT a, b FROM idx", "fetch_size": 2}
+ assert calls[1][0][1] == "/_sql/close"
+ assert calls[1].kwargs["body"] == {"cursor": "CUR-1"}
+
+
+def test_fetch_data_with_cursor_iterates_to_target_page() -> None:
+ """
+ For page_index=2, the code executes the initial query, then sends the
+ cursor twice. The rows returned belong to the third page.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]], "cursor": "C2"},
+ {"rows": [[2]], "cursor": "C3"},
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=2,
+ page_size=1,
+ )
+
+ assert rows == [[2]]
+ assert cols == ["a"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 4
+ assert calls[1].kwargs["body"] == {"cursor": "C1"}
+ assert calls[2].kwargs["body"] == {"cursor": "C2"}
+ assert calls[3][0][1] == "/_sql/close"
+ assert calls[3].kwargs["body"] == {"cursor": "C3"}
+
+
+def test_fetch_data_with_cursor_returns_empty_when_dataset_exhausted() -> None:
+ """
+ If the dataset has fewer pages than the requested page_index, the
+ cursor becomes falsy mid-iteration. Return empty rows, do not call
+ close, do not raise.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]]}, # no cursor → dataset ends here
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=5,
+ page_size=1,
+ )
+
+ assert rows == []
+ assert cols == ["a"]
+ assert len(database._transport.perform_request.call_args_list) == 2
+
+
+def test_fetch_data_with_cursor_does_not_close_when_no_cursor_present() -> None:
+ """
+ Some responses (tiny result sets) come back without a cursor token.
+ The code must not send a close request with a missing cursor.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0], [1]]},
+ ]
+ )
+
+ rows, _ = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=0,
+ page_size=50,
+ )
+
+ assert rows == [[0], [1]]
+ assert len(database._transport.perform_request.call_args_list) == 1
+
+
+def test_fetch_data_with_cursor_closes_cursor_even_if_iteration_raises() -> None:
+ """
+ If an intermediate cursor request raises, the cursor from the most
+ recent successful response must still be closed. Prevents server-side
+ cursor leaks on transport errors.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ class BoomError(RuntimeError):
+ pass
+
+ responses = [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ ]
+
+ call_count = {"n": 0}
+ recorded_close = {}
+
+ def perform_request(method, path, body=None, **_kwargs):
Review Comment:
Added explicit parameter and return type annotations to this `perform_request` mock callback.
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
+ "helpers.py no longer references supports_offset; the OFFSET "
+ "guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
+ )
+
+ tree = ast.parse(source)
+ unguarded: list[int] = []
+
+ class Visitor(ast.NodeVisitor):
+ def __init__(self) -> None:
Review Comment:
Added a docstring to the `Visitor` class.
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
+ "helpers.py no longer references supports_offset; the OFFSET "
+ "guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
+ )
+
+ tree = ast.parse(source)
+ unguarded: list[int] = []
+
+ class Visitor(ast.NodeVisitor):
+ def __init__(self) -> None:
+ self._in_guarded_if = 0
Review Comment:
Added a docstring to the `__init__` method.
--
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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #39509:
URL: https://github.com/apache/superset/pull/39509#issuecomment-4286504679
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 057f04420e0bb0b8ed10fafc8305cf8e5c6fc297 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69e71e800b9f7e0008b5277c |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-39509--superset-docs-preview.netlify.app](https://deploy-preview-39509--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> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-39509--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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39509:
URL: https://github.com/apache/superset/pull/39509#issuecomment-4286480355
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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3115707418
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Use of assert in production code</b></div>
<div id="fix">
Replace `assert` statement with explicit error handling using `pytest.fail()` or raise an exception, as `assert` can be disabled with Python's `-O` flag.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
from pathlib import Path
+import pytest
HELPERS_PATH = (
Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
@@ -54,10 +55,10 @@
"""
source = HELPERS_PATH.read_text()
- assert "supports_offset" in source, (
+ if "supports_offset" not in source:
+ pytest.fail(
"helpers.py no longer references supports_offset; the OFFSET "
"guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
- )
+ )
```
</div>
</details>
</div>
<small><i>Code Review Run #0b865c</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
##########
superset/db_engine_specs/elasticsearch.py:
##########
@@ -29,16 +32,108 @@
SupersetDBAPIProgrammingError,
)
+if TYPE_CHECKING:
+ from superset.models.core import Database
+
logger = logging.getLogger()
+def _fetch_page_via_cursor(
+ database: Database,
+ sql: str,
+ page_index: int,
+ page_size: int,
+ sql_path: str,
+ close_path: str,
+) -> tuple[list[list[Any]], list[str]]:
+ """
+ Iterate Elasticsearch/OpenSearch SQL cursor pagination to return a single
+ page of results.
+
+ Executes ``sql`` with ``fetch_size = page_size``, then sends cursor
+ follow-up requests ``page_index`` times to skip earlier pages. Closes the
+ cursor when done to release server-side state. Returns
+ ``(rows, columns)``.
+
+ If the dataset is exhausted before reaching ``page_index``, returns an
+ empty rows list with the column names from the initial request.
+
+ Note: the Elasticsearch SQL cursor is forward-only, so cost is linear in
+ ``page_index`` — reaching page N issues N round trips to the cluster.
+ Deep pagination (hundreds of pages) will therefore be noticeably slower
+ than on ``OFFSET``-capable engines. This is a protocol limitation, not
+ an implementation choice.
+ """
+ # The Elasticsearch SQL API rejects trailing semicolons, and any LIMIT
+ # in the submitted statement caps the result set before the cursor can
+ # page through it. ``fetch_size`` drives pagination instead.
+ # Assumption: Superset only appends a trailing ``LIMIT N`` for engines
+ # with ``supports_offset=False``. If that ever changes (e.g.
+ # ``FETCH FIRST N ROWS`` or ``TOP N``), extend this sanitizer to match.
+ sanitized_sql = sql.strip().rstrip(";").strip()
+ sanitized_sql = re.sub(
+ r"\s+LIMIT\s+\d+\s*$", "", sanitized_sql, flags=re.IGNORECASE
+ )
+
+ # The raw transport does not auto-set Content-Type the way the Python
+ # DB-API driver does; ES rejects POSTs without a JSON content type.
+ json_headers = {"Content-Type": "application/json"}
+ with database.get_raw_connection() as conn:
+ transport = conn.es.transport
+ response = transport.perform_request(
+ "POST",
+ sql_path,
+ headers=json_headers,
+ body={"query": sanitized_sql, "fetch_size": page_size},
+ )
+ columns = [col["name"] for col in response.get("columns", [])]
+ rows = response.get("rows", [])
+ cursor = response.get("cursor")
+
+ try:
+ for _ in range(page_index):
+ if not cursor:
+ # Dataset exhausted before reaching the target page —
+ # no cursor to close (ES returns no cursor on the final
+ # page). Return immediately with empty rows.
+ return [], columns
+ response = transport.perform_request(
+ "POST",
+ sql_path,
+ headers=json_headers,
+ body={"cursor": cursor},
+ )
+ rows = response.get("rows", [])
+ cursor = response.get("cursor")
+
+ return rows, columns
+ finally:
+ if cursor:
+ # Best-effort cleanup. If close itself fails we don't want
+ # to mask the original error (if any) — swallow and log.
+ try:
+ transport.perform_request(
+ "POST",
+ close_path,
+ headers=json_headers,
+ body={"cursor": cursor},
+ )
+ except Exception: # pylint: disable=broad-except
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blind exception catch should be specific</b></div>
<div id="fix">
Replace the broad `Exception` catch on line 121 with a specific exception type (e.g., `ConnectionError`, `TimeoutError`, or `RequestException`) to avoid masking unexpected errors.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (ConnectionError, TimeoutError, Exception): # pylint: disable=broad-except
````
</div>
</details>
</div>
<small><i>Code Review Run #0b865c</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_elasticsearch.py:
##########
@@ -92,3 +93,325 @@ def test_opendistro_sqla_column_label(original: str, expected: str) -> None:
from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
assert OpenDistroEngineSpec.make_label_compatible(original) == expected
+
+
+def test_elasticsearch_spec_opts_out_of_offset_fetch() -> None:
+ """
+ Elasticsearch SQL does not support OFFSET. The spec must opt out so the
+ query builder does not emit OFFSET clauses that crash the parser.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ assert ElasticSearchEngineSpec.supports_offset is False
+
+
+def test_opendistro_spec_opts_out_of_offset_fetch() -> None:
+ """
+ OpenDistro/OpenSearch SQL also does not support OFFSET.
+ """
+ from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
+
+ assert OpenDistroEngineSpec.supports_offset is False
+
+
+def _build_fake_database(transport_responses: list[dict[str, Any]]) -> MagicMock:
+ """
+ Build a mocked Database whose get_raw_connection() yields a connection
+ whose es.transport.perform_request returns transport_responses sequentially.
+ """
+ database = MagicMock(name="Database")
+
+ responses_iter = iter(transport_responses)
+
+ def perform_request(method, path, body=None, **_kwargs):
+ return next(responses_iter)
+
+ transport = MagicMock()
+ transport.perform_request.side_effect = perform_request
+ conn = MagicMock()
+ conn.es.transport = transport
+
+ ctx = MagicMock()
+ ctx.__enter__ = MagicMock(return_value=conn)
+ ctx.__exit__ = MagicMock(return_value=False)
+ database.get_raw_connection.return_value = ctx
+ database._transport = transport # expose for assertions
+ return database
+
+
+def test_fetch_data_with_cursor_returns_first_page_when_page_index_zero() -> None:
+ """
+ Page index 0 = return the rows from the initial query, no cursor
+ iteration needed. The cursor must still be closed if present.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {
+ "columns": [{"name": "a"}, {"name": "b"}],
+ "rows": [[1, "x"], [2, "y"]],
+ "cursor": "CUR-1",
+ },
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a, b FROM idx",
+ page_index=0,
+ page_size=2,
+ )
+
+ assert rows == [[1, "x"], [2, "y"]]
+ assert cols == ["a", "b"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 2
+ assert calls[0][0][0] == "POST"
+ assert calls[0][0][1] == "/_sql"
+ assert calls[0].kwargs["body"] == {"query": "SELECT a, b FROM idx", "fetch_size": 2}
+ assert calls[1][0][1] == "/_sql/close"
+ assert calls[1].kwargs["body"] == {"cursor": "CUR-1"}
+
+
+def test_fetch_data_with_cursor_iterates_to_target_page() -> None:
+ """
+ For page_index=2, the code executes the initial query, then sends the
+ cursor twice. The rows returned belong to the third page.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]], "cursor": "C2"},
+ {"rows": [[2]], "cursor": "C3"},
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=2,
+ page_size=1,
+ )
+
+ assert rows == [[2]]
+ assert cols == ["a"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 4
+ assert calls[1].kwargs["body"] == {"cursor": "C1"}
+ assert calls[2].kwargs["body"] == {"cursor": "C2"}
+ assert calls[3][0][1] == "/_sql/close"
+ assert calls[3].kwargs["body"] == {"cursor": "C3"}
+
+
+def test_fetch_data_with_cursor_returns_empty_when_dataset_exhausted() -> None:
+ """
+ If the dataset has fewer pages than the requested page_index, the
+ cursor becomes falsy mid-iteration. Return empty rows, do not call
+ close, do not raise.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]]}, # no cursor → dataset ends here
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=5,
+ page_size=1,
+ )
+
+ assert rows == []
+ assert cols == ["a"]
+ assert len(database._transport.perform_request.call_args_list) == 2
+
+
+def test_fetch_data_with_cursor_does_not_close_when_no_cursor_present() -> None:
+ """
+ Some responses (tiny result sets) come back without a cursor token.
+ The code must not send a close request with a missing cursor.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0], [1]]},
+ ]
+ )
+
+ rows, _ = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=0,
+ page_size=50,
+ )
+
+ assert rows == [[0], [1]]
+ assert len(database._transport.perform_request.call_args_list) == 1
+
+
+def test_fetch_data_with_cursor_closes_cursor_even_if_iteration_raises() -> None:
+ """
+ If an intermediate cursor request raises, the cursor from the most
+ recent successful response must still be closed. Prevents server-side
+ cursor leaks on transport errors.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ class BoomError(RuntimeError):
+ pass
+
+ responses = [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ ]
+
+ call_count = {"n": 0}
+ recorded_close = {}
+
+ def perform_request(method, path, body=None, **_kwargs):
+ call_count["n"] += 1
+ # calls: 1=initial query, 2=cursor follow-up (raises), 3=close
+ if call_count["n"] == 2:
+ raise BoomError("transport blew up")
+ if path.endswith("/close"):
+ recorded_close["body"] = body
+ return {}
+ return responses[call_count["n"] - 1]
+
+ transport = MagicMock()
+ transport.perform_request.side_effect = perform_request
+ conn = MagicMock()
+ conn.es.transport = transport
+ ctx = MagicMock()
+ ctx.__enter__ = MagicMock(return_value=conn)
+ ctx.__exit__ = MagicMock(return_value=False)
+ database = MagicMock()
+ database.get_raw_connection.return_value = ctx
+
+ with pytest.raises(BoomError):
+ ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=3,
+ page_size=1,
+ )
+
+ assert recorded_close.get("body") == {"cursor": "C1"}, (
+ "The cursor from the last successful response must be closed "
+ "even when a later iteration raises."
+ )
+
+
+@pytest.mark.parametrize(
+ "sql_in,expected_query",
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Wrong parametrize argument type</b></div>
<div id="fix">
Change the first argument to `pytest.mark.parametrize` on line 317 from a string to a tuple. Use `("sql_in", "expected_query")` instead of `"sql_in,expected_query"`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
("sql_in", "expected_query"),
````
</div>
</details>
</div>
<small><i>Code Review Run #0b865c</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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #39509:
URL: https://github.com/apache/superset/pull/39509#issuecomment-4765057349
@atrsa GitHub shows conflicts here, so this needs a rebase on current `master` before we can re-run CI and review properly.
One thing I'd like your take on while you're revisiting it: `_fetch_samples_via_cursor` runs the normal samples payload to source `coltypes`, then does the cursor round trips on top, so every page ≥ 2 runs an extra query. The docstring already flags it as a TODO. Is the extra execution something we can avoid in this PR, or is it genuinely needed until we have an ES-type to coltype translator?
--
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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39509:
URL: https://github.com/apache/superset/pull/39509#issuecomment-4939170833
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 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(drill-detail): paginate Elasticsearch samples via engine cursor [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 #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3115587354
##########
tests/unit_tests/db_engine_specs/test_elasticsearch.py:
##########
@@ -92,3 +93,325 @@ def test_opendistro_sqla_column_label(original: str, expected: str) -> None:
from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
assert OpenDistroEngineSpec.make_label_compatible(original) == expected
+
+
+def test_elasticsearch_spec_opts_out_of_offset_fetch() -> None:
+ """
+ Elasticsearch SQL does not support OFFSET. The spec must opt out so the
+ query builder does not emit OFFSET clauses that crash the parser.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ assert ElasticSearchEngineSpec.supports_offset is False
+
+
+def test_opendistro_spec_opts_out_of_offset_fetch() -> None:
+ """
+ OpenDistro/OpenSearch SQL also does not support OFFSET.
+ """
+ from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
+
+ assert OpenDistroEngineSpec.supports_offset is False
+
+
+def _build_fake_database(transport_responses: list[dict[str, Any]]) -> MagicMock:
+ """
+ Build a mocked Database whose get_raw_connection() yields a connection
+ whose es.transport.perform_request returns transport_responses sequentially.
+ """
+ database = MagicMock(name="Database")
+
+ responses_iter = iter(transport_responses)
+
+ def perform_request(method, path, body=None, **_kwargs):
Review Comment:
**Suggestion:** Add explicit parameter and return type annotations to the newly added nested request helper to satisfy typing requirements for new functions. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
def perform_request(
method: str,
path: str,
body: dict[str, Any] | None = None,
**_kwargs: Any,
) -> dict[str, Any]:
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The existing helper is newly introduced and lacks explicit type annotations, which matches the stated typing rule. The improved code adds annotations for all parameters and the return type, and it uses names and types already available in the file (`Any` is imported). The syntax is valid for the codebase's Python version.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fdb_engine_specs%2Ftest_elasticsearch.py%0A%2A%2ALine%3A%2A%2A%20126%3A126%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20explicit%20parameter%20and%20return%20type%20annotations%20to%20the%20newly%20added%20nested%20request%20helper%20to%20satisfy%20typing%20requirements%20for%20new%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20imple
ment%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fdb_engine_specs%2Ftest_elasticsearch.py%0A%2A%2ALine%3A%2A%2A%20126%3A126%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20explicit%20parameter%20and%20return%20type%20annotations%20to%20the%20newly%20added%20nested%20request%20helper%20to%20satisfy%20typing%20requirements%20for%20new%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%2
0validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_elasticsearch.py
**Line:** 126:126
**Comment:**
*Custom Rule: Add explicit parameter and return type annotations to the newly added nested request helper to satisfy typing requirements for new functions.
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%2F39509&comment_hash=6434a3de9153a8c255395bbc41f6b91dc9bcd5381296dc5feb5c41076df10157&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=6434a3de9153a8c255395bbc41f6b91dc9bcd5381296dc5feb5c41076df10157&reaction=dislike'>👎</a>
##########
tests/unit_tests/db_engine_specs/test_elasticsearch.py:
##########
@@ -92,3 +93,325 @@ def test_opendistro_sqla_column_label(original: str, expected: str) -> None:
from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
assert OpenDistroEngineSpec.make_label_compatible(original) == expected
+
+
+def test_elasticsearch_spec_opts_out_of_offset_fetch() -> None:
+ """
+ Elasticsearch SQL does not support OFFSET. The spec must opt out so the
+ query builder does not emit OFFSET clauses that crash the parser.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ assert ElasticSearchEngineSpec.supports_offset is False
+
+
+def test_opendistro_spec_opts_out_of_offset_fetch() -> None:
+ """
+ OpenDistro/OpenSearch SQL also does not support OFFSET.
+ """
+ from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
+
+ assert OpenDistroEngineSpec.supports_offset is False
+
+
+def _build_fake_database(transport_responses: list[dict[str, Any]]) -> MagicMock:
+ """
+ Build a mocked Database whose get_raw_connection() yields a connection
+ whose es.transport.perform_request returns transport_responses sequentially.
+ """
+ database = MagicMock(name="Database")
+
+ responses_iter = iter(transport_responses)
+
+ def perform_request(method, path, body=None, **_kwargs):
+ return next(responses_iter)
+
+ transport = MagicMock()
+ transport.perform_request.side_effect = perform_request
+ conn = MagicMock()
+ conn.es.transport = transport
+
+ ctx = MagicMock()
+ ctx.__enter__ = MagicMock(return_value=conn)
+ ctx.__exit__ = MagicMock(return_value=False)
+ database.get_raw_connection.return_value = ctx
+ database._transport = transport # expose for assertions
+ return database
+
+
+def test_fetch_data_with_cursor_returns_first_page_when_page_index_zero() -> None:
+ """
+ Page index 0 = return the rows from the initial query, no cursor
+ iteration needed. The cursor must still be closed if present.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {
+ "columns": [{"name": "a"}, {"name": "b"}],
+ "rows": [[1, "x"], [2, "y"]],
+ "cursor": "CUR-1",
+ },
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a, b FROM idx",
+ page_index=0,
+ page_size=2,
+ )
+
+ assert rows == [[1, "x"], [2, "y"]]
+ assert cols == ["a", "b"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 2
+ assert calls[0][0][0] == "POST"
+ assert calls[0][0][1] == "/_sql"
+ assert calls[0].kwargs["body"] == {"query": "SELECT a, b FROM idx", "fetch_size": 2}
+ assert calls[1][0][1] == "/_sql/close"
+ assert calls[1].kwargs["body"] == {"cursor": "CUR-1"}
+
+
+def test_fetch_data_with_cursor_iterates_to_target_page() -> None:
+ """
+ For page_index=2, the code executes the initial query, then sends the
+ cursor twice. The rows returned belong to the third page.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]], "cursor": "C2"},
+ {"rows": [[2]], "cursor": "C3"},
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=2,
+ page_size=1,
+ )
+
+ assert rows == [[2]]
+ assert cols == ["a"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 4
+ assert calls[1].kwargs["body"] == {"cursor": "C1"}
+ assert calls[2].kwargs["body"] == {"cursor": "C2"}
+ assert calls[3][0][1] == "/_sql/close"
+ assert calls[3].kwargs["body"] == {"cursor": "C3"}
+
+
+def test_fetch_data_with_cursor_returns_empty_when_dataset_exhausted() -> None:
+ """
+ If the dataset has fewer pages than the requested page_index, the
+ cursor becomes falsy mid-iteration. Return empty rows, do not call
+ close, do not raise.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]]}, # no cursor → dataset ends here
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=5,
+ page_size=1,
+ )
+
+ assert rows == []
+ assert cols == ["a"]
+ assert len(database._transport.perform_request.call_args_list) == 2
+
+
+def test_fetch_data_with_cursor_does_not_close_when_no_cursor_present() -> None:
+ """
+ Some responses (tiny result sets) come back without a cursor token.
+ The code must not send a close request with a missing cursor.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0], [1]]},
+ ]
+ )
+
+ rows, _ = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=0,
+ page_size=50,
+ )
+
+ assert rows == [[0], [1]]
+ assert len(database._transport.perform_request.call_args_list) == 1
+
+
+def test_fetch_data_with_cursor_closes_cursor_even_if_iteration_raises() -> None:
+ """
+ If an intermediate cursor request raises, the cursor from the most
+ recent successful response must still be closed. Prevents server-side
+ cursor leaks on transport errors.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ class BoomError(RuntimeError):
+ pass
+
+ responses = [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ ]
+
+ call_count = {"n": 0}
+ recorded_close = {}
+
+ def perform_request(method, path, body=None, **_kwargs):
Review Comment:
**Suggestion:** Add explicit parameter and return type annotations to this newly added nested request helper to comply with the type-hinting rule for new functions. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
def perform_request(
method: str,
path: str,
body: dict[str, Any] | None = None,
**_kwargs: Any,
) -> dict[str, Any]:
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The callback is newly added and currently untyped, so the type-hinting rule is violated if that rule applies to new functions. The proposed replacement adds explicit types without changing behavior, and all referenced symbols (`Any`, `responses`, `BoomError`) exist in the surrounding test.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fdb_engine_specs%2Ftest_elasticsearch.py%0A%2A%2ALine%3A%2A%2A%20282%3A282%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20explicit%20parameter%20and%20return%20type%20annotations%20to%20this%20newly%20added%20nested%20request%20helper%20to%20comply%20with%20the%20type-hinting%20rule%20for%20new%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20
and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fdb_engine_specs%2Ftest_elasticsearch.py%0A%2A%2ALine%3A%2A%2A%20282%3A282%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20explicit%20parameter%20and%20return%20type%20annotations%20to%20this%20newly%20added%20nested%20request%20helper%20to%20comply%20with%20the%20type-hinting%20rule%20for%20new%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20
all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_elasticsearch.py
**Line:** 282:282
**Comment:**
*Custom Rule: Add explicit parameter and return type annotations to this newly added nested request helper to comply with the type-hinting rule for new functions.
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%2F39509&comment_hash=7c3942308ae29eb5d0e986e361b14c6f5aed4785e4a42f4995f3643bf9b1fa6c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=7c3942308ae29eb5d0e986e361b14c6f5aed4785e4a42f4995f3643bf9b1fa6c&reaction=dislike'>👎</a>
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
+ "helpers.py no longer references supports_offset; the OFFSET "
+ "guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
+ )
+
+ tree = ast.parse(source)
+ unguarded: list[int] = []
+
+ class Visitor(ast.NodeVisitor):
+ def __init__(self) -> None:
+ self._in_guarded_if = 0
+
+ def visit_If(self, node: ast.If) -> None: # noqa: N802
+ if _uses_supports_offset(node.test):
+ self._in_guarded_if += 1
+ for child in node.body:
+ self.visit(child)
+ self._in_guarded_if -= 1
+ for child in node.orelse:
+ self.visit(child)
+ else:
+ self.generic_visit(node)
+
+ def visit_Assign(self, node: ast.Assign) -> None: # noqa: N802
+ if _is_qry_offset_assignment(node) and self._in_guarded_if == 0:
Review Comment:
**Suggestion:** Add a docstring to the new assignment visitor method to comply with the docstring rule for added functions. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
"""Record line numbers for offset assignments that are outside guarded blocks."""
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new visitor method is missing a docstring, so the suggestion correctly addresses the violation. The added string literal is valid and does not introduce any code changes beyond documentation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmodels%2Ftest_helpers_offset.py%0A%2A%2ALine%3A%2A%2A%2079%3A79%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20new%20assignment%20visitor%20method%20to%20comply%20with%20the%20docstring%20rule%20for%20added%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Clau
de](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmodels%2Ftest_helpers_offset.py%0A%2A%2ALine%3A%2A%2A%2079%3A79%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20new%20assignment%20visitor%20method%20to%20comply%20with%20the%20docstring%20rule%20for%20added%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/models/test_helpers_offset.py
**Line:** 79:79
**Comment:**
*Custom Rule: Add a docstring to the new assignment visitor method to comply with the docstring rule for added functions.
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%2F39509&comment_hash=438906baec56faabc40d8d192210446fdfa3c640ae33df65f5ec0fc485e37e6d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=438906baec56faabc40d8d192210446fdfa3c640ae33df65f5ec0fc485e37e6d&reaction=dislike'>👎</a>
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
+ "helpers.py no longer references supports_offset; the OFFSET "
+ "guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
+ )
+
+ tree = ast.parse(source)
+ unguarded: list[int] = []
+
+ class Visitor(ast.NodeVisitor):
+ def __init__(self) -> None:
+ self._in_guarded_if = 0
Review Comment:
**Suggestion:** Add a docstring to the new initializer method to satisfy the docstring requirement for newly added functions. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
"""Initialize the guard-depth tracker for conditional offset checks."""
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The initializer is newly added and lacks a docstring, so the suggestion matches the rule violation. The added docstring is valid Python and does not change behavior.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmodels%2Ftest_helpers_offset.py%0A%2A%2ALine%3A%2A%2A%2065%3A65%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20new%20initializer%20method%20to%20satisfy%20the%20docstring%20requirement%20for%20newly%20added%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Clau
de](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmodels%2Ftest_helpers_offset.py%0A%2A%2ALine%3A%2A%2A%2065%3A65%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20new%20initializer%20method%20to%20satisfy%20the%20docstring%20requirement%20for%20newly%20added%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/models/test_helpers_offset.py
**Line:** 65:65
**Comment:**
*Custom Rule: Add a docstring to the new initializer method to satisfy the docstring requirement for newly added functions.
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%2F39509&comment_hash=d78ff2330fa8eabc19c1f340fc82283349add4c8820487c120c8f42e07345381&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=d78ff2330fa8eabc19c1f340fc82283349add4c8820487c120c8f42e07345381&reaction=dislike'>👎</a>
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
+ "helpers.py no longer references supports_offset; the OFFSET "
+ "guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
+ )
+
+ tree = ast.parse(source)
+ unguarded: list[int] = []
+
+ class Visitor(ast.NodeVisitor):
+ def __init__(self) -> None:
Review Comment:
**Suggestion:** Add a docstring to the newly introduced inner class so the class definition is self-documented. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
"""Walk the AST and track whether offset assignments are properly guarded."""
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The suggestion addresses a real docstring omission in the newly introduced inner class. Adding a class docstring is syntactically valid and does not affect runtime behavior.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmodels%2Ftest_helpers_offset.py%0A%2A%2ALine%3A%2A%2A%2064%3A64%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20newly%20introduced%20inner%20class%20so%20the%20class%20definition%20is%20self-documented.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.co
deant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmodels%2Ftest_helpers_offset.py%0A%2A%2ALine%3A%2A%2A%2064%3A64%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20newly%20introduced%20inner%20class%20so%20the%20class%20definition%20is%20self-documented.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/models/test_helpers_offset.py
**Line:** 64:64
**Comment:**
*Custom Rule: Add a docstring to the newly introduced inner class so the class definition is self-documented.
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%2F39509&comment_hash=ca199d0c43605b9e3fcdc9c33976561f06ac1475176882f2b5205cdaa04e634a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=ca199d0c43605b9e3fcdc9c33976561f06ac1475176882f2b5205cdaa04e634a&reaction=dislike'>👎</a>
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
+ "helpers.py no longer references supports_offset; the OFFSET "
+ "guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
+ )
+
+ tree = ast.parse(source)
+ unguarded: list[int] = []
+
+ class Visitor(ast.NodeVisitor):
+ def __init__(self) -> None:
+ self._in_guarded_if = 0
+
+ def visit_If(self, node: ast.If) -> None: # noqa: N802
+ if _uses_supports_offset(node.test):
Review Comment:
**Suggestion:** Add a docstring to the new visitor method so all newly added functions include documentation. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
"""Track guarded branches when an `if` condition checks `supports_offset`."""
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced method without a docstring, so the suggestion is aligned with the docstring rule. The improved code is syntactically correct and only adds documentation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmodels%2Ftest_helpers_offset.py%0A%2A%2ALine%3A%2A%2A%2068%3A68%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20new%20visitor%20method%20so%20all%20newly%20added%20functions%20include%20documentation.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.code
ant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmodels%2Ftest_helpers_offset.py%0A%2A%2ALine%3A%2A%2A%2068%3A68%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20new%20visitor%20method%20so%20all%20newly%20added%20functions%20include%20documentation.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/models/test_helpers_offset.py
**Line:** 68:68
**Comment:**
*Custom Rule: Add a docstring to the new visitor method so all newly added functions include documentation.
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%2F39509&comment_hash=3874959afbf7f494d9f8acced41c304e8c9d14d07e667236c160f6f8c57d28b4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=3874959afbf7f494d9f8acced41c304e8c9d14d07e667236c160f6f8c57d28b4&reaction=dislike'>👎</a>
##########
tests/unit_tests/db_engine_specs/test_elasticsearch.py:
##########
@@ -92,3 +93,325 @@ def test_opendistro_sqla_column_label(original: str, expected: str) -> None:
from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
assert OpenDistroEngineSpec.make_label_compatible(original) == expected
+
+
+def test_elasticsearch_spec_opts_out_of_offset_fetch() -> None:
+ """
+ Elasticsearch SQL does not support OFFSET. The spec must opt out so the
+ query builder does not emit OFFSET clauses that crash the parser.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ assert ElasticSearchEngineSpec.supports_offset is False
+
+
+def test_opendistro_spec_opts_out_of_offset_fetch() -> None:
+ """
+ OpenDistro/OpenSearch SQL also does not support OFFSET.
+ """
+ from superset.db_engine_specs.elasticsearch import OpenDistroEngineSpec
+
+ assert OpenDistroEngineSpec.supports_offset is False
+
+
+def _build_fake_database(transport_responses: list[dict[str, Any]]) -> MagicMock:
+ """
+ Build a mocked Database whose get_raw_connection() yields a connection
+ whose es.transport.perform_request returns transport_responses sequentially.
+ """
+ database = MagicMock(name="Database")
+
+ responses_iter = iter(transport_responses)
+
+ def perform_request(method, path, body=None, **_kwargs):
+ return next(responses_iter)
+
+ transport = MagicMock()
+ transport.perform_request.side_effect = perform_request
+ conn = MagicMock()
+ conn.es.transport = transport
+
+ ctx = MagicMock()
+ ctx.__enter__ = MagicMock(return_value=conn)
+ ctx.__exit__ = MagicMock(return_value=False)
+ database.get_raw_connection.return_value = ctx
+ database._transport = transport # expose for assertions
+ return database
+
+
+def test_fetch_data_with_cursor_returns_first_page_when_page_index_zero() -> None:
+ """
+ Page index 0 = return the rows from the initial query, no cursor
+ iteration needed. The cursor must still be closed if present.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {
+ "columns": [{"name": "a"}, {"name": "b"}],
+ "rows": [[1, "x"], [2, "y"]],
+ "cursor": "CUR-1",
+ },
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a, b FROM idx",
+ page_index=0,
+ page_size=2,
+ )
+
+ assert rows == [[1, "x"], [2, "y"]]
+ assert cols == ["a", "b"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 2
+ assert calls[0][0][0] == "POST"
+ assert calls[0][0][1] == "/_sql"
+ assert calls[0].kwargs["body"] == {"query": "SELECT a, b FROM idx", "fetch_size": 2}
+ assert calls[1][0][1] == "/_sql/close"
+ assert calls[1].kwargs["body"] == {"cursor": "CUR-1"}
+
+
+def test_fetch_data_with_cursor_iterates_to_target_page() -> None:
+ """
+ For page_index=2, the code executes the initial query, then sends the
+ cursor twice. The rows returned belong to the third page.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]], "cursor": "C2"},
+ {"rows": [[2]], "cursor": "C3"},
+ {}, # close
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=2,
+ page_size=1,
+ )
+
+ assert rows == [[2]]
+ assert cols == ["a"]
+
+ calls = database._transport.perform_request.call_args_list
+ assert len(calls) == 4
+ assert calls[1].kwargs["body"] == {"cursor": "C1"}
+ assert calls[2].kwargs["body"] == {"cursor": "C2"}
+ assert calls[3][0][1] == "/_sql/close"
+ assert calls[3].kwargs["body"] == {"cursor": "C3"}
+
+
+def test_fetch_data_with_cursor_returns_empty_when_dataset_exhausted() -> None:
+ """
+ If the dataset has fewer pages than the requested page_index, the
+ cursor becomes falsy mid-iteration. Return empty rows, do not call
+ close, do not raise.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0]], "cursor": "C1"},
+ {"rows": [[1]]}, # no cursor → dataset ends here
+ ]
+ )
+
+ rows, cols = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=5,
+ page_size=1,
+ )
+
+ assert rows == []
+ assert cols == ["a"]
+ assert len(database._transport.perform_request.call_args_list) == 2
+
+
+def test_fetch_data_with_cursor_does_not_close_when_no_cursor_present() -> None:
+ """
+ Some responses (tiny result sets) come back without a cursor token.
+ The code must not send a close request with a missing cursor.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ database = _build_fake_database(
+ [
+ {"columns": [{"name": "a"}], "rows": [[0], [1]]},
+ ]
+ )
+
+ rows, _ = ElasticSearchEngineSpec.fetch_data_with_cursor(
+ database=database,
+ sql="SELECT a FROM idx",
+ page_index=0,
+ page_size=50,
+ )
+
+ assert rows == [[0], [1]]
+ assert len(database._transport.perform_request.call_args_list) == 1
+
+
+def test_fetch_data_with_cursor_closes_cursor_even_if_iteration_raises() -> None:
+ """
+ If an intermediate cursor request raises, the cursor from the most
+ recent successful response must still be closed. Prevents server-side
+ cursor leaks on transport errors.
+ """
+ from superset.db_engine_specs.elasticsearch import ElasticSearchEngineSpec
+
+ class BoomError(RuntimeError):
+ pass
Review Comment:
**Suggestion:** Add a class docstring to the new exception class so newly introduced classes include documentation. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
"""Raised when the mocked transport fails during cursor iteration."""
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new class is introduced without a docstring, so this directly addresses a class-documentation rule. Adding the docstring is syntactically valid and does not affect runtime behavior.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fdb_engine_specs%2Ftest_elasticsearch.py%0A%2A%2ALine%3A%2A%2A%20273%3A273%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20class%20docstring%20to%20the%20new%20exception%20class%20so%20newly%20introduced%20classes%20include%20documentation.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](
https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fdb_engine_specs%2Ftest_elasticsearch.py%0A%2A%2ALine%3A%2A%2A%20273%3A273%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20class%20docstring%20to%20the%20new%20exception%20class%20so%20newly%20introduced%20classes%20include%20documentation.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_elasticsearch.py
**Line:** 273:273
**Comment:**
*Custom Rule: Add a class docstring to the new exception class so newly introduced classes include documentation.
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%2F39509&comment_hash=3e1f0b5e83353d4f8f032cb62b66e9338ea25f284a11432e0b68c9d5a64dcf10&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=3e1f0b5e83353d4f8f032cb62b66e9338ea25f284a11432e0b68c9d5a64dcf10&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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39509:
URL: https://github.com/apache/superset/pull/39509#issuecomment-4929415706
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 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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3347470580
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to replace `assert` with `pytest.fail()` is a general best practice for production code to avoid issues with Python's `-O` optimization flag. However, in the context of a unit-test module where `pytest` is the standard runner and `assert` is the idiomatic way to verify conditions, your assessment that this is a false positive is correct. You are not required to apply this suggestion.
**tests/unit_tests/models/test_helpers_offset.py**
```
- assert "supports_offset" in source, (
+ if "supports_offset" not in source:
+ pytest.fail(
"helpers.py no longer references supports_offset; the OFFSET "
"guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
- )
+ )
```
##########
superset/db_engine_specs/elasticsearch.py:
##########
@@ -29,16 +32,108 @@
SupersetDBAPIProgrammingError,
)
+if TYPE_CHECKING:
+ from superset.models.core import Database
+
logger = logging.getLogger()
+def _fetch_page_via_cursor(
+ database: Database,
+ sql: str,
+ page_index: int,
+ page_size: int,
+ sql_path: str,
+ close_path: str,
+) -> tuple[list[list[Any]], list[str]]:
+ """
+ Iterate Elasticsearch/OpenSearch SQL cursor pagination to return a single
+ page of results.
+
+ Executes ``sql`` with ``fetch_size = page_size``, then sends cursor
+ follow-up requests ``page_index`` times to skip earlier pages. Closes the
+ cursor when done to release server-side state. Returns
+ ``(rows, columns)``.
+
+ If the dataset is exhausted before reaching ``page_index``, returns an
+ empty rows list with the column names from the initial request.
+
+ Note: the Elasticsearch SQL cursor is forward-only, so cost is linear in
+ ``page_index`` — reaching page N issues N round trips to the cluster.
+ Deep pagination (hundreds of pages) will therefore be noticeably slower
+ than on ``OFFSET``-capable engines. This is a protocol limitation, not
+ an implementation choice.
+ """
+ # The Elasticsearch SQL API rejects trailing semicolons, and any LIMIT
+ # in the submitted statement caps the result set before the cursor can
+ # page through it. ``fetch_size`` drives pagination instead.
+ # Assumption: Superset only appends a trailing ``LIMIT N`` for engines
+ # with ``supports_offset=False``. If that ever changes (e.g.
+ # ``FETCH FIRST N ROWS`` or ``TOP N``), extend this sanitizer to match.
+ sanitized_sql = sql.strip().rstrip(";").strip()
+ sanitized_sql = re.sub(
+ r"\s+LIMIT\s+\d+\s*$", "", sanitized_sql, flags=re.IGNORECASE
+ )
+
+ # The raw transport does not auto-set Content-Type the way the Python
+ # DB-API driver does; ES rejects POSTs without a JSON content type.
+ json_headers = {"Content-Type": "application/json"}
+ with database.get_raw_connection() as conn:
+ transport = conn.es.transport
+ response = transport.perform_request(
+ "POST",
+ sql_path,
+ headers=json_headers,
+ body={"query": sanitized_sql, "fetch_size": page_size},
+ )
+ columns = [col["name"] for col in response.get("columns", [])]
+ rows = response.get("rows", [])
+ cursor = response.get("cursor")
+
+ try:
+ for _ in range(page_index):
+ if not cursor:
+ # Dataset exhausted before reaching the target page —
+ # no cursor to close (ES returns no cursor on the final
+ # page). Return immediately with empty rows.
+ return [], columns
+ response = transport.perform_request(
+ "POST",
+ sql_path,
+ headers=json_headers,
+ body={"cursor": cursor},
+ )
+ rows = response.get("rows", [])
+ cursor = response.get("cursor")
+
+ return rows, columns
+ finally:
+ if cursor:
+ # Best-effort cleanup. If close itself fails we don't want
+ # to mask the original error (if any) — swallow and log.
+ try:
+ transport.perform_request(
+ "POST",
+ close_path,
+ headers=json_headers,
+ body={"cursor": cursor},
+ )
+ except Exception: # pylint: disable=broad-except
Review Comment:
<!-- Bito Reply -->
The suggestion to replace the broad `Exception` catch with specific exceptions is intended to improve error handling precision. However, since the `finally` block is used for resource cleanup and the broad catch is already annotated with a pylint disable comment to prevent masking errors, maintaining the current implementation is acceptable. You may choose to ignore this suggestion if the current approach is necessary for your cleanup logic.
**superset/db_engine_specs/elasticsearch.py**
```
except (ConnectionError, TimeoutError, Exception): # pylint: disable=broad-except
```
--
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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39509:
URL: https://github.com/apache/superset/pull/39509#issuecomment-4612550381
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 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(drill-detail): paginate Elasticsearch samples via engine cursor [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 #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3559552449
##########
tests/unit_tests/models/test_helpers_offset.py:
##########
@@ -0,0 +1,93 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import ast
+from pathlib import Path
+
+HELPERS_PATH = (
+ Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py"
+)
+
+
+def _uses_supports_offset(node: ast.AST) -> bool:
+ """True if any attribute access on `node` references 'supports_offset'."""
+ return any(
+ isinstance(child, ast.Attribute) and child.attr == "supports_offset"
+ for child in ast.walk(node)
+ )
+
+
+def _is_qry_offset_assignment(stmt: ast.AST) -> bool:
+ """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`)."""
+ if not isinstance(stmt, ast.Assign):
+ return False
+ call = stmt.value
+ if not isinstance(call, ast.Call):
+ return False
+ func = call.func
+ return isinstance(func, ast.Attribute) and func.attr == "offset"
+
+
+def test_helpers_guards_offset_with_supports_offset_flag() -> None:
+ """
+ Regression guard: the `.offset()` call in get_sqla_query must be wrapped
+ in an `if` that checks `supports_offset`. Without this guard,
+ engines that do not support OFFSET (Elasticsearch SQL) crash drill-
+ to-detail on page 2+.
+
+ We parse the AST rather than grep the source so the test survives
+ Black-style reformatting and trivial refactors.
+ """
+ source = HELPERS_PATH.read_text()
+ assert "supports_offset" in source, (
+ "helpers.py no longer references supports_offset; the OFFSET "
+ "guard is gone — Elasticsearch drill-to-detail will crash on page 2+."
+ )
+
+ tree = ast.parse(source)
+ unguarded: list[int] = []
+
+ class Visitor(ast.NodeVisitor):
+ """Flag `.offset()` assignments not guarded by a `supports_offset` check."""
+
+ def __init__(self) -> None:
+ """Track nesting depth inside `supports_offset`-guarded `if` blocks."""
+ self._in_guarded_if = 0
+
+ def visit_If(self, node: ast.If) -> None: # noqa: N802
+ """Descend into the body under a `supports_offset` guard when present."""
+ if _uses_supports_offset(node.test):
+ self._in_guarded_if += 1
+ for child in node.body:
+ self.visit(child)
+ self._in_guarded_if -= 1
+ for child in node.orelse:
+ self.visit(child)
+ else:
+ self.generic_visit(node)
Review Comment:
**Suggestion:** The AST guard logic only checks whether `supports_offset` appears anywhere in the `if` condition, but it does not verify that the condition is actually the positive guard (eg `...supports_offset` rather than `not ...supports_offset`). Because of that, this test would still pass if `.offset()` were placed under an inverted condition and emitted on engines that do not support offsets. Tighten the AST check to validate the condition polarity before marking a block as guarded. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Future inverted OFFSET guard not caught by regression test.
⚠️ Drill-detail and samples may emit OFFSET on unsupported engines.
⚠️ Test suite gives false assurance about offset gating.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset/models/helpers.py` and locate `ExploreMixin.get_sqla_query` near lines
3900–4019; observe the OFFSET guard: `if row_offset and
self.database.db_engine_spec.supports_offset: qry = qry.offset(row_offset)` at lines
3934–3936.
2. Open `tests/unit_tests/models/test_helpers_offset.py` and inspect
`_uses_supports_offset` (lines 25–30) and `Visitor.visit_If` (lines 70–80); note that
`_uses_supports_offset` returns True for any `ast.If.test` containing an attribute named
`supports_offset`, without checking polarity (e.g. positive vs `not supports_offset`), and
`visit_If` treats only the `if` body as guarded when `_uses_supports_offset(node.test)` is
True.
3. Consider a regression where `helpers.py` is modified so the guard is inverted, e.g. `if
row_offset and not self.database.db_engine_spec.supports_offset: qry =
qry.offset(row_offset)` at the same location (lines 3934–3936). In the AST, the `If.test`
still contains a `supports_offset` attribute access, so `_uses_supports_offset(node.test)`
returns True, `_in_guarded_if` is incremented while visiting `node.body`, and the `qry =
qry.offset(...)` assignment is seen with `_in_guarded_if == 1`, causing `visit_Assign`
(lines 82–86) not to record it in `unguarded`. The Visitor finishes with `unguarded`
empty, and the regression test still passes even though `.offset()` is now emitted
precisely when `supports_offset` is False.
4. Follow the runtime path for samples/drill-to-detail pagination: `get_samples` in
`superset/views/datasource/utils.py` (lines 98–215) uses `get_limit_clause` to compute
`row_offset`/`row_limit` and builds a `QueryContext` that ultimately calls
`datasource.get_query_str(...)`, which in turn uses `ExploreMixin.get_query_str_extended`
and `get_sqla_query` (helpers.py lines 1443–1455 and 3900–4019). On an engine where
`engine_spec.supports_offset` is False (e.g. Elasticsearch SQL), the inverted guard would
still attach `.offset(row_offset)` to the compiled SQL, causing `parsing_exception` errors
on page > 1 while the regression test in `tests/unit_tests/models/test_helpers_offset.py`
continues to pass, because it only checks for the presence of `supports_offset` in the
condition, not that the condition is a positive guard.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c21f900496f0477a8d0785ef329940b8&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=c21f900496f0477a8d0785ef329940b8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/models/test_helpers_offset.py
**Line:** 70:80
**Comment:**
*Incorrect Condition Logic: The AST guard logic only checks whether `supports_offset` appears anywhere in the `if` condition, but it does not verify that the condition is actually the positive guard (eg `...supports_offset` rather than `not ...supports_offset`). Because of that, this test would still pass if `.offset()` were placed under an inverted condition and emitted on engines that do not support offsets. Tighten the AST check to validate the condition polarity before marking a block as guarded.
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%2F39509&comment_hash=3c798d625855e75a305261e86532cb34fb14a246bb0a77f92cf1f7e07adbfcbc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=3c798d625855e75a305261e86532cb34fb14a246bb0a77f92cf1f7e07adbfcbc&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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "atrsa (via GitHub)" <gi...@apache.org>.
atrsa commented on PR #39509:
URL: https://github.com/apache/superset/pull/39509#issuecomment-4928251989
> @atrsa GitHub shows conflicts here, so this needs a rebase on current `master` before we can re-run CI and review properly.
>
> One thing I'd like your take on while you're revisiting it: `_fetch_samples_via_cursor` runs the normal samples payload to source `coltypes`, then does the cursor round trips on top, so every page ≥ 2 runs an extra query. The docstring already flags it as a TODO. Is the extra execution something we can avoid in this PR, or is it genuinely needed until we have an ES-type to coltype translator?
We can, addressed that along with conflicts resolution. Now inferred from returned 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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39509:
URL: https://github.com/apache/superset/pull/39509#discussion_r3554768033
##########
superset/views/datasource/utils.py:
##########
@@ -176,15 +178,88 @@ def get_samples( # pylint: disable=too-many-arguments
if count_star_data.get("status") == QueryStatus.FAILED:
raise DatasetSamplesFailedError(count_star_data.get("error"))
- sample_data = samples_instance.get_payload()["queries"][0]
+ engine_spec = datasource.database.db_engine_spec
+ row_offset = limit_clause["row_offset"]
+ row_limit = limit_clause["row_limit"]
- if sample_data.get("status") == QueryStatus.FAILED:
- QueryCacheManager.delete(count_star_data.get("cache_key"), CacheRegion.DATA)
- raise DatasetSamplesFailedError(sample_data.get("error"))
+ if not engine_spec.supports_offset and row_offset > 0:
+ try:
+ sample_data = _fetch_samples_via_cursor(
+ datasource=datasource,
+ samples_instance=samples_instance,
+ count_star_data=count_star_data,
+ page_index=row_offset // row_limit,
+ page_size=row_limit,
+ )
+ except DatasetSamplesFailedError:
+ raise
+ except Exception as exc:
+ QueryCacheManager.delete(
+ count_star_data.get("cache_key"), CacheRegion.DATA
+ )
+ logger.exception("Cursor-based samples pagination failed")
+ raise DatasetSamplesFailedError(
+ "Failed to fetch samples via cursor pagination"
+ ) from exc
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Assign exception message to variable</b></div>
<div id="fix">
TRY003 and EM101 violations: Avoid long exception messages and don't use string literals directly in exception constructors. Assign the message to a variable first.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
)
logger.exception("Cursor-based samples pagination failed")
message = "Failed to fetch samples via cursor pagination"
raise DatasetSamplesFailedError(
message,
) from exc
````
</div>
</details>
</div>
<small><i>Code Review Run #bd8776</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
##########
superset/views/datasource/utils.py:
##########
@@ -176,15 +178,88 @@ def get_samples( # pylint: disable=too-many-arguments
if count_star_data.get("status") == QueryStatus.FAILED:
raise DatasetSamplesFailedError(count_star_data.get("error"))
- sample_data = samples_instance.get_payload()["queries"][0]
+ engine_spec = datasource.database.db_engine_spec
+ row_offset = limit_clause["row_offset"]
+ row_limit = limit_clause["row_limit"]
- if sample_data.get("status") == QueryStatus.FAILED:
- QueryCacheManager.delete(count_star_data.get("cache_key"), CacheRegion.DATA)
- raise DatasetSamplesFailedError(sample_data.get("error"))
+ if not engine_spec.supports_offset and row_offset > 0:
+ try:
+ sample_data = _fetch_samples_via_cursor(
+ datasource=datasource,
+ samples_instance=samples_instance,
+ count_star_data=count_star_data,
+ page_index=row_offset // row_limit,
+ page_size=row_limit,
+ )
+ except DatasetSamplesFailedError:
+ raise
+ except Exception as exc:
+ QueryCacheManager.delete(
+ count_star_data.get("cache_key"), CacheRegion.DATA
+ )
+ logger.exception("Cursor-based samples pagination failed")
+ raise DatasetSamplesFailedError(
+ "Failed to fetch samples via cursor pagination"
+ ) from exc
+ else:
+ sample_data = samples_instance.get_payload()["queries"][0]
+ if sample_data.get("status") == QueryStatus.FAILED:
+ QueryCacheManager.delete(
+ count_star_data.get("cache_key"), CacheRegion.DATA
+ )
+ raise DatasetSamplesFailedError(sample_data.get("error") or "")
sample_data["page"] = page
sample_data["per_page"] = per_page
sample_data["total_count"] = count_star_data["data"][0]["COUNT(*)"]
return sample_data
except (IndexError, KeyError) as exc:
raise DatasetSamplesFailedError from exc
+
+
+def _fetch_samples_via_cursor(
+ datasource: Datasource,
+ samples_instance: QueryContext,
+ count_star_data: dict[str, Any],
+ page_index: int,
+ page_size: int,
+) -> dict[str, Any]:
+ """
+ Fetch a single page of samples via engine-spec cursor pagination.
+
+ Used when ``datasource.database.db_engine_spec.supports_offset`` is
+ False and a non-first page is requested. Compiles the same SQL Superset
+ would run for the normal samples payload — without executing it — and
+ delegates cursor iteration to the engine spec. The engine spec is
+ responsible for stripping any trailing ``LIMIT`` from the SQL so the
+ cursor is not capped to a single page.
+
+ ``coltypes`` are inferred from the returned rows with
+ ``extract_dataframe_dtypes``, the same function the non-cursor path
+ uses to type page 1 — it works off the actual returned values, not
+ ``cursor.description``, so no ES-type-to-coltype translator is needed
+ and no extra query is required to source them.
+ """
+ query_obj = samples_instance.queries[0]
+ sql = samples_instance.datasource.get_query_str(query_obj.to_dict())
+ if not sql:
+ QueryCacheManager.delete(count_star_data.get("cache_key"), CacheRegion.DATA)
+ raise DatasetSamplesFailedError("Empty samples query")
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Assign exception message to variable</b></div>
<div id="fix">
TRY003 and EM101 violations at line 247: Similar to earlier issues, exception messages should be assigned to variables before raising exceptions.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
query_obj = samples_instance.queries[0]
sql = samples_instance.datasource.get_query_str(query_obj.to_dict())
if not sql:
message = "Empty samples query"
QueryCacheManager.delete(count_star_data.get("cache_key"), CacheRegion.DATA)
raise DatasetSamplesFailedError(message)
````
</div>
</details>
</div>
<small><i>Code Review Run #bd8776</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(drill-detail): paginate Elasticsearch samples via engine cursor [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39509:
URL: https://github.com/apache/superset/pull/39509#issuecomment-4928093548
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39509?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `26.02740%` with `54 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.79%. Comparing base ([`afc7dbe`](https://app.codecov.io/gh/apache/superset/commit/afc7dbeff2346220ce24b45cbb8dddb8ec4a1277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`33f9112`](https://app.codecov.io/gh/apache/superset/commit/33f911240de18911ce3728af298912b0f4f34330?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39509?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/views/datasource/utils.py](https://app.codecov.io/gh/apache/superset/pull/39509?src=pr&el=tree&filepath=superset%2Fviews%2Fdatasource%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS91dGlscy5weQ==) | 3.33% | [29 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39509?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/db\_engine\_specs/elasticsearch.py](https://app.codecov.io/gh/apache/superset/pull/39509?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Felasticsearch.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2VsYXN0aWNzZWFyY2gucHk=) | 36.84% | [24 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39509?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/models/helpers.py](https://app.codecov.io/gh/apache/superset/pull/39509?src=pr&el=tree&filepath=superset%2Fmodels%2Fhelpers.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/39509?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (afc7dbe) and HEAD (33f9112). Click for more details.
>
> <details><summary>HEAD has 6 uploads less than BASE</summary>
>
>| Flag | BASE (afc7dbe) | HEAD (33f9112) |
>|------|------|------|
>|python|8|2|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39509 +/- ##
==========================================
- Coverage 64.60% 55.79% -8.82%
==========================================
Files 2712 2712
Lines 151168 151235 +67
Branches 34769 34774 +5
==========================================
- Hits 97665 84374 -13291
- Misses 51680 66103 +14423
+ Partials 1823 758 -1065
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39509/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/39509/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.03% <26.02%> (-0.02%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39509/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/39509/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/39509/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.54% <26.02%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39509/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.60% <26.02%> (-18.37%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39509/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39509/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/39509?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