Skip to content

Keep DateTime64 precision and timezone in named Tuple, Nested, Variant and Map binds - #1130

Closed
sebastianbreguel wants to merge 1 commit into
ClickHouse:mainfrom
sebastianbreguel:fix/dt64-nested-bind-precision
Closed

sebastianbreguel wants to merge 1 commit into
ClickHouse:mainfrom
sebastianbreguel:fix/dt64-nested-bind-precision

Conversation

@sebastianbreguel

Copy link
Copy Markdown
Contributor

Summary

When a datetime is bound server-side to a DateTime64 element, _promote_datetime64 wraps it in DT64Param to keep sub-second precision. This only worked at the top level, in Array, and in unnamed Tuple. Other container types sent whole seconds (ClickHouse 26.3, D = datetime(2024, 1, 2, 3, 4, 5, 123456), U = the same time in UTC):

{p:Tuple(DateTime64(6), Int32)}            ('2024-01-02 03:04:05.123456',1)   <- works
{p:Tuple(a DateTime64(6), b Int32)}        ('2024-01-02 03:04:05.000000',1)
{p:Tuple(a DateTime64(6, 'Asia/Tokyo'))}   ('2024-01-02 03:04:05.000000')    <- precision and timezone lost (U)
{p:Variant(DateTime64(6), String)}         2024-01-02 03:04:05.000000
{p:Nested(a DateTime64(6))}                [('2024-01-02 03:04:05.000000')]
{p:Map(String, DateTime64(6))}             {'k':'2024-01-02 03:04:05.000000'}  (dict_parameter_format='map')

Named elements failed because "a DateTime64(6)" was read as a type called a.

What changed, all in clickhouse_connect/driver/binding.py:

  • _strip_element_name removes the name from a named element before promotion. It handles quoted names, including backtick or double quotes with doubled or backslash-escaped quotes, and names containing parentheses. DateTime64 (6) is not mistaken for a name.
  • _promote_datetime64 now also handles Nested (as an array of tuples), Variant, and Map.
    • Map is handled only with dict_parameter_format='map'. JSON mode serializes the dict itself and is unchanged.
    • Variant is promoted only when it has no plain DateTime alternative. I checked on the server that a value with a fraction makes it pick DateTime64 instead of DateTime. Promoting would therefore change the variant that Variant(DateTime, DateTime64(6)) gets today, so that case stays as it is.
  • DT64Param takes an optional tz. A promoted element carries the timezone named in its own DateTime64 type, so Tuple(a DateTime64(6, 'Asia/Tokyo'), b DateTime64(6, 'UTC')) renders each element in its own zone. Before, the whole parameter used one timezone. Top-level binds and Array binds produce the same strings as before. A DT64Param with no timezone placed inside one of these containers gets the element's timezone too. The wrapper is copied, so the caller's object is not changed. The SQLAlchemy subclass _DateTime64QueryParam honors that timezone. A DT64Param that already has a timezone, and the _64 suffix, are unchanged.
  • _unwrap_outer and the element-timezone lookup are cached (lru_cache(maxsize=256)). Before this, every array element parsed the same type string again. Both caches hold only immutable results.

Still unchanged: plain DateTime (non-64) elements in a multi-timezone Tuple still share the first timezone found. That case has no wrapper, and fixing it is a separate change.

Verification

  • New unit tests in TestBindQueryDateTime64Precision: named tuple, named tuple tz hint, per-element timezones, quoted names (4 variants), existing DT64Param / SQLAlchemy wrappers taking the element timezone (directly and through Variant), nested, variant, map. Plus a guard showing that Variant(DateTime, DateTime64(6)) is unchanged.

  • Integration tests test_datetime64_params_in_containers (6 cases, including a wrapped DT64Param) and test_datetime64_params_in_map, sync and async. They compare against the server's toString. On main, 25 of the selected unit and integration cases fail. With the fix all pass.

  • ruff format --check, ruff check, and mypy are clean. Unit tests: 4352 passed. Integration (-n 4, ClickHouse 26.3, including the SQLAlchemy suite): 1806 passed, 29 skipped. 2 failures are test_wrong_port_error_message, which needs local port 9000 (environment). 18 unit errors are test_httpclient.py tests that hardcode localhost:8123 (environment).

  • Performance: bind_query on origin/main vs this PR, timed with /usr/bin/time -l. There were 16 interleaved runs per side, each in a fresh process, and the order alternated each round. Python 3.12, Apple Silicon. Wall and CPU cover the bind loop. I started each case once the 1-minute load average was under 3; it stayed between 2.5 and 3.3 during the runs. Cases:

    • plain: {x:Int32}, {s:String}, {d:DateTime}, 20,000 binds.
    • arr1k: Array(DateTime64(6)) with 1,000 values, 200 binds.
    • arr100k: 100,000 values, 2 binds.
    • _tz: the same type with 'Asia/Tokyo'.
    case side wall p50/p80/p95 (s) CPU p50/p80/p95 (s) peak RSS p50 (MiB)
    plain main 0.114 / 0.115 / 0.117 0.113 / 0.115 / 0.117 33.5
    plain PR 0.101 / 0.102 / 0.103 0.101 / 0.102 / 0.103 33.5
    arr1k main 0.506 / 0.514 / 0.517 0.505 / 0.513 / 0.517 33.7
    arr1k PR 0.406 / 0.412 / 0.414 0.405 / 0.411 / 0.413 33.8
    arr1k_tz main 0.756 / 0.757 / 0.761 0.754 / 0.756 / 0.760 33.8
    arr1k_tz PR 0.428 / 0.433 / 0.437 0.427 / 0.432 / 0.437 33.8
    arr100k main 0.513 / 0.523 / 0.530 0.511 / 0.522 / 0.529 61.6
    arr100k PR 0.421 / 0.425 / 0.427 0.420 / 0.424 / 0.426 61.7
    arr100k_tz main 0.796 / 0.803 / 0.809 0.795 / 0.799 / 0.805 61.7
    arr100k_tz PR 0.452 / 0.462 / 0.464 0.451 / 0.460 / 0.463 61.6

    Nothing got slower. DateTime64 array binds are about 18 to 43% faster because of the parse cache, and plain about 11%. Peak RSS is within noise: the medians differ by at most 0.1 MiB.

Checklist

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

@sebastianbreguel

Copy link
Copy Markdown
Contributor Author

Folded into #1118, since both change binding.py and test_params.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant