More upload test changes for clickhouse #42531
Conversation
|
| [["Obi-Wan Kenobi" "No one really knows me"] | ||
| ["Puke Nightstalker" "Nothing - you can't prove it"]]) | ||
| (rows-for-table table)))) | ||
| (is (= (set (rows-with-auto-pk |
There was a problem hiding this comment.
Why are we need to add this casting to a set, does Clickhouse not provide a stable order?
Not important for this PR, but I think it would be good to test that order is preserved for all the drivers that support that. This probably doesn't warrant a feature or method, but perhaps there's another way to opt out of the stronger test?
There was a problem hiding this comment.
Without an order by clause, Clickhouse queries will return the last rows inserted first, while other drivers return the first rows appended first. I don't see how we can change this without adding an auto-incrementing PK and ordering the results by that column. We don't document any guarantees about the order so I think this is okay for now while we allow drivers to not have auto-incrementing PKs.
There was a problem hiding this comment.
Ah thanks for clarifying its semantics. Not that it's crucial, if we wanted to make the tests "tighter" in the short term we could only cast to a set if :upload-with-auto-pk is false. It would be easy enough to strengthen or relax that internal coupling if things change.
| :valid #t "2000-01-01T00:00:00" | ||
| :invalid "2023-01-01T00:00:00+01" | ||
| :msg "'2023-01-01T00:00:00+01' is not a recognizable datetime"}] | ||
| (driver/upload-type->database-type driver/*driver* ::upload/offset-datetime) |
This PR contains a bunch of changes needed for the planned support of CSV uploads for clickhouse.
Most notably, it allows clickhouse to not support
::upload/offset-datetimetypes. Insteaddriver/upload-type->database-typewill return nil for this type, and we'll default to use thevarchar-255database-type by default.