All committers have signed the CLA.

Workflow [PR], commit [42c2808]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR fixes the dead validation branch in executable UDF command-parameter parsing so empty or otherwise invalid placeholder names are no longer registered as command parameters, and it extends the integration test to cover the empty-name variants that were previously missing. I did not find any remaining correctness, compatibility, or test-coverage gaps in the current diff.

Final Verdict

✅ Approve. No new inline findings.

Merged fresh master into the branch (it was ~4 days / 1091 commits behind and red) to re-trigger CI. The merge was clean — no conflicts, and the net diff is unchanged (the single-character fix in ExternalUserDefinedExecutableFunctionsLoader.cpp plus the two integration-test files).

The only public-CI red was AST fuzzer (amd_debug, targeted, old_compatibility) — Logical error: Too large size (A) passed to allocator (STID 1367-1ab3). This is an unrelated, long-standing master-wide fuzzer flake: the same error fires on many unrelated PRs (#100377, #101435, #96630, #106255, …) and directly on master (pull_request_number = 0 on 2026-05-12, 2026-05-11, 2026-04-02). The stack trace is in generic pipeline execution (ExceptionKeepingTransform → PipelineExecutor), with nothing touching UDF command-parameter parsing. Tracked by #83577. The fresh CI run should clear it.

Also resolved the addressed (and now outdated) review thread about the dashed parameter name — the test config now uses a space ({test parameter:UInt64}), per the applied suggestion.

The remaining CH Inc sync failure is the internal sync check; the push re-runs it.

Re-checked the branch and the fresh CI run (commit 7ed1b6d8). The branch is 0 commits behind master, MERGEABLE, the fix is approved, all review threads are resolved, and AI Review is ✅ Approve with no required actions. The net diff is unchanged: the single-character fix (&& → ||) in ExternalUserDefinedExecutableFunctionsLoader.cpp plus the two integration-test files.

Both CI reds are unrelated, pre-existing master-wide flakes — neither touches executable-UDF command-parameter parsing:

1. Stateless tests (arm_binary, parallel) → 04204_global_in_join_max_rows_to_transfer_103333

• Failure: Code: 102. UNEXPECTED_PACKET_FROM_SERVER: Unexpected packet from server 127.0.0.2:9000 (expected TablesStatusResponse, got ProfileInfo): While executing Remote. — a transient distributed-connection protocol race, not a test-logic failure.
• The harness's own randomized-settings diagnosis re-ran the test 154 times with the same settings: Runs: 154, Failed: 0 → "All reruns passed. The failure is not reproducible (likely a transient issue)."
• The test is a GLOBAL IN / GLOBAL JOIN max_rows_to_transfer regression test — it does not involve UDFs.
• This expected TablesStatusResponse, got ProfileInfo flake is master-wide: 51 occurrences across 23 distinct tests and 44 distinct PRs in the last 30 days (CIDB), and 04204 itself failed directly on master (pull_request_number = 0) on 2026-06-29 and 2026-06-30.

2. Stress test (arm_tsan) → Hung check failed, possible deadlock found

• Tracked by the open master-wide flake issue Hung check failed, possible deadlock found #107941. The hung-check stack trace is generic query-analyzer / function-execution (QueryAnalyzer::resolveFunction, IExecutableFunction::execute), with nothing specific to UDF command-parameter parsing.

@groeneai, the 04204 red is an instance of the master-wide UNEXPECTED_PACKET_FROM_SERVER (expected TablesStatusResponse, got ProfileInfo) distributed-protocol flake (51 hits / 23 tests / 44 PRs in 30d, including master); it has no tracking issue yet. Could you investigate and provide a fix (or open a tracking issue) in a separate PR? The hung check is already tracked by #107941.

No code change is needed on this PR — it is ready to merge once CI settles.

@alexey-milovidov The 04204 red is that family, and your own #108854 already fixes it - a separate PR or tracking issue would just duplicate it.

• expected TablesStatusResponse, got ProfileInfo: 0 hits in the prior 30d, then a sharp onset ~2026-06-29 17:54 UTC (CIDB: 51 hits / 23 tests / 44 PRs, incl. master x7) - a recurrence of Connection left in a broken state (was: flaky test 03259_grouping_sets_aliases) #93018, matching your numbers.
• 04204 itself is healthy on master (2586 OK / 2 FAIL in 7d; both FAILs are this desync, not test logic), so it is purely a victim of the pool desync.
• Fix flaky distributed query "Unexpected packet from server (expected TablesStatusResponse, got ProfileInfo)" #108854 ships a deterministic regression test (04489_distributed_table_status_desync_recovery), so it covers 04204 and the other victims directly.

I am tracking #108854 to confirm the desync stops on master after it merges. Hung check is #107941. Nothing needed on this PR.

Re-checked the completed CI run on commit 7ed1b6d8. The branch is MERGEABLE, the fix is approved, all review threads are resolved, AI Review is ✅ Approve, and the net diff is unchanged: the single-character fix (&& → ||) in ExternalUserDefinedExecutableFunctionsLoader.cpp plus the two integration-test files. The branch's merge base is from today (only a handful of commits behind master), so no re-merge is needed.

All three CI reds are unrelated, pre-existing master-wide flakes — none touches executable-UDF command-parameter parsing:

1. Stateless tests (arm_binary, parallel) → 04204_global_in_join_max_rows_to_transfer_103333 — the UNEXPECTED_PACKET_FROM_SERVER (expected TablesStatusResponse, got ProfileInfo) distributed-protocol desync. The harness re-ran it 154 times with the same settings (all passed). Already covered by #108854.

2. Stress test (arm_tsan) → Hung check failed, possible deadlock found — tracked by the master-wide flake issue #107941. The hung-check stack is generic query-analyzer/function execution, with nothing specific to UDF parsing.

3. Upgrade check (amd_release) → Error message in clickhouse-server.log — this third red surfaced after my earlier triage (the job was still running then), so it was not covered above. It is also an unrelated master-wide flake:

• The flagged error is Code: 395 ... Value passed to 'throwIf' function is non-zero ... FUNCTION_THROW_IF_VALUE_IS_NON_ZERO logged repeatedly by MutatePlainMergeTreeTask / MergeTreeBackgroundExecutor for a mutation if(1, _CAST(plus(x, throwIf(1)), 'UInt64'), x).
• This is the deliberately-failing async mutation from the stateless test 02597_column_update_tricky_expression_and_replication.sql (ALTER TABLE test UPDATE d = d + throwIf(1) where 1 SETTINGS mutations_sync=0;). The test intentionally creates the failing mutation and then KILL MUTATION ... SYNCs it; the background-executor retries during that window flood clickhouse-server.log, and the upgrade-check log scanner picks them up. There is no executable-UDF code anywhere in this path.
• CIDB confirms it is master-wide: the same upgrade-check FUNCTION_THROW_IF_VALUE_IS_NON_ZERO error fired on 24 distinct, unrelated PRs in the last 7 days (e.g. add option skip_unavailable_shards_mode #79091, Fix server abort in ~CallbackRunnerTask and preserve thread name when borrowing a group #108887, Feature: Enable overlay databases for server. #86768, Allow EXISTS on a dictionary with the SHOW DICTIONARIES privilege #108084, Validate Azure Blob Storage upload settings early #103394, Add dotProductTransposed distance function for QBit #108100, Fix CREATE OR REPLACE MATERIALIZED VIEW ... POPULATE losing the source subscription #108728, Allowlist remote database engine connection errors in upgrade check #108560 …), at a steady ~3–7/day.

@groeneai, the Upgrade check (amd_release) red here is the FUNCTION_THROW_IF_VALUE_IS_NON_ZERO upgrade-log false-positive coming from the deliberately-failing throwIf mutation in 02597_column_update_tricky_expression_and_replication.sql — 24 distinct PRs in 7d, no tracking issue yet. Could you investigate and provide a fix (or open a tracking issue) in a separate PR? The 04204 desync is covered by #108854 and the hung check by #107941.

No code change is needed on this PR — it is correct, approved, and ready to merge once the flakes clear.

Merged fresh master into the branch (head 7ed1b6d8 → 42c2808dc47; it was ~1300 commits / ~1.5 days behind and red) to re-trigger CI. The merge was clean — no conflicts, and the net diff is unchanged: the single-character fix (&& → ||) in ExternalUserDefinedExecutableFunctionsLoader.cpp plus the two integration-test files.

The re-merge is worthwhile now (unlike at the time of the earlier triage) because the fixers for the three flake reds have since landed in master:

• Stateless (arm_binary) → 04204 UNEXPECTED_PACKET_FROM_SERVER (TablesStatusResponse/ProfileInfo desync) — fixed by Fix flaky distributed query "Unexpected packet from server (expected TablesStatusResponse, got ProfileInfo)" #108854 (merged 2026-07-01).
• Upgrade check (amd_release) → FUNCTION_THROW_IF_VALUE_IS_NON_ZERO — the deliberately-failing throwIf mutation from 02597_column_update_tricky_expression_and_replication.sql is now allowlisted generically in tests/docker_scripts/upgrade_runner.sh (grep -av -e "Value passed to 'throwIf' function is non-zero") via Allowlist throwIf upgrade-check errors from 04341_broken_mutation_part_log_flush #108856 and Upgrade check: allowlist the expected toInt8('fail') mutation error from 01155_old_mutation_parts_to_do #108835 (both merged 2026-07-01).

So the fresh run should clear reds #1 and #3 deterministically. The remaining possibility is the generic Stress (arm_tsan) hung-check flake tracked by #107941, which is unrelated to executable-UDF parsing and typically does not recur on re-run.

No code change is needed — the fix is correct, approved, and ready to merge once CI settles.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 0/5 (0.00%) · Uncovered code

Full report · Diff report