You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "gabbyasuncion (via GitHub)" <gi...@apache.org> on 2026/04/11 01:20:32 UTC
[PR] introduce bugs [superset]
gabbyasuncion opened a new pull request, #39288:
URL: https://github.com/apache/superset/pull/39288
Bug 1 — Off-by-one in column deduplication
superset/result_set.py:57 — Changed seen[s_fixed_case] = 0 to = 1. The dedup counter now starts at 1 instead of 0, so the first duplicate column gets suffix __2 instead of __1.
Result sets with duplicate column names produce col, col__2, col__3 instead of the expected col, col__1, col__2, breaking any downstream code that relies on the __1 naming
convention.
Bug 2 — Force-refresh no longer bypasses cache
superset/common/utils/query_cache_manager.py:164 — Removed or force_query from the early-return guard in QueryCacheManager.get(). When a user explicitly requests a forced refresh
(force_query=True), the cache is still consulted and stale data is returned. The bug is intermittent — it only manifests when a cached value exists — making it difficult to reproduce
consistently.
Bug 3 — Logic inversion in dashboard favorite
superset/daos/dashboard.py:564 — Changed if dashboard.id not in ids: to if dashboard.id in ids: in add_favorite(). The guard that prevents duplicate FavStar entries is inverted: a
record is only inserted when one already exists, which is never true on a first add. Users cannot favorite any dashboard. The UI appears to respond normally (no error is raised), but
the favorite never persists.
Bug 4 — Exception chain broken in SQL Lab query save
superset/commands/sql_lab/execute.py:203 — Removed from ex from the raise SqlLabException(...) in _save_new_query(). The original SQLAlchemyError — including its DB-level message,
SQLSTATE code, and traceback — is no longer chained. Production error reports show only the generic wrapper exception, making root-cause analysis significantly harder.
Bug 5 — Incorrect limit factor at boundary condition
superset/commands/sql_lab/execute.py:233 — Changed limits[0] > limits[1] to limits[0] >= limits[1] in _set_query_limit(). When the SQL-embedded limit exactly equals the UI dropdown
limit, the code should assign LimitingFactor.QUERY_AND_DROPDOWN. Instead it assigns LimitingFactor.DROPDOWN, misattributing which limit is controlling the result set and showing
users incorrect explanatory text.
Bug 6 — Array off-by-one in SQL Lab tab clone
superset-frontend/src/SqlLab/reducers/sqlLab.ts:121 — Changed state.tabHistory[state.tabHistory.length - 1] to state.tabHistory[state.tabHistory.length]. Accessing an array at its
own length always returns undefined in JavaScript, so queryEditor is never found. Spreading undefined produces an empty object, and the cloned tab is created without any inherited
properties — name, query, database, schema — silently opening a blank editor instead of a copy of the active one.
--
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] introduce bugs [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 #39288:
URL: https://github.com/apache/superset/pull/39288#discussion_r3067310877
##########
superset/daos/dashboard.py:
##########
@@ -561,7 +561,7 @@ def update_colors_config(
@staticmethod
def add_favorite(dashboard: Dashboard) -> None:
ids = DashboardDAO.favorited_ids([dashboard])
- if dashboard.id not in ids:
+ if dashboard.id in ids:
Review Comment:
**Suggestion:** The favorite insertion guard is inverted, so a new favorite is only added when one already exists. This prevents first-time favoriting from ever persisting; switch the condition back to insert only when the dashboard is not already favorited. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard favorites endpoint /api/v1/dashboard/<pk>/favorites broken.
- ❌ Users cannot favorite new dashboards successfully.
- ⚠️ UI indicates success though favorite not persisted.
```
</details>
```suggestion
if dashboard.id not in ids:
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR code deployed and log in as a user who has not yet
favorited a particular dashboard `pk`.
2. Trigger the favorites status check endpoint `GET /api/v1/dashboard/favorite_status/`
(implemented in `superset/dashboards/api.py:1725-1764`) with `rison` containing `[pk]`;
this calls `DashboardDAO.favorited_ids(dashboards)` at
`superset/dashboards/api.py:1725-1764`, which in turn uses `DashboardDAO.favorited_ids` at
`superset/daos/dashboard.py:338` and returns a list of dashboard IDs favorited by the
current user (which does not include `pk` yet).
3. Call the favorite creation endpoint `POST /api/v1/dashboard/<pk>/favorites` handled by
`DashboardsRestApi.add_favorite` at `superset/dashboards/api.py:1747`, which executes
`AddFavoriteDashboardCommand(pk).run()` at `superset/commands/dashboard/fave.py:31-39`;
`run()` then calls `DashboardDAO.add_favorite(self._dashboard)` at
`superset/commands/dashboard/fave.py:36-39`.
4. Inside `DashboardDAO.add_favorite` at `superset/daos/dashboard.py:562-565`, the code
computes `ids = DashboardDAO.favorited_ids([dashboard])`; since this is a first-time
favorite, `dashboard.id` is not in `ids`, so the inverted guard `if dashboard.id in ids:`
at line 564 evaluates to `False` and the `FavStar` insert on lines 565-572 is skipped,
resulting in no row added despite the endpoint returning `200 OK`.
5. Re-check favorite status by calling `GET /api/v1/dashboard/favorite_status/` again with
`pk`; the handler in `superset/dashboards/api.py:1725-1764` still receives
`favorited_dashboard_ids` that do not contain `pk`, so the response shows `"value": false`
for that dashboard, demonstrating that the favorite never persisted even though the POST
endpoint reported success.
6. Compare with chart favorites logic in `superset/daos/chart.py:77-80`, where
`ChartDAO.add_favorite` correctly uses `if chart.id not in ids:` before inserting a
`FavStar`, confirming that the dashboard condition `if dashboard.id in ids:` is the
inverted variant of the established pattern and causes first-time favorites to be dropped.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/dashboard.py
**Line:** 564:564
**Comment:**
*Logic Error: The favorite insertion guard is inverted, so a new favorite is only added when one already exists. This prevents first-time favoriting from ever persisting; switch the condition back to insert only when the dashboard is not already favorited.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=84eb61fb4d246f5d6556bbf8ab16363d2a6acc2f1508da87fe7dbaa0a02ff924&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=84eb61fb4d246f5d6556bbf8ab16363d2a6acc2f1508da87fe7dbaa0a02ff924&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] introduce bugs [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39288:
URL: https://github.com/apache/superset/pull/39288#issuecomment-4227584073
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] introduce bugs [superset]
Posted by "gabbyasuncion (via GitHub)" <gi...@apache.org>.
gabbyasuncion closed pull request #39288: introduce bugs
URL: https://github.com/apache/superset/pull/39288
--
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] introduce bugs [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 #39288:
URL: https://github.com/apache/superset/pull/39288#discussion_r3067314192
##########
superset/commands/sql_lab/execute.py:
##########
@@ -200,7 +200,7 @@ def _save_new_query(self, query: Query) -> None:
"The query record was not created as expected",
ex,
"Please contact an administrator for further assistance or try again.",
- ) from ex
+ )
Review Comment:
**Suggestion:** The SQLAlchemy exception is wrapped without exception chaining, so the original traceback and DB-level context are lost. Re-raise with `from ex` to preserve the underlying cause for debugging and error reporting. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SQL Lab execute endpoint hides underlying SQLAlchemyError traceback.
- ⚠️ Production logs lose DB-specific error codes and messages.
- ⚠️ On-call debugging of query-creation failures becomes significantly harder.
```
</details>
```suggestion
) from ex
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Issue a POST to the SQL Lab execute endpoint `/api/v1/sqllab/execute/` as done in
`tests/integration_tests/sql_lab/api_tests.py:360-379`, hitting
`SqlLabRestApi.execute_sql_query` in `superset/sqllab/api.py:498-61`.
2. Inside `execute_sql_query`, a `SqlJsonExecutionContext` is created and
`ExecuteSqlCommand` is instantiated via `_create_sql_json_command` at
`superset/sqllab/api.py:70-80`, then `ExecuteSqlCommand.run()` is called at
`superset/commands/sql_lab/execute.py:94-125`.
3. `run()` calls `_run_sql_json_exec_from_scratch` (`execute.py:142-163`), which in turn
calls `_save_new_query(query)` at `execute.py:145-146` to persist the `Query` ORM object
before executing the SQL.
4. Force `self._query_dao.create(query)` inside `_save_new_query` (`execute.py:194-196`)
to raise `SQLAlchemyError` (e.g. via a failing metadata DB); the `except SQLAlchemyError
as ex:` block at `execute.py:196-203` raises `SqlLabException(..., ex, ...)` without `from
ex`, so when `ExecuteSqlCommand.run` wraps this again in
`SqlLabException(self._execution_context, exception=ex) from ex` at `execute.py:124-125`
and the error is returned by `SqlLabRestApi.execute_sql_query` (`sqllab/api.py:62-68`),
the Python exception chain's root cause is the inner `SqlLabException` rather than the
original `SQLAlchemyError`, and the DB-specific traceback and message are no longer
directly visible in error reports.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/sql_lab/execute.py
**Line:** 203:203
**Comment:**
*Logic Error: The SQLAlchemy exception is wrapped without exception chaining, so the original traceback and DB-level context are lost. Re-raise with `from ex` to preserve the underlying cause for debugging and error reporting.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=d2fe7e517b4ae7cb3bc0e517785215dae20858b7f0f9309b963351eec0fb2a18&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=d2fe7e517b4ae7cb3bc0e517785215dae20858b7f0f9309b963351eec0fb2a18&reaction=dislike'>👎</a>
##########
superset/commands/sql_lab/execute.py:
##########
@@ -230,7 +230,7 @@ def _set_query_limit(self, rendered_query: str) -> None:
db_engine_spec.get_limit_from_sql(rendered_query),
self._execution_context.limit,
]
- if limits[0] is None or limits[0] > limits[1]: # type: ignore
+ if limits[0] is None or limits[0] >= limits[1]: # type: ignore
Review Comment:
**Suggestion:** The limit comparison treats equal limits as dropdown-limited, making the `QUERY_AND_DROPDOWN` branch unreachable. Use a strict `>` comparison so equal query and dropdown limits are correctly classified as both factors. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SQL Lab metadata mislabels combined dropdown and query limits.
- ⚠️ `/api/v1/query/` consumers see inaccurate limiting_factor values.
- ⚠️ Streaming export never encounters QUERY_AND_DROPDOWN limiting state.
```
</details>
```suggestion
if limits[0] is None or limits[0] > limits[1]: # type: ignore
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute a SQL Lab query through POST `/api/v1/sqllab/execute/` (see
`tests/integration_tests/sql_lab/api_tests.py:360-379`) where the SQL text contains `LIMIT
N` and the request's dropdown limit is also `N`, so both limits are equal when parsed.
2. The request hits `SqlLabRestApi.execute_sql_query` in `superset/sqllab/api.py:498-61`,
which builds an `ExecuteSqlCommand` via `_create_sql_json_command` (`sqllab/api.py:70-80`)
and calls `command.run()` at `sqllab/api.py:52-53`.
3. Inside `ExecuteSqlCommand.run`, `_run_sql_json_exec_from_scratch`
(`superset/commands/sql_lab/execute.py:142-160`) renders the query and calls
`_set_query_limit_if_required(rendered_query)` at `execute.py:154-221`, which in turn
invokes `_set_query_limit(rendered_query)` at `execute.py:227-244`.
4. In `_set_query_limit`, `db_engine_spec.get_limit_from_sql(rendered_query)` and
`self._execution_context.limit` populate `limits[0]` and `limits[1]` at
`execute.py:229-232`; when they are equal, the condition `if limits[0] is None or
limits[0] >= limits[1]:` at line 233 is true, so `limiting_factor` is set to
`LimitingFactor.DROPDOWN` and the `else` branch for `LimitingFactor.QUERY_AND_DROPDOWN` at
`execute.py:237-239` is never taken, meaning combined dropdown-and-query limiting is
misclassified in the persisted `Query` and exposed to consumers of `/api/v1/query/` in
`superset/queries/api.py:70-31` and to streaming exports in
`superset/commands/sql_lab/streaming_export_command.py:14-21`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/sql_lab/execute.py
**Line:** 233:233
**Comment:**
*Logic Error: The limit comparison treats equal limits as dropdown-limited, making the `QUERY_AND_DROPDOWN` branch unreachable. Use a strict `>` comparison so equal query and dropdown limits are correctly classified as both factors.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=d2f954ccfcbb25ae3c6ec5528bec1eae0697b6fefc8a271a75a58cdffa10eb2f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=d2f954ccfcbb25ae3c6ec5528bec1eae0697b6fefc8a271a75a58cdffa10eb2f&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] introduce bugs [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 #39288:
URL: https://github.com/apache/superset/pull/39288#discussion_r3067313144
##########
superset/common/utils/query_cache_manager.py:
##########
@@ -161,7 +161,7 @@ def get(
Initialize QueryCacheManager by query-cache key
"""
query_cache = cls()
- if not key or not _cache[region] or force_query:
+ if not key or not _cache[region]:
Review Comment:
**Suggestion:** The cache lookup guard no longer checks `force_query`, so forced refresh requests still read from cache when a key exists. This breaks the force-refresh contract and can return stale results; restore the `force_query` short-circuit in the early return condition. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset samples API `/samples` ignores force refresh flag.
- ❌ Chart queries via QueryContext serve stale data on force.
- ⚠️ Users cannot reliably bypass cache when debugging data.
```
</details>
```suggestion
if not key or not _cache[region] or force_query:
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Send a first request that populates the query cache without forcing a refresh, for
example POST to the samples API implemented in
`superset/views/datasource/views.py:205-239` (`samples()` view) with `force=false` (parsed
by `SamplesRequestSchema` in `superset/views/datasource/schemas.py:90-18`). This calls
`get_samples()` in `superset/views/datasource/utils.py:91-129`, which constructs a
`QueryContext` via `QueryContextFactory().create(..., force=force)` at `utils.py:111-120`.
2. That `QueryContext`'s processor executes `QueryContextProcessor.get_df_payload()` in
`superset/common/query_context_processor.py:82-119`. Since this is the first call for this
key, `force_query` computed at line 93 is `False`, and `QueryCacheManager.get(...)` at
line 94 returns an empty `QueryCacheManager` because
`_cache[CacheRegion.DATA].get(cache_key)` returns `None` (guard at
`query_cache_manager.py:153-165` does not early-return). The code then runs the query and
persists the result to the cache via `cache.set_query_result(...)` at
`query_context_processor.py:52-62`, which internally calls `QueryCacheManager.set()` in
`query_cache_manager.py:74-86`. At this point the cache holds a value for that
`cache_key`.
3. Now send a second request for the same dataset and parameters but with `force=true` to
the same `samples()` endpoint in `superset/views/datasource/views.py:205-239`.
`SamplesRequestSchema` parses `force` (schema field at
`superset/views/datasource/schemas.py:100-106`), `get_samples()` passes this as `force`
into `QueryContextFactory().create(..., force=force)` (`utils.py:111-120`), and the
resulting `QueryContext` has `self.force == True`.
4. In this forced request, `QueryContextProcessor.get_df_payload()` again runs
(`query_context_processor.py:82-119`). It computes `force_query =
self._query_context.force or timeout == CACHE_DISABLED_TIMEOUT` at line 93, so
`force_query` is `True`. It then calls `QueryCacheManager.get(key=cache_key,
region=CacheRegion.DATA, force_query=force_query, force_cached=force_cached)` at line 94.
Because the guard in `QueryCacheManager.get()` (current PR code at
`superset/common/utils/query_cache_manager.py:153-165`) is `if not key or not
_cache[region]:` and does *not* include `or force_query`, the method proceeds to
`_cache[region].get(key)` at line 28, finds the previously cached value, sets
`query_cache.is_loaded = True`, and returns it. The subsequent block in `get_df_payload()`
starting at line 113 (`if query_obj and cache_key and not cache.is_loaded:`) is skipped,
so no fresh query is executed and the response sent back to the client uses stale cached
data despite `force=true`. With the suggested fix `if not key or not _cache[region] or
force_query:`, `QueryCacheManager.get()` would return an unloaded `QueryCacheManager` when
`force_query=True`, causing `get_df_payload()`'s `if ... not cache.is_loaded:` branch to
execute a fresh query as intended.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/utils/query_cache_manager.py
**Line:** 164:164
**Comment:**
*Logic Error: The cache lookup guard no longer checks `force_query`, so forced refresh requests still read from cache when a key exists. This breaks the force-refresh contract and can return stale results; restore the `force_query` short-circuit in the early return condition.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=5ffb20e508e53c3ab511167d257ed3b338ab85f16efad6007507d474538af86f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=5ffb20e508e53c3ab511167d257ed3b338ab85f16efad6007507d474538af86f&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] introduce bugs [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 #39288:
URL: https://github.com/apache/superset/pull/39288#discussion_r3067311523
##########
superset/result_set.py:
##########
@@ -54,7 +54,7 @@ def dedup(l: list[str], suffix: str = "__", case_sensitive: bool = True) -> list
seen[s_fixed_case] += 1
item += suffix + str(seen[s_fixed_case])
else:
- seen[s_fixed_case] = 0
+ seen[s_fixed_case] = 1
Review Comment:
**Suggestion:** Initializing the deduplication counter at `1` causes an off-by-one error in generated duplicate column names, so the first duplicate becomes `__2` instead of `__1`. This breaks the existing naming contract used by tests and downstream column lookups. Initialize first-seen entries to `0` so the next occurrence correctly gets suffix `__1`. [off-by-one]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ SQL Lab `execute_query` returns misnamed duplicate result columns.
- ⚠️ SQLA column introspection returns unexpected `__2` suffixes.
- ⚠️ `Database.load_into_dataframe` uses inconsistent duplicate column names.
- ❌ `TestSupersetResultSet.test_dedup` fails, breaking integration tests.
```
</details>
```suggestion
seen[s_fixed_case] = 0
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the integration test `TestSupersetResultSet.test_dedup` in
`superset/tests/integration_tests/result_set_tests.py:29-37`, which calls `dedup(["foo",
"bar", "foo", "bar", "Foo"])` and `dedup(["foo", "bar", "bar", "bar", "Bar"])` expecting
`__1` and `__2` suffixes on duplicates.
2. The `dedup` function in `superset/result_set.py:36-59` executes; for the first
occurrence of a value (e.g. `"bar"`), the `else` branch at line 57 sets
`seen[s_fixed_case] = 1`, and on the next duplicate at lines 53-55 `seen[s_fixed_case]` is
incremented to `2`, producing `"bar__2"` as the first suffixed duplicate instead of
`"bar__1"`.
3. The assertions in `superset/tests/integration_tests/result_set_tests.py:39-45`
expecting `["foo", "bar", "bar__1", "bar__2", "Bar"]` and in lines 23-29 expecting
`["foo", "bar", "bar__1", "bar__2", "Bar__3"]` fail because the actual output is `["foo",
"bar", "bar__2", "bar__3", "Bar"]` and `["foo", "bar", "bar__2", "bar__3", "Bar__4"]`
respectively.
4. In a running Superset instance, execute a SQL Lab query via `execute_query` in
`superset/sql_lab.py:10-52` that returns duplicate column names (e.g. `SELECT 1 AS col, 2
AS col`), which builds a `SupersetResultSet` at `superset/sql_lab.py:42-44`; in
`SupersetResultSet.__init__` (`superset/result_set.py:134-159`), `column_names =
dedup(normalize_cursor_description_names(cursor_description))` calls the buggy `dedup`, so
the first duplicate column is exposed to the client as `col__2` instead of `col__1`,
breaking the documented naming convention used by tests and any downstream code relying on
`__1` for the first duplicate.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/result_set.py
**Line:** 57:57
**Comment:**
*Off By One: Initializing the deduplication counter at `1` causes an off-by-one error in generated duplicate column names, so the first duplicate becomes `__2` instead of `__1`. This breaks the existing naming contract used by tests and downstream column lookups. Initialize first-seen entries to `0` so the next occurrence correctly gets suffix `__1`.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=9b71f12efa4c5eece83c3602edd1ce10e8c7bcc0379db2e80ba1bc20dc91b174&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39288&comment_hash=9b71f12efa4c5eece83c3602edd1ce10e8c7bcc0379db2e80ba1bc20dc91b174&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] introduce bugs [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 #39288:
URL: https://github.com/apache/superset/pull/39288#discussion_r3067317848
##########
superset/daos/dashboard.py:
##########
@@ -561,7 +561,7 @@ def update_colors_config(
@staticmethod
def add_favorite(dashboard: Dashboard) -> None:
ids = DashboardDAO.favorited_ids([dashboard])
- if dashboard.id not in ids:
+ if dashboard.id in ids:
db.session.add(
Review Comment:
**🔴 Architect Review — CRITICAL**
DashboardDAO.add_favorite uses `if dashboard.id in ids` before inserting a FavStar, so a favorite is only added when it already exists and first-time favorites are never created.
**Suggestion:** Invert the guard to `if dashboard.id not in ids` (matching ChartDAO.add_favorite) so a FavStar is inserted only when no existing favorite is present.
##########
superset/result_set.py:
##########
@@ -54,7 +54,7 @@ def dedup(l: list[str], suffix: str = "__", case_sensitive: bool = True) -> list
seen[s_fixed_case] += 1
item += suffix + str(seen[s_fixed_case])
else:
- seen[s_fixed_case] = 0
+ seen[s_fixed_case] = 1
Review Comment:
**🟠 Architect Review — HIGH**
dedup initializes the counter for first-seen values to 1, causing the first duplicate to be suffixed `__2` instead of `__1`, which contradicts the function's own docstring and existing tests that expect `__1` for the first duplicate.
**Suggestion:** Initialize seen[s_fixed_case] to 0 on first encounter so the first duplicate increments to 1 and receives the `__1` suffix, preserving the established naming convention.
##########
superset-frontend/src/SqlLab/reducers/sqlLab.ts:
##########
@@ -118,7 +118,7 @@ export default function sqlLabReducer(
},
[actions.CLONE_QUERY_TO_NEW_TAB]() {
const queryEditor = state.queryEditors.find(
- qe => qe.id === state.tabHistory[state.tabHistory.length - 1],
+ qe => qe.id === state.tabHistory[state.tabHistory.length],
Review Comment:
**🟠 Architect Review — HIGH**
The CLONE_QUERY_TO_NEW_TAB handler indexes tabHistory with length instead of length - 1, so it almost always fails to find the active queryEditor and builds the progenitor from undefined, breaking the "clone current tab" behavior.
**Suggestion:** Change the lookup to use state.tabHistory[state.tabHistory.length - 1] so the clone source is the last active tab, consistent with other tabHistory usage in this reducer.
##########
superset/common/utils/query_cache_manager.py:
##########
@@ -161,7 +161,7 @@ def get(
Initialize QueryCacheManager by query-cache key
"""
query_cache = cls()
- if not key or not _cache[region] or force_query:
+ if not key or not _cache[region]:
return query_cache
Review Comment:
**🟠 Architect Review — HIGH**
QueryCacheManager.get no longer checks force_query in its early-return guard, so even when force_query=True the method will still read and serve cached results if present, violating the force-refresh contract.
**Suggestion:** Reinstate force_query in the early-return condition (e.g. if not key or not _cache[region] or force_query) so forced executions always bypass cache lookup.
--
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] introduce bugs [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39288:
URL: https://github.com/apache/superset/pull/39288#discussion_r3067374982
##########
superset/commands/sql_lab/execute.py:
##########
@@ -230,7 +230,7 @@ def _set_query_limit(self, rendered_query: str) -> None:
db_engine_spec.get_limit_from_sql(rendered_query),
self._execution_context.limit,
]
- if limits[0] is None or limits[0] > limits[1]: # type: ignore
+ if limits[0] is None or limits[0] >= limits[1]: # type: ignore
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code and behavior change</b></div>
<div id="fix">
Changed > to >= but left else clause unreachable, altering limiting_factor for equal limits from QUERY_AND_DROPDOWN to DROPDOWN. This changes query limit application logic.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- elif limits[1] > limits[0]: # type: ignore
- self._execution_context.query.limiting_factor = LimitingFactor.QUERY
- else: # limits[0] == limits[1]
- self._execution_context.query.limiting_factor = (
- LimitingFactor.QUERY_AND_DROPDOWN
- )
+ elif limits[1] > limits[0]: # type: ignore
+ self._execution_context.query.limiting_factor = LimitingFactor.QUERY
```
</div>
</details>
</div>
<small><i>Code Review Run #330ee5</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/daos/dashboard.py:
##########
@@ -561,7 +561,7 @@ def update_colors_config(
@staticmethod
def add_favorite(dashboard: Dashboard) -> None:
ids = DashboardDAO.favorited_ids([dashboard])
- if dashboard.id not in ids:
+ if dashboard.id in ids:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Logic error in favorite addition condition</b></div>
<div id="fix">
The condition flip from 'not in' to 'in' inverts the logic: it now adds a FavStar only when the dashboard is already favorited, which could lead to incorrect behavior or duplicates if no unique constraint exists. Based on the `favorited_ids` method returning existing favorites and `remove_favorite` assuming single entries, this should check 'not in' to add only if absent.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/daos/dashboard.py
+++ superset/daos/dashboard.py
@@ -564,1 +564,1 @@
- if dashboard.id in ids:
+ if dashboard.id not in ids:
```
</div>
</details>
</div>
<small><i>Code Review Run #330ee5</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-frontend/src/SqlLab/reducers/sqlLab.ts:
##########
@@ -118,7 +118,7 @@ export default function sqlLabReducer(
},
[actions.CLONE_QUERY_TO_NEW_TAB]() {
const queryEditor = state.queryEditors.find(
- qe => qe.id === state.tabHistory[state.tabHistory.length - 1],
+ qe => qe.id === state.tabHistory[state.tabHistory.length],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Array index out of bounds</b></div>
<div id="fix">
This change accesses an out-of-bounds index in the tabHistory array. In JavaScript, array indices are 0-based, so state.tabHistory[state.tabHistory.length] is undefined. All other usages of tabHistory in the codebase correctly use state.tabHistory[state.tabHistory.length - 1] to get the last element, as seen in actions/sqlLab.ts and core/sqlLab/index.ts. This will cause queryEditor to be undefined, leading to runtime errors when cloning queries.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
qe => qe.id === state.tabHistory[state.tabHistory.length - 1],
````
</div>
</details>
</div>
<small><i>Code Review Run #330ee5</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