rusackas commented on PR #34513: URL: https://github.com/apache/superset/pull/34513#issuecomment-3857246154
## Addressing Review Feedback I've gone through all the review comments and here's how each concern has been addressed: ### @korbit-ai - Incomplete INTERVAL type handling ✅ **Addressed** - The mutator now handles multiple formats: - `timedelta` objects → milliseconds (primary psycopg2 case) - `int`/`float` values → converted to milliseconds - `None` → 0 (safe for aggregations) - Strings → pass through unchanged ### @giftig - Display not user-friendly ✅ **Addressed** - Changed from seconds to **milliseconds**, which enables the built-in `DURATION` formatter. Users can now see `1d 2h 30m 45s` instead of raw numbers by setting Number Format to `DURATION`. ### @giftig - Missing documentation on why mutator is needed ✅ **Addressed** - Added clear comments explaining: - Why normalization is needed (psycopg2 returns timedelta) - Why milliseconds (DURATION formatter compatibility) - How string representations are handled ### @codeant-ai-for-open-source nitpicks | Concern | Response | |---------|----------| | NULL handling (None→0) | Kept as-is - charts need numeric values for aggregations | | Regex specificity (`^interval`) | Consistent with 50+ similar patterns in codebase | | Type matching robustness | Base class uses `.get()` for lookup - not an issue | | KeyError in test | Tests fail clearly on missing key - minor style preference | ### Additional improvements - Fixed type annotation to match MySQL pattern (`types.TypeEngine`) - Rebased on latest master The PR is ready for another review. Thanks to everyone for the thorough feedback! -- 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]
