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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
