fix(insights): stop toDateTime re-parsing an already-parsed datetime

[sampennington]

Problem

Insight queries (Trends, Funnels, etc.) using a DateTime-typed property crashed the whole query with ClickHouse A value of illegal type ... to function 'parseDateTime64BestEffortOrNull'. Expected: String, got: DateTime64. This was one of the largest deterministic query-builder failures in the Product analytics — insight query failures triage — part of the ongoing effort to reduce deterministic query-builder bugs.

The property-type swapper wraps a DateTime property in toDateTime() to coerce the stored string. When an enclosing toDateTime() also wrapped that value, the printer couldn't tell the inner result was already a datetime — it resolved the wrapped value's type through an Alias whose declared type was stale (still String, from before the swapper rewrote the alias's inner expression). So it emitted parseDateTime64BestEffortOrNull on a value that was already a DateTime64, which ClickHouse rejects.

Changes

• The property-type swapper now sets a CallType (returning DateTimeType) on the toDateTime() call it synthesizes for DateTime properties — previously the call was untyped.
• The ClickHouse printer looks through Alias nodes when resolving a function's overload, so a stale alias type no longer hides the real expression type. This makes toDateTime()'s DateTime → toDateTime overload fire instead of falling back to the string-parse default.

How did you test this code?

I'm an agent. Automated tests run locally:

• New regression test test_to_datetime_does_not_double_parse_datetime_property in posthog/hogql/printer/test/test_printer.py, reproducing the double-parse and asserting it no longer happens.
• Full test_printer.py + test_property_types.py suites — 649 passed, 184 snapshots unchanged.

Publish to changelog?

no

🤖 Agent context

Authored by Claude Code (Opus 4.7). Found via system.query_log analysis of failing product-analytics insight queries (exception_code = 43, illegal-type on parseDateTime64BestEffortOrNull).

Decisions:

• Two root causes had to be fixed together: the swapper produced an untyped toDateTime() call, and even with a type the printer read it through a stale Alias type. Fixing only one leaves the bug.
• Chose to look through aliases generically in the overload resolver (not just for toDateTime) — an alias's declared type being stale after a transform rewrites its inner expression is a general hazard, and resolving against the real expression is strictly more correct.
• A separate, rarer variant (parseDateTime64BestEffortOrNull on a DateTime column reached through a subquery, seen in HogQLQuery rather than insight queries) is not addressed here and may need follow-up.

Agent-authored; requires human review.

[github-actions]

🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down.

Add the run-playwright label if you want an E2E sweep before merging — CI will pick it up automatically.

Most PRs don't need this. Real regressions still get caught on master and fix-forward.

[sampennington]

Automated code review

Reviewed the diff against posthog/hogql/printer/clickhouse.py, posthog/hogql/transforms/property_types.py, and the new test. The root-cause analysis is sound and the two-part fix (typed synthetic call + alias-aware overload resolution) addresses both halves of the bug correctly.

Correctness

• property_types.py:574-585 — Setting CallType(return_type=DateTimeType(nullable=True)) on the synthesized toDateTime() is the right call. CallType.resolve_constant_type just returns return_type (ast.py:611-612), so an enclosing toDateTime() now sees a DateTimeType first-arg and the ((DateTimeType, DateType, IntegerType), "toDateTime") overload (conversions.py:119-120) fires instead of the parseDateTime64BestEffortOrNull default. Good.
• clickhouse.py:186-194 — Looking through Alias nodes during overload resolution is generically more correct: a stale alias type should never mask the real expression type. The while loop handles nested aliases and the None guards are preserved.
• The alias-unwrapping only affects the func_meta.overloads branch. The later constant-folding optimization (isinstance(node.args[0], Constant) and isinstance(node.args[0].type, StringType)) still reads node.args[0] directly — correct, since a constant is never wrapped in an alias here, so no behavior change there.

Points worth confirming

• Hardcoded arg_types=[ast.StringType(nullable=True)] — the synthesized CallType.arg_types claims a single nullable-string arg. The wrapped node is a Field to a property whose stored value is a string, so this is accurate in practice, but arg_types is otherwise unused by the printer here (only return_type matters). Fine as-is; just descriptive rather than load-bearing.
• Nullability — DateTimeType(nullable=True) is the safe choice (parseDateTime64BestEffortOrNull is nullable). No concern.
• Scope — the PR body already notes the rarer subquery/HogQLQuery variant of the same illegal-type error is not covered. Good that it is called out; filing a follow-up issue would keep it from being lost.

Tests

• test_to_datetime_does_not_double_parse_datetime_property reproduces the exact failure path and asserts parseDateTime64BestEffortOrNull appears exactly once. It would fail on master (count == 2), so it is a genuine regression guard.
• Suggestion (non-blocking): also assert the printed SQL contains a toDateTime( call for the inner property, to pin that the overload actually resolved — as written, a future change that dropped both parses entirely would still pass.
• Consider one parameterized case for a DateTime person or group property, since _field_type_to_property_call is shared across all three property scopes — cheap extra coverage for the same code path.

Style / maintainability

• Comments on both hunks explain the why concisely — appropriate use.
• No convention issues; matches existing HogQL AST/printer patterns.

Verdict: Ship it — minor test-strengthening suggestions are non-blocking.