danfye commented on PR #45858:
URL: https://github.com/apache/superset/pull/45858#issuecomment-6577998152
Independent verification on macOS (Apple Silicon, Python 3.12, env from
`requirements/development.txt`) at head `6a46056c`:
```
pytest tests/unit_tests/db_engine_specs/test_oracle.py
tests/unit_tests/db_engine_specs/test_impala.py
→ 85 passed
pytest tests/unit_tests/db_engine_specs/ (this head)
→ 2401 passed, 13 skipped, 3 xfailed
pytest tests/unit_tests/db_engine_specs/ (clean origin/master,
same interpreter)
→ 2400 passed, 13 skipped, 3 xfailed → +1 test collected, no
regressions
ruff check superset/db_engine_specs/{oracle,impala}.py
tests/unit_tests/db_engine_specs/test_{oracle,impala}.py
→ All checks passed
```
Nothing to change for CI purposes — it is green here too, and it matches how
the other engine specs are tested.
One thing that may be worth recording before this merges, because CI cannot
prove it: both new tests assert the **emitted string** (`TRUNC(ts, 'D')`,
`TRUNC(CAST(ts as DATE), 'IW')`), which is exactly what the suite does for
every other spec, so fine — but the Impala docstring claims `'D'` "always
truncates to Monday", and that is a property of Impala, not of the string. I
looked for the documented unit list for Impala's `TRUNC` on a date/timestamp
and could not reach it (the Apache Impala `impala_dates` page 404s for me and a
search turned up nothing citable), so as far as a reader can tell from this PR,
the only evidence for `'D'` is the live verification in the thread.
Since a wrong guess there silently regroups every Impala week bucket for
every user, it would help to paste the actual query and output into the PR
description — Impala version, and what `TRUNC(<date>, 'D')` returns for a date
whose week straddles a year boundary (say 2025-12-31 and 2026-01-01), which is
the same class of edge case this PR exists to fix on the Oracle side. If that
came out Monday for both, then the docstring is earned and the next person does
not have to re-derive it. And if there is any session/locale dependence to it,
`'WEEK'`-style unit would be worth a look as the documented alternative.
Happy to re-run the suite on any follow-up push.
--
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]
