s1ny1998 commented on PR #43000: URL: https://github.com/apache/superset/pull/43000#issuecomment-5614668401
@rusackas done — the `append` fix is in a7abe8d, on top of the commit @aminghadersohi reviewed. **`append` + `date`.** Appending to an existing table now reads its column types with `DESCRIBE TABLE` and runs the frame through the same `_coerce_to_declared_types` as the create path. So the server's schema still decides the types, and only the values get adjusted to fit. I went a little past `date` while I was there: - Wrapped types are unwrapped before matching, so a user-created `LowCardinality(Nullable(String))` or `DateTime64(3, 'UTC')` column is handled the same as ours. - Only columns that already hold `date`/`datetime` objects are converted for a `DateTime` column. Text like `01/02/2021` is never reparsed as a date; it goes to the driver as-is, and the driver rejects it. - Columns whose server type is anything else, or that the table doesn't have, go through untouched. **@aminghadersohi's other points.** - On the rename-after-drop path, a `logger.error` now names the staging table that holds the rows. - The `_coerce_to_declared_types` docstring now says `bytes` land as their repr. - The rename-failure test now asserts the staging table holds the loaded frame, not just that a staging table exists. **Tests.** The test double's `insert_df` is now as strict as the driver's writers: `String` needs `str` and `DateTime` needs `timestamp()`. It also tracks each table's schema from its `CREATE` and serves `DESCRIBE`, so the `upload → append` two-step fails in the unit tests the way it failed against a server. 164 pass. Against the pre-fix `clickhouse.py` the new file fails 10 tests: the append cases, the unwrap cases and the logging assertion. The other 154 pass there too, so none of the failures come from the double. I also ran it end to end this time against a live ClickHouse 26.9 with `clickhouse-connect==1.7.2`. That covered: - a messy CSV created and read back (NULLs, `0.1`, `1e40`, pre-1970 microsecond timestamps, a `uint64` above `Int64`) - the date append - a mixed-ID append onto `String` - an append into a user-created table with its own sort key, which stayed intact - `replace` via `EXCHANGE` - `replace` on an `Ordinary` database, which really does report `NOT_IMPLEMENTED` and takes the rename fallback - a failed `replace` leaving the original table intact - a dotted table name - post-upload reflection On the pre-fix code, the three append scenarios fail with exactly the `timestamp`/`encode` errors from the review. With this commit all ten pass. `pre-commit` is green. I've also resolved the CodeAnt atomicity thread and the two Bito DRY threads, as agreed above, and updated the description to cover the staging swap and the append coercion. -- 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]
