iframe-proxy

BoloniniD · 2026-04-04T22:15:36Z

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 into CHANGELOG.md):

Fixes silent data loss when using large intervals with DateTime columns in TTL expressions due to 32-bit Unix timestamp overflow. Closes #101763

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Note

Medium Risk
Changes TTL expression type analysis and runtime evaluation for Date/DateTime columns, which directly affects TTL expiration decisions and data deletion/part cleanup behavior. While scoped to temporal types, incorrect casts or analyzer mismatches could change retention outcomes for some tables.

Overview
Prevents silent TTL overflow on 16/32-bit Date/DateTime columns by analyzing TTL expressions against widened temporal types (Date32/DateTime64(0, tz)), preserving Nullable semantics and dropping LowCardinality where required.

Updates TTL execution to cast required input columns to the types expected by the analyzed expression before running it, ensuring the runtime block matches the widened analysis view.

Adds a stateless regression test covering large-interval TTLs across rows/column/GROUP BY/recompress TTL modes, plus LowCardinality/Nullable temporal columns and a negative control for normal expiration.

^{Reviewed by Cursor Bugbot for commit 43b6d26. Bugbot is set up for automated code reviews on this repo. Configure here.}

clickhouse-gh · 2026-04-10T12:27:29Z

Workflow [PR], commit [1ced73d]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
Stress test (amd_tsan)		FAIL
	Hung check failed, possible deadlock found	FAIL	cidb, issue
Stress test (arm_tsan)		FAIL
	Hung check failed, possible deadlock found	FAIL	cidb, issue

AI Review

Summary

This PR fixes the reported direct Date / DateTime TTL overflow path by analyzing temporal source columns as Date32 / DateTime64 and casting execution inputs to match. The direct ts + INTERVAL 100 YEAR case is materially improved, but there are still unresolved data-loss paths around the compatibility fallback and stale part TTL metadata, so I do not think this is merge-ready as a complete bug fix.

PR Metadata

💡 The Bug Fix category is appropriate, but the changelog entry overstates the current scope. If the scoped approach is kept, use narrower text such as: Fixes silent data loss for direct Date/DateTime column plus large interval TTL expressions by evaluating that arithmetic in widened temporal types.

Findings

❌ Blockers

[src/Storages/TTLDescription.cpp:271] [dismissed by author -- https://github.com/Fix ALTER TABLE MODIFY TTL with DateTime causing data loss on 32-bit overflow #101793#discussion_r3407663118] The fallback re-analyzes the entire TTL expression with original narrow types when any subexpression rejects the widened type. This can still hit the reported direct-overflow shape inside a mixed expression, for example if(flag, ts + INTERVAL 100 YEAR, tumbleStart(ts, INTERVAL 1 HOUR) + INTERVAL 1 DAY): tumbleStart makes widened analysis throw, then the targeted ts + INTERVAL 100 YEAR branch runs through the old DateTime arithmetic and can wrap into the past. Please make the fallback fail closed when the retried expression still applies large interval arithmetic to Date / DateTime, or move the protection into the interval arithmetic implementation itself.
[src/Processors/Transforms/TTLTransform.cpp:76] [dismissed by author -- https://github.com/Fix ALTER TABLE MODIFY TTL with DateTime causing data loss on 32-bit overflow #101793#discussion_r3369599491] Existing parts can already contain wrapped ttl_infos computed by the old narrow expression, and the whole-part drop shortcut checks old_ttl_info.max before the widened expression can recompute row TTLs. A part inserted before upgrade with wrapped max in the past can therefore be deleted after upgrade even though this PR would now evaluate the TTL into the future. Please recompute or ignore stale ttl_infos for widened Date / DateTime TTL descriptions before taking the whole-part drop path.

Tests

⚠️ Add a regression for the fallback case where one narrow-only subexpression forces fallback while another branch still contains direct DateTime + INTERVAL arithmetic.
⚠️ Add an upgrade-style regression that starts from a part with stale wrapped ttl_infos and proves the widened TTL is recomputed before any whole-part delete shortcut runs.

Final Verdict

Status: ❌ Block

Minimum required actions: fail-close the fallback or implement checked interval arithmetic, and handle stale wrapped part TTL metadata before merge. If maintainers intentionally keep the narrower scope, the changelog should say so explicitly.

BoloniniD · 2026-04-20T10:09:24Z

Hi @zlareb1, most checks are now passing. Would appreciate a review when you have time.

P.S. Thanks to @fastio (#102020). I incorporated the widening approach from their PR to make this fix more robust.

alexey-milovidov

This is doubtful. A complex logic for a simple change. Maybe we can find a better way?

Without this, `widenTemporalColumns` stripped `Nullable` when rewriting `Date`/`DateTime` source columns. The analyzer then saw the column as non-null and constant-folded `isNull` / `ifNull`, silently changing TTL decisions for actually-`NULL` rows in both rows TTL and `DELETE WHERE`. `LowCardinality` is still dropped because `LowCardinality(DateTime64)` is rejected by `DataTypeLowCardinality`. The runtime cast in `ITTLAlgorithm::executeExpressionAndGetColumn` converts the original `LC` column to the widened type. Added stateless coverage for `Nullable(Date)` and `Nullable(DateTime)` TTL expressions exercising `ifNull` over the column.

Workflow files in this branch were stale or had been deleted as a workaround for GitHub's workflow-scope check. Restoring them to match `master` so the PR diff no longer contains those unrelated changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

These changes to CI workflow / infrastructure files were unrelated to the purpose of this pull request and were introduced accidentally (stale base branch). Restoring them to match `master`.

PedroTadim · 2026-06-03T10:35:55Z

Hello! Is this PR going to be finished and merged soon?

# Conflicts: # src/Processors/TTL/ITTLAlgorithm.cpp

…xercises overflow Case 2 set `mutations_sync = 0`, and that leaked into the following cases. With the default `materialize_ttl_after_modify = 1`, those `ALTER ... MODIFY TTL` statements only enqueued an asynchronous `MATERIALIZE TTL` mutation, so the immediate `SELECT count()` could run before the TTL was evaluated and pass even if the 32-bit overflow path were still live. Keep `mutations_sync = 2` for the rest of the file so the materialization completes before the assertions. Addresses review feedback on the direct `ts + INTERVAL 100 YEAR` case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

clickhouse-gh · 2026-06-07T15:25:48Z

    ExpressionAndSets result;
    auto ttl_string = ast->formatWithSecretsOneLine();
-    auto syntax_analyzer_result = TreeRewriter(context).analyze(ast, columns);
+    auto syntax_analyzer_result = TreeRewriter(context).analyze(ast, widenTemporalColumns(columns));


The widening here only helps after the TTL expression is evaluated, but existing parts may already have wrapped TTL metadata from the old DateTime evaluation. For example, a table created before this change with TTL ts + INTERVAL 100 YEAR can have table_ttl.max stored as a past timestamp while the rows themselves are still present. On the first TTL merge or MATERIALIZE TTL after upgrade, TTLTransform / TTLDeleteFilterTransform still check old_ttl_info.max and set the whole-part drop path before running the widened expression, so the part can be deleted without ever recalculating ts + INTERVAL 100 YEAR in the widened domain.

That leaves an upgrade path for the same silent data loss this PR is trying to close. Please make the whole-part shortcut recalculate or ignore stale Date / DateTime TTL infos for widened TTL descriptions, and add a regression that starts from stale wrapped part TTL metadata.

This is a pre-existing stored-metadata concern in code this PR does not modify (TTLTransform / TTLDeleteFilterTransform). The exposure window is narrow: if the old narrow evaluation stored max in the past, those rows would already have been dropped by the old code before any upgrade — the only remaining case is a part inserted just before upgrade and not yet TTL-merged. Closing it requires recomputing ttl_infos or disabling the whole-part drop shortcut for widened TTL descriptions, which has merge-performance trade-offs, so it is a maintainer design call rather than a mechanical fix. See the status comment for the proposed direction.

Analyzing the TTL expression against `Date` / `DateTime` source columns widened to `Date32` / `DateTime64(0, tz)` fixes the silent 16/32-bit overflow (ClickHouse#101763), but it also rejects previously-valid TTLs whose functions accept only the narrow temporal types. For example `tumbleStart` requires `DateTime`, not `DateTime64`, so ALTER TABLE t MODIFY TTL tumbleStart(ts, INTERVAL 1 HOUR) + INTERVAL 1 DAY started failing with an illegal-argument-type exception, which would also break `ATTACH` / startup of legacy tables after an upgrade. Wrap the widened analysis in a fallback: if it throws, retry against the original column types. Such expressions explicitly operate in the narrow `Date` / `DateTime` domain and are out of scope for the overflow fix. The common `column + INTERVAL ...` case is unaffected and still analyzed in the widened 64-bit domain. Extract the analysis into a helper that clones the AST per attempt so a failed widened pass does not leave a half-rewritten AST for the fallback. Add Case 12 to `04077_datetime32_overflow` covering the `tumbleStart` compatibility regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

clickhouse-gh · 2026-06-13T08:14:21Z

+        {
+            return analyzeExpressionAndSets(ast, widened_columns, context);
+        }
+        catch (const Exception &) // NOLINT(bugprone-empty-catch): intentional fallback to the narrow analysis below


The fallback here disables widening for the entire TTL expression. If widened analysis fails because one subexpression needs the narrow type, any later large interval also runs in the old narrow domain. For example TTL tumbleStart(ts, INTERVAL 1 HOUR) + INTERVAL 100 YEAR reaches this catch because tumbleStart rejects DateTime64, then the retry analyzes the final + INTERVAL 100 YEAR as addYears on DateTime; that path still casts the result back to UInt32, so a timestamp past 2106 can wrap into the past and expire rows.

Please keep the compatibility fallback from accepting expressions that still apply large interval arithmetic to narrow Date / DateTime results, or make the fallback narrower so the result of the narrow-only function is converted back to DateTime64 before the interval arithmetic is analyzed.

This is not a regression versus pre-PR behavior: the fallback fires only when widened analysis throws (a function rejects DateTime64, e.g. tumbleStart). The targeted column + INTERVAL ... case never reaches it. Expressions like tumbleStart(ts, INTERVAL 1 HOUR) + INTERVAL 100 YEAR already wrapped before this PR and still wrap via the fallback — the same out-of-scope class @alexey-milovidov described above. Removing the fallback would break ATTACH of legacy tables using tumbleStart-style TTLs. The clean fix that would also cover this is throwing on overflow in addInterval itself; raised as a design decision in the status comment.

alexey-milovidov · 2026-06-30T07:15:55Z

Status update (continue-pr)

Merged current master into the branch (it was ~6360 commits / 17 days behind): b4763c536fa..1ced73da590, no conflicts. The net diff against master is still exactly the four PR files (src/Processors/TTL/ITTLAlgorithm.cpp, src/Storages/TTLDescription.cpp, and the 04077_datetime32_overflow test); submodule pointers are identical to master. Both changed translation units compile against current master (-fsyntax-only), CI was green on the previous head, and it is re-running now.

I did not expand the scope of the change. Three review threads are still open; here is where each stands and why I left the code as-is rather than adding logic:

Narrowing-back expressions (toDateTime(ts) + INTERVAL ..., CAST(ts AS DateTime) + ...) — already adjudicated by @alexey-milovidov as out of scope (Fix ALTER TABLE MODIFY TTL with DateTime causing data loss on 32-bit overflow #101793 (comment)). No action.
AI blocker — fallback re-narrows mixed expressions (TTLDescription.cpp:271, e.g. tumbleStart(ts, INTERVAL 1 HOUR) + INTERVAL 100 YEAR). The fallback fires only when widened analysis throws because a function rejects DateTime64 (e.g. tumbleStart). The simple column + INTERVAL ... case this PR targets never reaches it (widening succeeds, no throw). So this is not a regression: such derived/narrow-result expressions already wrapped before this PR and still wrap via the fallback — the same class as (1). Removing the fallback is not an option, as it would break ATTACH of legacy tables that use tumbleStart-style TTLs. A precise fix (rewriting each narrow-only sub-result back to DateTime64 before the interval) is exactly the AST surgery the "complex logic for a simple change" comment is pushing back on.
AI blocker — stale wrapped ttl_infos on upgrade (TTLTransform.cpp:76). A pre-existing stored-metadata concern in code this PR does not touch. The window is narrow: if the old narrow code computed max in the past, it would already have dropped those rows before the upgrade — the only remaining exposure is a part inserted just before upgrade and not yet TTL-merged. Closing it means recomputing ttl_infos / disabling the whole-part drop shortcut for widened TTL descriptions, which carries merge-performance trade-offs. That is a design call, not a mechanical fix.

Design decision for the maintainer. All three open items have the same shape: the widening approach fixes the reported case (#101763) but cannot, by construction, catch interval arithmetic applied to a narrow value produced inside the expression. The complete-and-simpler alternative @alexey-milovidov already named — throw on overflow directly in the UInt16/UInt32 add-interval implementations (FunctionDateOrDateTimeAddInterval.h), gated by allow_suspicious_ttl_expressions for legacy ATTACH — would cover all three in one place with far less code. It was attempted on this branch earlier and dialed back per scope, so it needs an explicit decision rather than me re-expanding the PR unilaterally:

(a) Merge the current scoped widening fix as-is (green, mergeable) and track the narrowing/upgrade cases as a follow-up; or
(b) Replace the widening + fallback machinery with overflow-throwing in addInterval, which covers all three threads.

@alexey-milovidov @BoloniniD — which direction do you want? I will implement (b) if that is the call.

Note on (b): the in-arithmetic fix could either throw on overflow (as @alexey-milovidov named in the thread above) or saturate to the max representable value — the latter is non-breaking and matches the addSaturating precedent in #108848. Whichever you prefer; the point is that it lives in addInterval rather than in per-TTL widening + fallback + cast machinery, and closes all three threads at once.

alexey-milovidov · 2026-06-30T09:20:04Z

CI follow-up (continue-pr)

The re-run after the master merge is essentially complete. The only red checks are Stress test (amd_tsan) and Stress test (arm_tsan), both failing on "Hung check failed, possible deadlock found" — the known master-wide flake #107941 (open). It is unrelated to this PR:

Both reds are TSan-only; every non-TSan stress variant is green (amd_debug, amd_asan_ubsan, amd_msan, arm_debug, arm_asan_ubsan, arm_asan_ubsan s3, arm_msan, arm_ubsan).
The hung query is a fuzzer-generated SELECT formatQuery($$SELECT [[[[…]]]]$$) on a deeply-nested array literal — a parser/formatter slowdown under TSan, not a deadlock in this PR's code.
The 240k-line hung-check thread dump contains zero TTL frames (TTLDescription, ITTLAlgorithm::executeExpressionAndGetColumn, widenTemporalColumns, and TTLTransform are all absent).

No code change is warranted for these. The net diff against master is still exactly the four PR files, and the PR is otherwise green and mergeable. The only substantive open item remains the scope/approach design decision from the status update above — (a) merge the scoped widening fix and track the narrowing/upgrade cases as follow-ups, or (b) move the protection into addInterval. Awaiting that call.

clickhouse-gh · 2026-06-30T10:48:14Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	0.00%	85.50%	+85.50%
Functions	0.00%	92.70%	+92.70%
Branches	0.00%	77.70%	+77.70%

Full report

alexey-milovidov · 2026-07-02T08:04:51Z

BoloniniD changed the title ~~Fix ALTER TABLE MODIFY TTL with DateTime causing data loss on 32-bit overflow~~ WIP Fix ALTER TABLE MODIFY TTL with DateTime causing data loss on 32-bit overflow Apr 4, 2026

BoloniniD marked this pull request as ready for review April 5, 2026 10:34

BoloniniD changed the title ~~WIP Fix ALTER TABLE MODIFY TTL with DateTime causing data loss on 32-bit overflow~~ Fix ALTER TABLE MODIFY TTL with DateTime causing data loss on 32-bit overflow Apr 5, 2026

zlareb1 added the can be tested Allows running workflows for external contributors label Apr 10, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Apr 10, 2026

clickhouse-gh Bot reviewed Apr 10, 2026

View reviewed changes

Comment thread src/Functions/FunctionDateOrDateTimeAddInterval.h Outdated

clickhouse-gh Bot reviewed Apr 10, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

clickhouse-gh Bot reviewed Apr 10, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

BoloniniD force-pushed the master branch from 64bd9ea to 0713b17 Compare April 18, 2026 17:13

clickhouse-gh Bot reviewed Apr 18, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

clickhouse-gh Bot reviewed Apr 18, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

clickhouse-gh Bot reviewed Apr 18, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

clickhouse-gh Bot reviewed Apr 18, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

clickhouse-gh Bot reviewed Apr 18, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

clickhouse-gh Bot reviewed Apr 18, 2026

View reviewed changes

Comment thread src/Processors/TTL/ITTLAlgorithm.h

clickhouse-gh Bot reviewed Apr 19, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04077_datetime32_overflow.sql Outdated

clickhouse-gh Bot reviewed Apr 19, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

clickhouse-gh Bot reviewed Apr 19, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04077_datetime32_overflow.sql Outdated

clickhouse-gh Bot reviewed Apr 19, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

clickhouse-gh Bot reviewed Apr 19, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04077_datetime32_overflow.sql Outdated

clickhouse-gh Bot reviewed Apr 19, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

clickhouse-gh Bot reviewed Apr 19, 2026

View reviewed changes

Comment thread src/Processors/TTL/TTLAggregationAlgorithm.cpp Outdated

BoloniniD added 2 commits April 19, 2026 23:55

Implement a fix for DateTime32 TTL data loss

875f410

Merge branch 'master' of github.com:ClickHouse/ClickHouse

1b9c433

BoloniniD force-pushed the master branch from 139fa83 to 1b9c433 Compare April 19, 2026 22:56

alexey-milovidov reviewed May 1, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

alexey-milovidov reviewed May 1, 2026

View reviewed changes

alexey-milovidov self-assigned this May 1, 2026

groeneai mentioned this pull request May 18, 2026

Add regression test for empty-pipe reverse-order read (STID 2635-3a9e) #105132

Merged

alexey-milovidov added 2 commits May 20, 2026 20:40

Merge remote-tracking branch 'origin/master' into pr-101793

77d1fdd

Restore workflow files to pre-merge state (push without workflow scope)

fab91c2

clickhouse-gh Bot added the manual approve Manual approve required to run CI label May 20, 2026

alexey-milovidov added 2 commits May 23, 2026 22:35

Restore unrelated workflow files from master

27a6de4

Workflow files in this branch were stale or had been deleted as a workaround for GitHub's workflow-scope check. Restoring them to match `master` so the PR diff no longer contains those unrelated changes.

Merge master to refresh merge base

43b6d26

alexey-milovidov removed the manual approve Manual approve required to run CI label May 24, 2026

alexey-milovidov and others added 2 commits May 31, 2026 04:46

Merge remote-tracking branch 'origin/master' into pr-101793

7d6cd13

Restore workflow files to pre-merge state (push without workflow scope)

7dc358c

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

clickhouse-gh Bot added the manual approve Manual approve required to run CI label May 31, 2026

alexey-milovidov added 2 commits May 31, 2026 06:37

Remove accidental modifications to unrelated files

2675216

These changes to CI workflow / infrastructure files were unrelated to the purpose of this pull request and were introduced accidentally (stale base branch). Restoring them to match `master`.

Merge remote-tracking branch 'origin/master' into work

6f81cda

alexey-milovidov removed the manual approve Manual approve required to run CI label May 31, 2026

clickhouse-gh Bot reviewed May 31, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04077_datetime32_overflow.sql

clickhouse-gh Bot reviewed May 31, 2026

View reviewed changes

Comment thread src/Storages/TTLDescription.cpp Outdated

alexey-milovidov and others added 2 commits June 7, 2026 15:10

Merge remote-tracking branch 'origin/master' into pr-101793

2e3f5ce

# Conflicts: # src/Processors/TTL/ITTLAlgorithm.cpp

clickhouse-gh Bot reviewed Jun 7, 2026

View reviewed changes

alexey-milovidov and others added 2 commits June 13, 2026 08:08

Merge remote-tracking branch 'origin/master' into pr-101793-tmp

841a50b

clickhouse-gh Bot reviewed Jun 13, 2026

View reviewed changes

Merge branch 'master' into master

b4763c5

alexey-milovidov mentioned this pull request Jun 13, 2026

Skip macOS-incompatible distributed tests 04327 and 04336 #107376

Merged

Merge remote-tracking branch 'origin/master' into pr-101793

1ced73d

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

BoloniniD commented Apr 4, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Documentation entry for user-facing changes

Uh oh!

clickhouse-gh Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

PR Metadata

Findings

Tests

Final Verdict

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BoloniniD commented Apr 20, 2026

Uh oh!

Uh oh!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PedroTadim commented Jun 3, 2026

Uh oh!

clickhouse-gh Bot Jun 7, 2026

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented Jun 30, 2026

Uh oh!

clickhouse-gh Bot commented Jun 30, 2026

LLVM Coverage Report

Uh oh!

alexey-milovidov commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BoloniniD commented Apr 4, 2026 •

edited by cursor Bot

Loading

clickhouse-gh Bot commented Apr 10, 2026 •

edited

Loading

alexey-milovidov commented Jun 30, 2026 •

edited

Loading