codeant-ai-for-open-source[bot] commented on PR #37020: URL: https://github.com/apache/superset/pull/37020#issuecomment-3730256147
## 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/37020/files#diff-595987046ff00d1af428f8b7360bdd1372bef133a42e58ff8ede585ea98cd254R1800-R2000'><strong>Remaining Deprecation</strong></a><br>The PR avoids calling SQLAlchemy's and_() with no arguments in the places updated, but there are other code paths that still return an and_() built from an empty conditions list (for example, get_time_filter returns and_(*l) which will call and_() with no args if both start/end are None). Those call sites should be audited and guarded to avoid SADeprecationWarning and potential future breakage.<br> - [ ] <a href='https://github.com/apache/superset/pull/37020/files#diff-595987046ff00d1af428f8b7360bdd1372bef133a42e58ff8ede585ea98cd254R3209-R3214'><strong>Code Duplication</strong></a><br>Near the newly added filter re-application logic there is duplicated branching that concatenates time_filters and where_clause_and and then calls and_(). This duplication appears in multiple places (get_sqla_query and _reapply_query_filters). Consolidating the logic would reduce risk of future regressions and keep behavior consistent.<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]
