-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice solution
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-datetime
types. Insteaddriver/upload-type->database-type
will return nil for this type, and we'll default to use thevarchar-255
database-type by default.