Propagate non-type exceptions from FunctionDynamicAdaptor by groeneai · Pull Request #121280 · ClickHouse/ClickHouse · GitHub

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):

Fixed FunctionDynamicAdaptor reporting LOGICAL_ERROR for a function over a Dynamic column when an unrelated exception, such as MEMORY_LIMIT_EXCEEDED, was raised while converting the nested result to the declared result type. Such exceptions now keep their own error code, message and stack instead of being reported as an internal error.

Description

ExecutableFunctionDynamicAdaptor::executeImpl calls castColumn to reconcile a nested function's result type with the one it declares for Dynamic arguments. Three of those calls wrapped every DB::Exception the cast could raise into LOGICAL_ERROR without inspecting e.code(). That check runs in the Exception constructor, so no caller can intercept it: a debug or sanitizer server dies, and a release build reports code 49 for what is code 241, burying the OOM signal.

Observed on master in Stress test (arm_asan_ubsan) for a1cfa11 (report), where an injected memory-tracker fault surfaced as:

Logical error: 'Cannot convert nested result of function upper with type String to the
expected result type FixedString(1048577): Query memory tracker: fault injected. ...'

That message prints its two type names in swapped slots: the cast runs FixedString(1048577) to String.

#102855 fixed the identical defect in the sibling FunctionVariantAdaptor, guarding all seven of its cast sites. This applies that guard verbatim to the three the Dynamic adaptor still has unguarded, on master and on every release branch from 24.10 to 26.9.

Nothing is downgraded: the four type-conversion codes are still relabelled, so a value-dependent failure reporting one still becomes LOGICAL_ERROR. Only the first site is observed in CI; the other two are fixed for consistency, and the four castColumn calls that carry no relabel are left alone, since wrapping them would add abort paths.

New test 05233_dynamic_adaptor_propagate_exceptions covers all three sites with a real max_memory_usage aimed inside the cast; each case reproduces the message above on an unfixed binary.

CIDB: the two nested-exception classes behind this shared message (90 days)

SELECT multiIf(position(test_context_raw, 'fault injected') > 0
            OR position(test_context_raw, 'memory limit exceeded') > 0,
               'RESOURCE: memory', 'TYPE: illegal type') AS nested_class,
       count() AS rows_,
       countIf(head_ref = 'master' AND pull_request_number = 0) AS true_master,
       uniqExactIf(pull_request_number, pull_request_number > 0) AS distinct_prs,
       min(check_start_time) AS first_seen, max(check_start_time) AS last_seen
FROM default.checks
WHERE test_name LIKE '%Cannot convert nested result of function%'
  AND test_status IN ('FAIL', 'ERROR')
  AND check_start_time > now() - INTERVAL 90 DAY
GROUP BY nested_class ORDER BY rows_ DESC

nested_class        rows_  true_master  distinct_prs  first_seen           last_seen
TYPE: illegal type  95     5            78            2026-07-08 14:17:56  2026-09-14 20:46:37
RESOURCE: memory    1      1            0             2026-09-21 02:36:11  2026-09-21 02:36:11

The TYPE class is the JSON_EXISTS-over-Dynamic divergence that #109944 fixed at the call site;
it is still arriving, which is why the LOGICAL_ERROR relabel is narrowed here rather than removed.
The RESOURCE row is this defect.

Workflow [PR]
Sync PR [sync-upstream/pr/121280]

ExecutableFunctionDynamicAdaptor::executeImpl reconciles a nested function's actual result type with the type it declares for Dynamic arguments by calling castColumn. Three of those calls wrapped every DB::Exception the cast could raise into LOGICAL_ERROR without inspecting e.code(), so a resource or cancellation error raised inside the cast was reported as an internal invariant violation: on a debug or sanitizer build the Exception constructor then reaches abortOnFailedAssertion and kills the server, and on a release build the user gets code 49 for what is actually code 241, which also buries the OOM signal. Observed on master in Stress test (arm_asan_ubsan) for commit a1cfa11, where a stress-injected memory-tracker fault surfaced as Logical error: 'Cannot convert nested result of function upper with type String to the expected result type FixedString(1048577): Query memory tracker: fault injected. ...' The same defect was fixed in the sibling FunctionVariantAdaptor by ClickHouse#102855, which guarded all seven of its cast sites. This applies that guard verbatim to the three relabel sites the Dynamic adaptor still had unguarded. Only the four type-conversion error codes are still relabelled, so the diagnostic for a real adaptor type-contract bug is preserved byte for byte, and every other exception keeps its own code, message and stack. The four remaining castColumn calls in the file (the isDynamic(result_type) paths) are deliberately left alone: they carry no relabel, so they cannot misclassify anything, and wrapping them would add new abort paths.

Internal second-model review: adjudication log (click to expand)

Pre-publication review by an independent model (engine: codex; 0 findings; coverage: all three
changed files in full, both adaptor implementations end to end, the generic function callers,
castColumn, the related tests, plus 10 runs of the new test and inspection of all three
cast-site stacks). My own cold review of the resulting code raised 3 nits, 0 blockers, 0 majors.

#	Sev	Finding	Verdict	Evidence / action
1	💡	The relabel message prints its two type names in swapped slots, so the quoted CI text reads backwards: the cast is FixedString(1048577) to String (FunctionDynamicAdaptor.cpp:191, :307, :539)	AGREE, not fixed here	Pre-existing, outside this diff, identical at the 7 sibling sites; fixing it would edit already-merged code. FunctionStringToString.h:51-65 gives the real direction, and the PR body states the swap so the quote cannot be misread.
2	💡	A value-dependent cast failure reporting one of the four type-conversion codes is still relabelled	AGREE, deliberate	castColumn has no build/exec split and those codes are polysemous, so narrowing further would drop the diagnostic for the invariant it exists to report. Not reproduced, and absent from CIDB. Stated in the PR body.
3	💡	The oracle is a memory-accounting threshold, so on a build whose accounting differs enough the limit can trip before the guarded cast and the case would pass without exercising it	AGREE, no change	It fails safe: vacuous, never red. The merged sibling 04101 carries the same property, and the three blockNumber() reference rows pin fixture drift.

Severity: ❌ blocker / ⚠️ major / 💡 nit.

Two claims I re-measured independently rather than taking from the write-up, since both decide
whether this PR is correct as scoped:

All three relabel sites, and only those, need the guard. The 4 bare castColumn calls carry
no relabel, so they cannot misclassify, and grep -rn "Cannot convert nested result" src/ hits
only these two adaptors. The codes the guard rethrows are exactly those an outer
try_build/try_execute does not swallow into an all-NULL result, so it cannot turn a loud
failure into a silent one.
The stable-release category. Per-site census on release branch 26.9: relabel sites at 185,
297 and 525, the only two bare throw; at 84 and 111 (both try_build/try_execute lambdas).
So 3 relabel sites, 0 guarded, on a shipped branch.

Session id: cron:clickhouse-review-slot-10:20260921-053006

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes, no fault injection: SELECT upper(d) FROM t SETTINGS max_memory_usage = 120000000, max_threads = 1, max_untracked_memory = 0 over a Dynamic column holding FixedString(1048577). Reproduced on every attempt across the whole verified window per site (A [70, 200] MB, B [150, 260] MB, C [80, 220] MB); outside it the limit trips before the cast.
b	Root cause explained?	FunctionStringToString declares String as its Dynamic result type but returns FixedString(N) for a FixedString(N) variant, so executeImpl legitimately calls castColumn to build a String column of the same byte width, which is the query's peak allocation. The surrounding catch (const Exception & e) did not inspect e.code() and relabelled MEMORY_LIMIT_EXCEEDED as LOGICAL_ERROR; that check runs in the Exception constructor, so no caller can intercept it.
c	Fix matches root cause?	Yes, at the throw site, the only place that still holds e.code(): rethrow unchanged unless the code is one of the four type-conversion codes. No widened bound, no size reduction, no tag, no symptom guard.
d	Test intent preserved / new tests added?	New test 05233_dynamic_adaptor_propagate_exceptions (.sql + .reference), mirroring the merged sibling 04101_variant_adaptor_propagate_exceptions: one case per relabel site asserting serverError MEMORY_LIMIT_EXCEEDED, plus a block-count pin and a positive control per fixture. No existing test weakened.
e	Both directions demonstrated?	Yes, on two Build-ID-verified binaries (unfixed b135f7d0, fixed c664e08a). Unfixed: all three cases exit 134 with Logical error: 'Cannot convert nested result of function upper ...' at FunctionDynamicAdaptor.cpp:185 / :297 / :525. Fixed: all three return code 241, the server stays up, and the whole file gives [ OK ] 0.74 sec.
f	Fix is general across code paths?	All three relabel sites are fixed in one push, not just the CIDB-observed one; the sibling FunctionVariantAdaptor was already guarded 7/7 by #102855, so every site in both files now carries a guard. The four castColumn calls that carry no relabel are deliberately left alone: wrapping them would add new abort paths.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes by construction: the guard tests only e.code(), so it is independent of type, wrapper and value (Nullable, LowCardinality, Const and Sparse are unwrapped above this frame by IFunction.cpp). The three fixtures still cover the distinct column shapes that reach these sites: a variant without NULLs, one with NULLs (observed types Nullable(...)), and multiple FixedString widths.
h	Backward compatible?	Yes. No setting, no serialization, no ABI, no header; SettingsChangesHistory.cpp correctly untouched. The only observable change is an error code becoming more accurate, from 49 to the exception's real code.
i	Invariants and contracts preserved?	Yes. The invariant is that this catch may relabel as LOGICAL_ERROR only when the exception reports the two types were not convertible; the guard states exactly that. throw; preserves the original code, message and stack instead of flattening them into e.message(). 14 insertions, 0 deletions, so the existing LOGICAL_ERROR path and its format string are unchanged. No lock, allocation or ownership contract is touched.

Session id: cron:clickhouse-impl-slot-6:20260921-041800

Workflow [PR], commit [31dae11]

Summary: ⏳

job_name	test_name	status	info	comment
Stress test (arm_tsan)		FAIL
	Segmentation fault (STID: 3422-39ed)	FAIL	cidb

AI Review

Summary

This PR narrows the three FunctionDynamicAdaptor relabel sites so that only actual type-conversion failures are rewritten to LOGICAL_ERROR, while unrelated exceptions such as MEMORY_LIMIT_EXCEEDED keep their original code, message, and stack. The changed paths match the earlier FunctionVariantAdaptor fix, and the new stateless test covers all three relabel sites with focused repro cases. I did not find a remaining correctness, safety, or evidence gap in the current diff.

Final Verdict

Status: ✅ Approve

Build profile diff (arm_release)

Comparing 31dae1165 with master de0734db6 (stripped binary size, per-symbol sizes and ThinLTO time; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

programs/clickhouse-stripped: smaller than the master baseline by the known offset between the two builds, so the difference is not shown. A delta that differs from the offset by more than 50% of it is shown, in either direction.

The official master build is compiled with -g and a pull request build is not, and XRay counts debug instructions towards its instrumentation threshold, so master instruments thousands of functions more and its binary is ~0.4% larger no matter what the pull request does.

Compile time of recompiled translation units

7 translation units recompiled, 7 s compile time in total, 7 of them have a recent master baseline.

Job report

groeneai added can be tested Allows running workflows for external contributors groeneai-origin-ci-master PR origin: master/nightly CI monitoring finding labels Sep 21, 2026

groeneai added the groeneai-origin-ci-master PR origin: master/nightly CI monitoring finding label Sep 21, 2026

clickhouse-gh Bot closed this Sep 21, 2026

clickhouse-gh Bot reopened this Sep 21, 2026

clickhouse-gh Bot closed this Sep 21, 2026

clickhouse-gh Bot reopened this Sep 21, 2026

groeneai requested review from Algunenano and Avogar September 21, 2026 06:16

clickhouse-gh Bot added pr-bugfix Pull request with bugfix, not backported by default comp-json-datatype JSON/Dynamic/Object datatype: semi-structured column storage, typed and dynamic paths, sub-object... labels Sep 21, 2026

Propagate non-type exceptions from FunctionDynamicAdaptor by groeneai · Pull Request #121280 · ClickHouse/ClickHouse · GitHub

Propagate non-type exceptions from FunctionDynamicAdaptor - #121280

Propagate non-type exceptions from FunctionDynamicAdaptor#121280
groeneai wants to merge 1 commit into
ClickHouse:masterfrom
groeneai:fix-dynamic-adaptor-propagate-non-type-exceptions

groeneai commented Sep 21, 2026 •
edited by clickhouse-gh Bot
Loading

Uh oh!

Uh oh!

groeneai commented Sep 21, 2026

Uh oh!

groeneai commented Sep 21, 2026

Uh oh!

clickhouse-gh Bot commented Sep 21, 2026 •
edited
Loading

Uh oh!

Uh oh!

clickhouse-gh Bot commented Sep 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Propagate non-type exceptions from FunctionDynamicAdaptor by groeneai · Pull Request #121280 · ClickHouse/ClickHouse · GitHub

Propagate non-type exceptions from FunctionDynamicAdaptor - #121280

Conversation

groeneai commented Sep 21, 2026 • edited by clickhouse-gh 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):

Description

Uh oh!

groeneai commented Sep 21, 2026

Uh oh!

groeneai commented Sep 21, 2026

Uh oh!

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

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

clickhouse-gh Bot commented Sep 21, 2026

Build profile diff (arm_release)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

groeneai commented Sep 21, 2026 •
edited by clickhouse-gh Bot
Loading

clickhouse-gh Bot commented Sep 21, 2026 •
edited
Loading