rusackas commented on PR #40180: URL: https://github.com/apache/superset/pull/40180#issuecomment-4884225227
@Elengar this is a well-built fix... I traced `filter_values_handler` against current master and the new logic is strictly safer (guards `convert_dttm` returning None where master would crash in `literal_column`, handles signed epochs, catches overflow), and the frontend-plus-backend belt-and-suspenders means adhoc-column cross-filters that still emit strings get coerced server-side anyway. Test coverage across BigQuery/ClickHouse/Postgres is thorough. Two things before we can merge: GitHub shows conflicts, but it's just `tests/unit_tests/models/helpers_test.py`... both master and this branch appended tests at the end of the file, so the rebase should be a quick both-sides keep. The `role="columnheader button"` removal in `TableChart.tsx` looks unrelated to temporal coercion... was that needed for the new test, or a drive-by? If it's a real a11y fix it probably deserves its own PR; if the test needed it, a comment would help. One observation, not blocking: a temporal column whose values are legitimately digit strings in some other encoding (yyyymmdd style) would now be read as epoch ms... those cases already failed at the database on the affected engines, so this is still a net improvement, just worth remembering if a report ever comes in. Happy to re-review after the rebase. Thanks! -- 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]
