codeant-ai-for-open-source[bot] commented on PR #36350: URL: https://github.com/apache/superset/pull/36350#issuecomment-3604122979
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36350/files#diff-462c756528595a2be4a0a3c3063077f984384c7858ecdb5bd3e9603163c3678bR748-R763'><strong>Transaction Behavior</strong></a><br>The new logic that persists the cancel query id calls `_db.session.commit()` inside `_execute_sql_with_mutation_and_logging`, which is a low-level helper used for executing SQL. Committing the global session here may unexpectedly commit unrelated pending objects and change transaction boundaries compared to previous behavior. It would be good to confirm this is safe for all call sites and consistent with Superset's transaction management model.<br> - [ ] <a href='https://github.com/apache/superset/pull/36350/files#diff-d3bf055ecf6a74aa0acbb0650d176b6c251aea7796543f77a2df3fd8b7e4c4b4R120-R247'><strong>Possible Bug</strong></a><br>In `get_df_payload`, `query_model` is only defined inside the `if query_obj and cache_key and not cache.is_loaded:` block (and within a nested `try`), but it is later referenced unconditionally in the `if query_model is not None:` block when building the payload. If the cache is already loaded or an exception is raised before `query_model` is assigned, this will trigger an `UnboundLocalError`. The reviewer should verify and fix the initialization/lifetime of `query_model` so it is always defined before use.<br> - [ ] <a href='https://github.com/apache/superset/pull/36350/files#diff-d3bf055ecf6a74aa0acbb0650d176b6c251aea7796543f77a2df3fd8b7e4c4b4R129-R168'><strong>Stability Issue</strong></a><br>In the best-effort creation of a `SqlLabQuery` row, a broad `except Exception` around `_db.session.commit()` only logs the error and sets `query_model = None` without rolling back the SQLAlchemy session. If the commit fails, the session can remain in a bad state and later metadata operations during the same request may fail. It would be safer to rollback the session and/or narrow the caught exception type.<br> - [ ] <a href='https://github.com/apache/superset/pull/36350/files#diff-d3bf055ecf6a74aa0acbb0650d176b6c251aea7796543f77a2df3fd8b7e4c4b4R120-R260'><strong>Possible Bug</strong></a><br>`query_model` is defined only inside the `if query_obj and cache_key and not cache.is_loaded:` branch of `get_df_payload`, but is later referenced unconditionally when constructing the payload. On cache hits (or when `query_obj`/`cache_key` is falsy) this can raise an `UnboundLocalError` when evaluating `if query_model is not None:`. This control-flow should be adjusted so that `query_model` is always defined before use or the client-id injection is guarded appropriately.<br> - [ ] <a href='https://github.com/apache/superset/pull/36350/files#diff-d056bcdd6bd2328f15cbc2f401653baee1a9da44977a043aa22723187255f838R57-R95'><strong>Performance Issue</strong></a><br>Including `client_id` in `cache_values` will likely make the chart data cache effectively per-client, reducing cache hit rates and increasing database load. Since `cache_values` are typically used to derive the cache key, this change may unintentionally alter caching semantics for all chart queries. It should be validated whether `client_id` truly needs to participate in the cache key, or if it can be conveyed via a different mechanism that does not fragment the cache.<br> </td></tr> </table> -- 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]
