Fix(clickhouse): support non-datetime time column partitioning#3357
Fix(clickhouse): support non-datetime time column partitioning#3357
Conversation
19fba89 to
ed66bbd
Compare
53151e0 to
14dee18
Compare
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
14dee18 to
19e2b06
Compare
georgesittas
left a comment
There was a problem hiding this comment.
Nice, thanks for addressing comments 👍
|
Hey @treysp what version of Clickhouse are you using? I am experimenting with sqlmesh + clickhouse and finding that this causes an error because Adding a scale of 6 here and just below fixes the problem. But the choice of 6 feels a bit arbitrary. What if the column were a nanosecond-precision string-formatted timestamp? |
|
Thanks for reporting - that looks like a bug. Precision is essentially arbitrary, so my inclination would be to go for nanosecond to prevent truncation. Do you see any issues with that? Pandas handles nanosecond timestamps, so conversion to Python should be ok. |
|
Addressing this in #3582 |
The Clickhouse adapter automatically partitions incremental by time models if the time column is not included in any partitioning expression.
Clickhouse strongly recommends against fine-grained/small partitions. Therefore, the automatic partitioning "floors" the time column to weekly granularity with the
toMonday()function.toMonday()only accepts date/datetime input types, so we currently error if the time column is string type.This PR allows non-date/datetime time columns by:
DateTime64before passing totoMonday()then cast output back to original time column typeDateTime64before passing totoMonday()Implementation note
This PR moves the
partitioned_byproperty from theModelMetato_Modelclass so it can access the_Modelcolumn_to_typesproperty.