s1ny1998 commented on PR #43000: URL: https://github.com/apache/superset/pull/43000#issuecomment-5490658535
@rusackas thanks — all three items are done and pushed. **Rebased.** The branch is now two clean commits on top of current `master` (the merge commits are gone, so this was a force-push). One conflict, in `test_clickhouse.py`: `master`'s new multivalue/`Array` tests landed in the same place as the upload tests, and both sides were kept. Worth noting that `master`'s new `^Array\(` mapping is ordered ahead of the `.*Float.*` entry this PR adds, so `Array(Float64)` still classifies as `MULTI_VALUE`. **`ruff-format`.** That was the only failing hook — a three-line wrap of the `client.insert_df(...)` call that should have been one line. `pre-commit run` is clean now across the board: `ruff-format`, `ruff`, `mypy (main)`, `pylint` (10.00/10) and the db-engine-spec metadata check. (The `python-dependency-liccheck` failure on the previous run was a cancellation, not a real failure — it should clear on this run.) **Bito's two suggestions.** I looked at both and don't think either should be actioned: 1. *Duplicate type mappings between `clickhouse.py` and `databend.py`.* The resemblance is structural rather than real — the regex sets differ (Databend matches `Double\d+`, ClickHouse matches `Int\d+`) and `convert_dttm` emits entirely different SQL (`toDate`/`toDateTime` vs `to_date`/`TO_TIMESTAMP`). Factoring these into a shared base would couple two independent engine specs to each other, and the block is almost entirely pre-existing code — this PR adds five lines to it. 2. *Duplicate test data tuples between `test_clickhouse.py` and `test_databend.py`.* Same reasoning: table-driven type expectations for two different engines, and mostly pre-existing. A shared fixture would make each suite's expectations harder to read and would tie one engine's test data to the other's. Happy to revisit if you'd rather see them addressed. Test status: 140 passing in `tests/unit_tests/db_engine_specs/test_clickhouse.py`, plus the full `db_engine_specs` directory and `upload_command_test.py`. I also re-checked the type mapping against the real library rather than the fakes — every type `_clickhouse_column_type` emits parses through `clickhouse_connect.datatypes.registry.get_from_name`, and `get_column_spec` classifies all of them correctly (wrapped floats `NUMERIC`, `Nullable(DateTime64(6))` `TEMPORAL` with `is_dttm=True`). @joe-clickhouse — if you have a moment for another pass, the substantive changes from your review have been in since the previous revision; this round is rebase and lint only. -- 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]
