codeant-ai-for-open-source[bot] commented on PR #36599: URL: https://github.com/apache/superset/pull/36599#issuecomment-3648411359
## 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/36599/files#diff-2290642a0ed1470a3bec264fa6d7870e97c6e103b425d3cf195ccbb97af3f622R159-R166'><strong>Multi-statement handling</strong></a><br>The code appends `LIMIT` to the whole SQL string. For multi-statement payloads (e.g. `WITH ...; INSERT ...`) or SQL with trailing comments, this may add LIMIT to the wrong statement. Consider detecting and modifying only the final SELECT statement.<br> - [ ] <a href='https://github.com/apache/superset/pull/36599/files#diff-2290642a0ed1470a3bec264fa6d7870e97c6e103b425d3cf195ccbb97af3f622R159-R166'><strong>Coarse LIMIT detection</strong></a><br>_apply_limit uses a simple substring check ("limit" in sql_lower) to decide whether a LIMIT already exists. This will skip adding an outer LIMIT if any inner CTE or comment contains the word "limit". That can leave outer SELECTs unbounded in cases where only inner queries include LIMIT and the outer query lacks one.<br> - [ ] <a href='https://github.com/apache/superset/pull/36599/files#diff-2290642a0ed1470a3bec264fa6d7870e97c6e103b425d3cf195ccbb97af3f622R159-R166'><strong>LIMIT detection</strong></a><br>`_apply_limit()` checks for the substring `"limit"` anywhere in `sql_lower`, which can produce false positives (matching "unlimited" or identifiers) and false negatives if LIMIT appears in a different statement. Use a word-boundary-aware regex to detect a standalone LIMIT clause.<br> - [ ] <a href='https://github.com/apache/superset/pull/36599/files#diff-2290642a0ed1470a3bec264fa6d7870e97c6e103b425d3cf195ccbb97af3f622R210-R215'><strong>CTE detection</strong></a><br>`_is_select_query()` treats any SQL that starts with `WITH` as a SELECT/CTE without verifying that a SELECT actually appears inside the statement or accounting for variants like `WITH RECURSIVE` or leading comments. This can misclassify non-CTE statements, or miss CTEs preceded by comments or parentheses.<br> - [ ] <a href='https://github.com/apache/superset/pull/36599/files#diff-2290642a0ed1470a3bec264fa6d7870e97c6e103b425d3cf195ccbb97af3f622R210-R215'><strong>Leading comments not handled</strong></a><br>_is_select_query strips whitespace but does not account for leading SQL comments (e.g., "-- ..." or "/* ... */"). If a query begins with a comment followed by a WITH/SELECT, startswith checks will fail and the query may be misclassified (not treated as a SELECT-like query).<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]
