Skip to content

Cherry-pick: Respect use_client_time_zone for string datetime literals parsed on the server#2046

Open
zvonand wants to merge 3 commits into
stable-25.8from
releasy/port/pr-109051-a7e5ac
Open

Cherry-pick: Respect use_client_time_zone for string datetime literals parsed on the server#2046
zvonand wants to merge 3 commits into
stable-25.8from
releasy/port/pr-109051-a7e5ac

Conversation

@zvonand

@zvonand zvonand commented Jul 15, 2026

Copy link
Copy Markdown
Member

Cherry-picked from ClickHouse#109051.

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fixed use_client_time_zone being ignored for DateTime/DateTime64 string literals interpreted on the server (asynchronous INSERT, SELECT literals). The client now propagates its local time zone as session_timezone when use_client_time_zone is enabled, so server-side parsing matches the synchronous INSERT path (ClickHouse#109051 by @groeneai).

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

…se-client-time-zone-108038

Respect use_client_time_zone for string datetime literals parsed on the server
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

Workflow [PR], commit [f7c3084]

@zvonand zvonand added the 25.8 25.8 Altinity Stable label Jul 15, 2026
@zvonand

zvonand commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

@blau-ai

@blau-ai

blau-ai commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

CI triage

Verdict: 2 red checks — 0 are a real code problem in this port. 1 is a metadata/sign-off issue, 1 is an unrelated flaky test.

The PR and the S3 Fast test/Config Workflow/Finish Workflow rows are just aggregate mirrors of the two checks below — no separate failures. Everything else is skipping.


1. DCO — ❌ metadata, PR-related but not a code issue

All commits are incorrectly signed off … 82e5b3d "Merge pull request ClickHouse#109051 from groeneai/fix-async…" — Sign-off not found

The cherry-picked commit message has no Signed-off-by: line. This is purely a commit-metadata gate, not a build/test problem.

Fix (needs a human — I don't force-push, and this is the PR head branch, not a blau/* branch):

git checkout releasy/port/pr-109051-a7e5ac
git commit --amend --signoff --no-edit
git push --force-with-lease origin releasy/port/pr-109051-a7e5ac

(If you'd rather not amend, DCO also accepts a separate empty remediation commit — see the "add remediation commit" option in the check output. Amending the single commit is cleaner here.)


2. Fast test — ❌ one flaky test, not related to this PR

Failing test: 03221_merge_profile_events (result diff, not the code this PR touches):

--- reference
+++ stdout
 Vertical  1  20000  1  2  480000  1  1  1  1  1  1  1  1
-Vertical  1  20000  1  2  480000  1  1  1  1  1  1  1  1
+Vertical  1  20000  1  2  480000  1  1  1  1  1  1  0  1

The only differing column is the 12th value — ProfileEvents['UserTimeMicroseconds'] > 0. For the small 20 000-row vertical merge it returned 0, i.e. the merge was fast/idle enough that accounted user-CPU time rounded to 0. That's an inherent timing/resource-accounting flake in this test (it asserts several …Milliseconds > 0 / CPU-time counters are non-zero), and it has nothing to do with client time-zone propagation.

Evidence it isn't this PR:

  • The diff only touches programs/client/Client.cpp and src/Client/ClientBase.{cpp,h} (client-side session_timezone seeding) plus a new stateless test. Nothing here influences merge profile-event accounting.
  • The PR's own new test 04401_async_insert_use_client_time_zone was [ SKIPPED ] in Fast test (it's tagged no-fasttest), so the failure is definitely not from this port's additions.

Next step: safe to re-run ("Fast test" only). If it recurs, it's a pre-existing flake in 03221_merge_profile_events worth a separate timing-tolerance fix, independent of this PR.


Note on validation of the port itself

The new test that actually exercises this fix (04401_async_insert_use_client_time_zone) is no-fasttest, and the Stateless tests … jobs are all skipping in this run — so the port's own behavior hasn't been executed by CI yet. Once DCO is green and the Fast test re-run passes, it's worth making sure the stateless suite runs so 04401 actually validates the change.

@zvonand

zvonand commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

@blau-ai

@blau-ai

blau-ai commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

CI triage

Verdict: exactly one failure is caused by this PR — the new test 04401_async_insert_use_client_time_zone that this cherry-pick adds, and it fails deterministically (not flaky). Everything else red is either the DCO sign-off check (a process issue on the cherry-picked upstream merge commit) or unrelated pre-existing/environmental test failures in the heavy s3 / DataLake configs. The PR check is just the aggregate gate rolling those up.


1. 04401_async_insert_use_client_time_zone — ❌ PR-caused, deterministic

Fails in every parallel stateless job (amd_asan 1/2 & 2/2, amd_debug, amd_binary ParallelReplicas & old-analyzer/DatabaseReplicated, arm_binary) and 50/50 runs in the amd_asan, flaky check.

Decisive diff (flaky check, job):

 @@ -1,7 +1,7 @@
-flag_async           1500036000     ← expected (client tz, America/Hermosillo = UTC-7)
+flag_async           1500003600     ← actual   (server tz)
 flag_sync            1500036000     ← sync insert is correct
-reset_default_async  1500036000
-set_async            1500036000
+reset_default_async  1500003600
+set_async            1500003600
 ...

What this means: the synchronous inserts (parsed client-side) get the correct client-timezone instant 1500036000; every asynchronous insert instead lands on the server-timezone instant 1500003600. In other words, the client-time-zone → session_timezone propagation that this PR adds has no effect on the async path in this build.

It is not a bad cherry-pick. The port includes all 5 upstream files, and the Client.cpp / ClientBase.cpp / ClientBase.h hunks are byte-identical to upstream ClickHouse#109051. The same code is green on upstream master. So the gap is on the server/branch side, specific to stable-25.8. Leading hypotheses:

  • The server-side half of the feature — async-INSERT VALUES parsing honoring session_timezone — is present on master but missing on stable-25.8, so a client-only backport is insufficient; or
  • On 25.8, client_local_timezone comes out empty (DateLUT::serverTimezoneInstance().getTimeZone() at connect() time doesn't yet reflect the client TZ), so the guard if (!client_local_timezone.empty() && …) skips the propagation — which would produce exactly this symptom (async falls back to server tz).

Suggested next steps (I did not push anything):

  1. On stable-25.8, run an async INSERT with an explicit SETTINGS session_timezone='America/Hermosillo' and a DateTime string literal — does the stored instant respect that zone? If no, the server-side change must be backported alongside this client change.
  2. Confirm client_local_timezone is actually populated on 25.8 (log/inspect it at connect()).
  3. Worth pinging the original author (@yariks5s / groeneai) on which upstream commit the server-side behavior depends.

If you'd like, I can dig into the 25.8 async-insert path and prepare either the missing server-side backport or a test adjustment on a blau/* branch — say the word and tell me which delivery you prefer.

2. DCO⚠️ not a code failure (process)

The cherry-picked upstream merge commit 82e5b3d has no Signed-off-by line; the merge commit 753e536 is fine. Fix: re-sign that commit (git commit -s --amend / rebase with sign-off) or waive DCO for this backport per your normal port policy — common for ports of upstream merge commits. (I can't amend/force-push under my permissions.)

3. PR — aggregate gate

Red only because of the stateless jobs above. No independent action.

4. Other stateless failures — ✅ not PR-related

The multi-failure parallel jobs consistently also fail tests in subsystems this PR never touches (it only changes client-side timezone handling in Client.cpp / ClientBase.*):

  • 03441_deltalake_clickhouse_virtual_columns, 03441_deltalake_clickhouse_public_datasets, 04340_datalake_schema_deep_recursion (DeltaLake / DataLake)
  • 04299_constraint_subst_correlated_subquery_root, 01710_projection_additional_filters
  • 03579_create_table_populate_from_s3, 04409_explain_actions_secret_args, 04410_explain_actions_secret_args_secondary

A client-timezone change cannot affect DeltaLake schema recursion, projections, or EXPLAIN secret-arg masking — these are pre-existing/environmental failures in the s3-storage / DatabaseReplicated / DataLake configs. Recommend confirming against a recent stable-25.8 base run and re-running; they shouldn't block on this PR's account.


Bottom line: fix the real one — 04401 (the async path isn't picking up the client timezone on 25.8, almost certainly a missing server-side prerequisite or an empty client_local_timezone) — and clear DCO. The remaining reds are noise from this PR's perspective.

🤖 Generated with Claude Code · evidence: run 29609649541

…r-side DateTime parsing

Test 04401_async_insert_use_client_time_zone (from ClickHouse#109051) requires the
server to respect session_timezone when parsing DateTime text into columns
without an explicit timezone. Upstream has this since PR ClickHouse#100647 (26.4,
backported to 26.3/26.2) but stable-25.8 does not, so async inserts (TCP)
and all inserts over HTTP were parsed in the server timezone.

- DataTypeDateTime/DataTypeDateTime64: resolve the effective timezone
  (respects session_timezone via DateLUT::instance()) when creating the
  default serialization. Adapted from upstream doGetSerialization() to
  25.8's doGetDefaultSerialization() API (no serialization object pool).
- MutationsDateTimeLiteralVisitor: wrap DateTime string literals in
  mutation predicates/assignments with explicit toDateTime(..., tz) at
  ALTER time, since background mutation threads lack the session context.
  Adapted make_intrusive -> std::make_shared for 25.8 AST pointers.
- Tests from the upstream PR: 02737 update, 04056, 04057.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

25.8 25.8 Altinity Stable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants