Extend cast_keep_nullable to work with Dynamic/JSON types

[seva-potapov]

Changelog category (leave one):

• Improvement

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

Extend cast_keep_nullable to work with Dynamic/JSON types. When set, casting NULL from types that can be Nullable will return NULL, otherwise NULL will throw CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN error.

Details

A customer reported two inconsistencies when casting JSON null values:

1. Query select v.a::Array(String) from (select '{"a":null}'::JSON as v) settings cast_keep_nullable=1 returns [] instead of signaling an error.
   The user set cast_keep_nullable=1 explicitly requesting null preservation, but the system silently swallowed the null into a default value.

2. Direct cast Nullable(String) NULL to Array(String) throws CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN, but when casting Nullable(String) NULL to Dynamic to Array(String) silently returns []. The same null value produces different results depending on the cast path.

Root cause: cast_keep_nullable only checks isNullable() on the source type. Dynamic (used by JSON) stores nulls via an internal discriminator, not the Nullable wrapper, so the setting has no effect. And ConvertImplFromDynamicToColumn unconditionally calls insertDefault() for null values regardless of any settings.

---

Note

Medium Risk
Changes CAST/:: behavior for Dynamic/JSON sources when cast_keep_nullable=1, potentially turning previously silent NULL-to-default conversions into CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN errors for non-nullable targets.

Overview
Extends cast_keep_nullable handling to Dynamic/JSON casts so that NULL preservation is enforced consistently.

When cast_keep_nullable=1, casting Dynamic-backed NULLs now returns NULL only for targets that can be nullable, and throws CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN when converting NULL into non-Nullable targets (instead of inserting defaults like []). Adds a dedicated stateless test to cover the new Dynamic/JSON null-casting behavior and backward-compatibility when the setting is off.

^{Written by Cursor Bugbot for commit c49951291f45693c333945271b91e6286ee36e1e. This will update automatically on new commits. Configure here.}

[clickhouse-gh]

Workflow [PR], commit [c76a7f5]

Summary: ✅

---

AI Review

Summary

This PR extends cast_keep_nullable handling for Dynamic/JSON casts and adds a stateless regression test. High-level verdict: there is one blocker in NULL-capability detection for target types. The new throw_on_null predicate treats Variant/Dynamic targets as non-null-capable because it relies on canBeInsideNullable, which is not the right semantic check for these types.

Missing context

• ⚠️ No CI logs/results were provided in this review context.

Findings

• ❌ Blockers
  • [src/Functions/FunctionsConversion.cpp:1481, src/Functions/FunctionsConversion.h:3219] throw_on_null is enabled using !result_type->canBeInsideNullable(), which incorrectly classifies Variant and Dynamic as unable to represent NULL. With cast_keep_nullable=1, Dynamic(NULL) -> Variant(...) can incorrectly throw CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN instead of preserving NULL in the target type.
  • Suggested fix: use a semantic NULL-capability check (!canContainNull(*result_type)) instead of !canBeInsideNullable(), and keep the condition consistent in both call sites.

Tests

• ⚠️ Add regression coverage for Dynamic(NULL) -> Variant(...) with cast_keep_nullable=1 (must not throw) and a non-nullable target that truly cannot contain NULL (must throw), to lock the intended boundary.

ClickHouse Rules

Item	Status	Notes
Deletion logging	➖
Serialization versioning	➖
Core-area scrutiny	✅
No test removal	✅
Experimental gate	➖
No magic constants	✅
Backward compatibility	⚠️	Potential behavior change for Dynamic(NULL) -> Variant(...) with cast_keep_nullable=1.
SettingsChangesHistory.cpp	➖
PR metadata quality	✅
Safe rollout	⚠️	Incorrect NULL-capability predicate risks surprising cast exceptions in rollout.
Compilation time	✅

Performance & Safety

• The concern is semantic safety/correctness: NULL handling can become unexpectedly stricter for targets that are actually NULL-capable, causing user-visible exceptions.

Final Verdict

• Status: ⚠️ Request changes
• Minimum required actions:
  • Replace throw_on_null predicate with a NULL-capability check (e.g. !canContainNull(*result_type)) in both affected call sites.
  • Add regression tests for Dynamic(NULL) cast to Variant(...) under cast_keep_nullable=1.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

[alexey-milovidov]

Variant also supports NULLs.

[alexey-milovidov]

I remember we have some method, e.g., canContainNulls

[seva-potapov]

yes, my previous attempt switched to canContainNulls but then it became overcomplicated and testing casting Variant to Columns became impossible to do. see the difference: https://github.com/ClickHouse/ClickHouse/compare/417dcb633628a4d47c4d688e3d1e65acdec030d8..52cbccb777427f6d03d9e98a9bbf5357eeb1eb9d

[alexey-milovidov]

Tests please.

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.10%	84.10%	+0.00%
Functions	24.50%	24.30%	-0.20%
Branches	76.60%	76.60%	+0.00%

PR changed lines: PR changed-lines coverage: 93.18% (41/44, 0 noise lines excluded)
Diff coverage report
Uncovered code