codeant-ai-for-open-source[bot] commented on PR #36777: URL: https://github.com/apache/superset/pull/36777#issuecomment-3678062614
## 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/36777/files#diff-595987046ff00d1af428f8b7360bdd1372bef133a42e58ff8ede585ea98cd254R3082-R3120'><strong>Possible Bug</strong></a><br>In the default non-boolean branch, the code checks `if eq:` before constructing the IN clause with `eq_without_none`. This should check `eq_without_none`; as written, when `eq` contains only `None` values the code will call `.in_([])` leading to an empty IN list (potentially producing invalid SQL or runtime errors). Validate and adjust checks to guard against empty lists passed to `.in_()`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36777/files#diff-595987046ff00d1af428f8b7360bdd1372bef133a42e58ff8ede585ea98cd254R3082-R3120'><strong>Code duplication / logic clarity</strong></a><br>`eq_without_none` is recomputed in multiple places and the boolean-handling and default handling branches duplicate similar null/IN logic. This duplication increases maintenance burden and makes correctness harder to verify. Consider computing `eq_without_none` once and centralizing the null+IN handling for both boolean and non-boolean paths.<br> - [ ] <a href='https://github.com/apache/superset/pull/36777/files#diff-4de7ed14a617a2aeae101734c998bbd3c08a41b2100f9a032b82cdb87ee5293cR245-R292'><strong>NULL handling</strong></a><br>Values that are None are currently skipped; when all inputs are None the method returns a constant false condition. An IN(...) containing NULL normally requires IS NULL semantics or a combination (e.g., col IN (...) OR col IS NULL). Skipping None may change semantics.<br> - [ ] <a href='https://github.com/apache/superset/pull/36777/files#diff-4de7ed14a617a2aeae101734c998bbd3c08a41b2100f9a032b82cdb87ee5293cR245-R292'><strong>Loose numeric conversion</strong></a><br>The new conversion treats any non-zero int/float as True via bool(val). That will map values like 2, -1, 3.14 to True which is likely unintended — the original intent seems to be to map only 0/1 (or strict boolean-like inputs). This can produce incorrect predicate semantics.<br> - [ ] <a href='https://github.com/apache/superset/pull/36777/files#diff-4de7ed14a617a2aeae101734c998bbd3c08a41b2100f9a032b82cdb87ee5293cR245-R292'><strong>Duplicate / tautology conditions</strong></a><br>boolean_values is not deduplicated; mixed inputs including both True and False will generate a tautology (col == True OR col == False). This is correct but inefficient and may hide logic issues. Consider deduplication and explicit detection of full-domain selection.<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]
