s1ny1998 commented on PR #43000: URL: https://github.com/apache/superset/pull/43000#issuecomment-5507451277
@aminghadersohi thank you — this is the most useful review I've had on a PR, and no apology needed for the wait. Both 🔴 findings were real, I reproduced each one, and both are fixed in 2258375. Pushed as a new commit on top of `f53b625` so you can diff against exactly what you reviewed. ## 1. `replace` destroying the table You were right, and the trigger is broader than the one you found. Reproducing your mixed-column case against `clickhouse-connect==1.7.2` and then widening the probe: ``` mixed int/str Nullable(String) AttributeError: 'int' object has no attribute 'encode' mixed float/str Nullable(String) AttributeError: 'float' object has no attribute 'encode' bytes Nullable(String) TypeError: expected bytes, str found decimal Nullable(String) AttributeError: 'decimal.Decimal' object has no attribute 'encode' uuid Nullable(String) AttributeError: 'UUID' object has no attribute 'encode' ``` And a case neither of us had spotted, in the same class but on the *other* fallback: object columns holding `datetime.date` are declared `Nullable(DateTime64(6))`, and that writer calls `timestamp()` — which `date` doesn't have, only `datetime` does. So a plain date-only column failed on insert too, every time, on all three `if_exists` strategies. There was even a test asserting that type mapping, so the DDL was intended; the insert just could never satisfy it. Your framing — "the same robustness that makes CREATE always succeed is what carries execution past the DROP" — is exactly right, so the fix goes at both ends: **The declared type is now honoured.** `_coerce_to_declared_types` renders declared-`String` columns through `str()` (so `bytes`, `Decimal`, `UUID` and lists survive) and normalizes declared-`DateTime64` columns with `to_datetime`. Every row in that table now serializes against the real driver. Only columns the current call declares are touched — on `append` to an existing table the server's schema governs, not our inference, which I think is the correct boundary. **Typing and coercion moved ahead of all DDL**, so anything that can fail on the data now fails while the old table is still standing. **And the swap is staged**, as you sketched: ``` CREATE TABLE <target>__superset_staging_<rand> (...) ENGINE = MergeTree ORDER BY tuple() insert_df(staging) EXCHANGE TABLES <target> AND <staging> DROP TABLE <staging> # holds the old data after the swap ``` `NOT_IMPLEMENTED` falls back to drop-and-rename for legacy `Ordinary` databases, discriminated on the server's error text rather than an exception class — deliberately, because `clickhouse-connect` isn't in `requirements/development.txt`, so this path can't import the driver. (I found that the hard way: my first attempt caught `ClickHouseError` and would have broken CI, since the replace path does execute in unit tests. The whole file now passes with `clickhouse_connect` made unimportable.) One hazard was mine rather than yours: on the legacy fallback the target is dropped before the rename, so from that point staging holds the only copy of the data. Cleanup is suppressed past that line now, with a test for it. For the record on the [CodeAnt thread](https://github.com/apache/superset/pull/43000#discussion_r3748775036) — I argued against staging there and @rusackas agreed, on the grounds that it matched every other spec's `if_exists='replace'`. Your concrete trigger is what changed my mind, and I think the parity argument was weaker than it looked: the other specs drive CREATE and INSERT from the same pandas type inference, whereas this spec hand-rolls the DDL and so manufactures the mismatch itself. That thread and its [String-fallback sibling](https://github.com/apache/superset/pull/43000#discussion_r3748775033) are both addressed now. ## 2. Dotted table name misrouted Confirmed, independently — I'd driven `insert_context_sequence` and hit the same `"." not in table` hole. Fixed by passing the `qualified` string, as you suggested; `quote_identifier` is idempotent for already-backticked input and the dotted branch takes a pre-qualified name as-is, so it's byte-identical on every non-dotted case: ``` 'has.dot' database='myschema' -> DESCRIBE TABLE has.dot ❌ before 'has.dot' database='myschema' -> DESCRIBE TABLE `myschema`.`has.dot` ✅ after ``` I left `normalize_table_name_for_upload` alone — rejecting dots there would be a cross-engine behaviour change, and this makes the name work rather than forbidding it. ## Secondary - **`supports_multivalues_insert` comment** — reworded. You're right that it described a code path this PR removes; it now says the flag is unread on the upload path and is kept accurate for `superset test-db`. - **`chunksize`** — documented on `_insert_df` as deliberately not forwarded, since `insert_df` blocks by data volume rather than row count. - **TOCTOU** — largely dissolved by staging, as you predicted. - **DRY bot threads** — waving off, for the reasons in my previous comment. ## Test coverage Your gap is closed. The test double now tracks which tables exist and what each holds, so the replace path is asserted on its outcome — *does the user still have their data?* — rather than on statement order. New cases: insert fails mid-replace (original intact, no staging left behind), `EXCHANGE` fails, `EXCHANGE` unsupported → rename fallback, rename fails after the drop (loaded rows preserved), `replace` against a missing table, the `String` and `date` coercions, non-mutation of the caller's frame, and the qualified-insert target parametrized over dotted and schema-qualified names. **154 passing** in the file, plus the full `db_engine_specs` directory and `upload_command_test.py`; `pre-commit` green including `mypy` and `pylint` 10.00/10. Still no live server here — the verification above is against the real `clickhouse-connect` serializers rather than a running ClickHouse, so if you have an instance handy a real `replace` over a dirty CSV would be the last thing worth confirming. Two notes for the record: `EXCHANGE TABLES` briefly doubles disk for the table being replaced, which seems fine for ad-hoc upload tables, and the staging table is uniquely suffixed so concurrent uploads to the same target don't collide on it. @joe-clickhouse — you may want to see the staging swap and the `date`/`DateTime64` finding, both of which touch your earlier points. -- 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]
