codeant-ai-for-open-source[bot] commented on PR #34513: URL: https://github.com/apache/superset/pull/34513#issuecomment-3658246396
## 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/34513/files#diff-8cec2642bdfebf02492bfb132c78133c229f409661a4e56c8ea96868a1afd63fR249-R263'><strong>NULL handling</strong></a><br>The INTERVAL mutator converts None to 0. Treating missing/NULL interval values as 0 may change semantics (missing vs zero). Confirm this is the intended behaviour for charts and downstream consumers; consider returning None/NaN or leaving value unchanged instead.<br> - [ ] <a href='https://github.com/apache/superset/pull/34513/files#diff-8cec2642bdfebf02492bfb132c78133c229f409661a4e56c8ea96868a1afd63fR24-R27'><strong>Type matching robustness</strong></a><br>The column mutator mapping uses the `INTERVAL` SQLA type class as the key. fetch_data builds the lookup using type(instance) of the SQLA column type. Verify `get_sqla_column_type(...)` returns an instance whose type matches the imported `INTERVAL` class across all Postgres dialects/drivers used in tests (e.g. ensure no dialect/driver returns a different class that would prevent the mutator from matching).<br> - [ ] <a href='https://github.com/apache/superset/pull/34513/files#diff-8cec2642bdfebf02492bfb132c78133c229f409661a4e56c8ea96868a1afd63fR242-R246'><strong>Regex specificity</strong></a><br>The regex used to detect interval types is `^interval`. This may match unexpected variations (e.g. `interval_custom`). Consider using a word boundary to avoid accidental matches and ensure only the proper `interval` base type is mapped.<br> - [ ] <a href='https://github.com/apache/superset/pull/34513/files#diff-3c1a73a96f73b26cced43bda74ec8f421dadb3bb62c1e3d87c1ee7ffea11116bR289-R289'><strong>Possible KeyError</strong></a><br>The test looks up the mutator with direct indexing `spec.column_type_mutators[INTERVAL]`. If the mapping is not registered correctly this will raise a KeyError and produce a less actionable failure. Consider retrieving with `.get()` and asserting presence for clearer failures.<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]
