codeant-ai-for-open-source[bot] commented on code in PR #43518:
URL: https://github.com/apache/superset/pull/43518#discussion_r3854655163
##########
superset/datasource/api.py:
##########
@@ -225,11 +242,15 @@ def get_column_values(
timeout = datasource.cache_timeout or app.config.get(
"CACHE_DEFAULT_TIMEOUT", 300
)
+ if search:
+ # Every distinct search term is its own key, so a few users typing
+ # would otherwise pin one entry per keystroke for the full timeout.
+ timeout = min(timeout, SEARCH_CACHE_TIMEOUT)
cache_manager.data_cache.set(cache_key, payload, timeout=timeout)
logger.debug(
"column-values cache MISS: uid=%s col=%s", datasource.uid,
column_name
)
- response = self.response(200, result=payload)
+ response = self.response(200, result=payload, limit=row_limit)
Review Comment:
**Suggestion:** The endpoint now returns a `limit` property, but the OpenAPI
response schema still documents only `result`. API consumers and generated
clients will therefore receive a response field that is absent from the
declared contract, preventing them from reliably discovering or typing the
truncation metadata. Add `limit` to the documented 200 response schema. [api
mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Generated clients omit the runtime `limit` field.
- โ ๏ธ API consumers cannot rely on truncation metadata.
- โ ๏ธ OpenAPI documentation disagrees with endpoint responses.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1eefcbd358ca43b09e8986c55e4fcb91&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=1eefcbd358ca43b09e8986c55e4fcb91&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/datasource/api.py
**Line:** 253:253
**Comment:**
*Api Mismatch: The endpoint now returns a `limit` property, but the
OpenAPI response schema still documents only `result`. API consumers and
generated clients will therefore receive a response field that is absent from
the declared contract, preventing them from reliably discovering or typing the
truncation metadata. Add `limit` to the documented 200 response schema.
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%2F43518&comment_hash=af7dd2476ecbc2e97305aa554ec206f52072cac8124b4272f27153be13d6aaa7&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43518&comment_hash=af7dd2476ecbc2e97305aa554ec206f52072cac8124b4272f27153be13d6aaa7&reaction=dislike'>๐</a>
##########
superset-frontend/src/filters/components/Select/buildQuery.ts:
##########
@@ -54,14 +54,21 @@ const buildQuery:
BuildQuery<PluginFilterSelectQueryFormData> = (
}
const sortColumns = sortMetric ? [sortMetric] : columns;
+ // Sorting by the searched column makes the engine scan and sort every
+ // match before applying the row limit, which is the dominant cost of
+ // search-as-you-type on a high-cardinality column. The dropdown re-sorts
+ // the returned page client-side, so the server sort buys nothing here. A
+ // sort metric is different: it selects *which* rows come back, so it has
+ // to stay.
+ const skipOrderBy = !!search && !sortMetric;
const query: QueryObject[] = [
{
...baseQueryObject,
columns,
metrics: sortMetric ? [sortMetric] : [],
filters: filters.concat(extraFilters),
orderby:
- sortMetric || sortAscending !== undefined
+ !skipOrderBy && (sortMetric || sortAscending !== undefined)
? sortColumns.map(column => [column, !!sortAscending])
: [],
Review Comment:
**Suggestion:** When a search is active without `sortMetric`, removing
`ORDER BY` makes the limited result page nondeterministic. The dropdown can
reorder the rows it receives, but it cannot restore the previously
deterministic selection of which rows enter the page, so repeated searches can
omit different matching values and produce unstable results. Preserve a
deterministic ordering or use a database-side strategy that avoids the full
sort without changing page membership. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ ๏ธ Search results can omit different matching values.
- โ ๏ธ Select filter suggestions become unstable across requests.
- โ ๏ธ Client sorting cannot restore excluded database rows.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=321fbf2a937c446c987d8bb111aa08f3&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=321fbf2a937c446c987d8bb111aa08f3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/filters/components/Select/buildQuery.ts
**Line:** 63:73
**Comment:**
*Api Mismatch: When a search is active without `sortMetric`, removing
`ORDER BY` makes the limited result page nondeterministic. The dropdown can
reorder the rows it receives, but it cannot restore the previously
deterministic selection of which rows enter the page, so repeated searches can
omit different matching values and produce unstable results. Preserve a
deterministic ordering or use a database-side strategy that avoids the full
sort without changing page membership.
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%2F43518&comment_hash=a796131c278bf724ca83a4f97700f10267198c32b9d3fc31e850562e3483a6ff&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43518&comment_hash=a796131c278bf724ca83a4f97700f10267198c32b9d3fc31e850562e3483a6ff&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: [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]
