rusackas opened a new pull request, #44772:
URL: https://github.com/apache/superset/pull/44772
### SUMMARY
Follow-up to #44500 (adds `epoch_us` support). Bito flagged during that
review that `get_timestamp_expr` (`superset/db_engine_specs/base.py`) hardcoded
a `{"epoch_s": ..., "epoch_ms": ..., "epoch_us": ...}` dict re-listing the same
keys `EPOCH_FORMATS` already declares in `superset/constants.py`. A future
`EPOCH_FORMATS` entry (say `epoch_ns`) would pass the `pdf in EPOCH_FORMATS`
guard but `KeyError` here instead of failing cleanly, since nothing kept the
two collections in sync. Non-blocking on #44500, tracked as this fast-follow
instead.
Every epoch method follows `f"{pdf}_to_dttm"` except `epoch_s`, whose method
predates `epoch_ms`/`epoch_us` and kept the shorter legacy name
`epoch_to_dttm`. Resolving the method name via `getattr()`, with a one-entry
alias dict for that sole exception, removes the duplicated key list entirely —
a new `EPOCH_FORMATS` entry following the naming convention needs no second
edit here.
Also collapses two triplicated `epoch_s`/`epoch_ms`/`epoch_us` test bodies
(`helpers_test.py`, `test_core.py`) into parametrized tests, matching the
pattern the other epoch tests #44500 added already use.
**On the lint-rule idea:** looked into whether a rule could catch "these two
collections must stay in sync" generally. Not realistically buildable without
heavy false-positive risk — there's no syntactic signal tying two arbitrary
literal collections together, only human judgment about which ones are
"supposed to" mirror each other. Checked the repo's existing custom pylint
plugin (`superset/extensions/pylint.py`, two narrow single-node checkers) and
ruff's rule set; neither has anything applicable, and this specific fix removes
the duplication rather than just detecting it, so there's nothing left here to
lint for. If this class of bug recurs elsewhere, a plain sync-assertion test
(the pattern `test_dataset_dao.py`'s `EPOCH_FORMATS`-parametrized test already
uses) is the standard, zero-false-positive-risk way this codebase already
reaches for.
Also swept the rest of `superset/db_engine_specs/` for the same anti-pattern
(a local dict/tuple re-listing keys from a canonical constant elsewhere); found
no other instance.
### TESTING INSTRUCTIONS
All existing epoch-related unit tests still pass (45 tests across
`test_base.py`, `test_postgres.py`, `core_test.py`, `helpers_test.py`,
`test_core.py`, `schema_tests.py`, `test_dataset_dao.py`), plus the full
`db_engine_specs`/model/dataset unit suite (2090 passed). `mypy`, `ruff`, and
`pylint` all pass on the changed files.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
