iframe-proxy

groeneai · 2026-05-18T16:19:33Z

Related: #95839

When arrayRemove's first argument is an Array(Variant(...)) whose Variant alternatives are all incompatible with the type of the second argument and variant_throw_on_type_mismatch = 0, equals returns Nullable(Nothing) instead of throwing. The previous code stripped the Nullable and then declared the resulting ColumnNothing as having type UInt8 when handing it to FunctionIf. The UInt8/UInt8 typed paths inside if did not match the actual column class, dispatch fell into executeGeneric, and result_column->insertFrom(*col_else, i) then tripped the assertTypeEquality chassert (typeid(ColumnUInt8) != typeid(ColumnNothing)) inside IColumn::insertFrom.

Detect the type mismatch right after removeNullable: when the column's underlying type is not UInt8, no real equality comparison was possible, so substitute a constant-zero UInt8 column. The downstream if then yields the correct filter (no element of arr is considered equal to elem) and arrayRemove returns the array unchanged.

Reproducer (STID 2508-1fb5, hit on PR #94517 since 2026-05-04):

SET variant_throw_on_type_mismatch = 0;
SELECT arrayRemove([NULL, [257, 65537]], [['hello']]);
-- before: Logical error 'assertTypeEquality' in IColumn::insertFrom
-- after : [NULL,[257,65537]]

This also covers the multi-alternative Variant query reported on issue #95839 (verified locally: the query aborts with the STID 2508-1fb5 assertion on master and returns a clean result with this fix):

SET variant_throw_on_type_mismatch = 0;
SELECT arrayRemove([[9223372036854775807, -9223372036854775808], [0.9999, 7]], [['hello']]);
-- before: Logical error '(isConst() || ...) ? getDataType() == rhs.getDataType() : typeid(*this) == typeid(rhs)'
-- after : [[9223372036854775807,-9223372036854775808],[0.9999,7]]

Follow-up to #101105 (which widened assertTypeEquality to handle wrapper columns on rhs); this PR closes the remaining call-site where arrayRemove was producing a column whose runtime type did not match its declared type.

Discovered via @alexey-milovidov directive on #94517 (2026-05-18T15:08:20Z), STID 2508-1fb5; re-confirmed on #94517 (2026-06-13) and tracked by issue #95839.

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

Fix a logical error in arrayRemove when the first argument is an array of Variant whose alternatives are all incompatible with the type of the second argument and variant_throw_on_type_mismatch is disabled. The function now treats the comparison as "never equal" and returns the array unchanged instead of triggering a server-side assertTypeEquality failure.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

Version info

Merged into: 26.6.1.768

When `arrayRemove`'s first argument is an `Array(Variant(...))` whose Variant alternatives are all incompatible with the type of the second argument AND `variant_throw_on_type_mismatch = 0`, `equals` returns `Nullable(Nothing)` instead of throwing. After `removeNullable` we therefore had a `ColumnNothing` that `arrayRemove` then wrapped under a declared type of `UInt8` and passed to the `if` resolver. `FunctionIf::executeImpl` dispatched the `UInt8/UInt8` typed paths, failed to match the actual `ColumnNothing` column, fell into `executeGeneric`, and tripped the `assertTypeEquality` chassert inside `IColumn::insertFrom` (`typeid(ColumnUInt8) != typeid(ColumnNothing)`). Detect this mismatch right after the `removeNullable`: when the column's underlying type is not `UInt8`, no real equality comparison was possible, so substitute a constant-zero `UInt8` column. The downstream `if` then computes the correct filter (no element of `arr` is considered equal to `elem`) and `arrayRemove` returns the array unchanged. Reproducer (STID 2508-1fb5, hit on PR ClickHouse#94517 since 2026-05-04): ``` SET variant_throw_on_type_mismatch = 0; SELECT arrayRemove([NULL, [257, 65537]], [['hello']]); -- before: server logical error -- after : [NULL,[257,65537]] ``` Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

groeneai · 2026-05-18T16:20:03Z

Pre-PR Validation Gate

(a) Deterministic repro? Yes — SET variant_throw_on_type_mismatch = 0; SELECT arrayRemove([NULL, [257, 65537]], [['hello']]) deterministically aborts on every run before the fix and returns [NULL,[257,65537]] after the fix.

(b) Root cause explained? Yes — [NULL, [257, 65537]] resolves to Array(Variant(Array(UInt32))). Inside arrayRemove's nullable branch we call equals(Variant(Array(UInt32)), Array(Array(String))). With variant_throw_on_type_mismatch = 0, FunctionVariantAdaptor returns Nullable(Nothing) (see src/Functions/FunctionVariantAdaptor.cpp:746-759) instead of throwing. The code then did removeNullable(equals_result->convertToFullIfNeeded()) and wrapped the resulting ColumnNothing under a declared type of UInt8. FunctionIf::executeImpl saw declared UInt8/UInt8, failed to match the actual ColumnNothing in the typed dispatch, fell through to executeGeneric (src/Functions/if.cpp:1474), and on result_column->insertFrom(*col_else, i) (src/Functions/if.cpp:904) tripped assertTypeEquality (src/Columns/IColumn.h:870) because typeid(ColumnUInt8) != typeid(ColumnNothing) and neither side is Const/Sparse/Replicated.

(c) Fix matches root cause? Yes — the fix sits exactly at the point where the type lie was introduced. Right after removeNullable, we check else_col->getDataType() != TypeIndex::UInt8 and replace the column with a constant-zero UInt8 of the correct row count. ColumnSparse::getDataType() delegates to the wrapped column, so a ColumnSparse(ColumnUInt8) is still treated as UInt8 and not replaced. This is not a defensive guard against the symptom — it stops the ColumnNothing from being mis-declared as UInt8 upstream of if.

(d) Test intent preserved? / New tests added? Yes — the existing arrayRemove tests (03707_function_array_remove, 04049_arrayRemove_nullable_tuple) and the sibling assertTypeEquality regression test (04106_assert_type_equality_column_replicated from #101105) all keep passing (60/60 across 20 runs). The new test 04256_arrayRemove_variant_no_throw_on_mismatch.sql exercises the previously-crashing path under variant_throw_on_type_mismatch = 0 and also asserts that the default (= 1) still throws ILLEGAL_TYPE_OF_ARGUMENT, so we don't silently turn a real error into a no-op.

(e) Both directions demonstrated? Yes — locally reproduced the chassert with variant_throw_on_type_mismatch = 0; SELECT arrayRemove([NULL, [257, 65537]], [['hello']]) (server abort with the exact stack trace from CIDB STID 2508-1fb5), then verified the same query returns [NULL,[257,65537]] after the fix. Existing arrayRemove semantics unchanged for all non-Variant inputs.

(f) Fix is general, not a narrow patch? Two checks:

Sibling paths in arrayRemove: the onlyNull branch (elem_type->onlyNull()) only uses is_null/not, which always return UInt8. The non-nullable branch (!canContainNull(arr) && !canContainNull(elem)) uses not_equals directly into filter_col, and the existing checkAndGetColumn<ColumnUInt8> at line 177 already converts a non-UInt8 filter into a clean LOGICAL_ERROR rather than a chassert. So only the nullable if-branch was vulnerable.
Upstream check: the root issue is the FunctionVariantAdaptor Nullable(Nothing) return type when variant_throw_on_type_mismatch = 0. Other BuildAndExecuteFunction-style callers using a hardcoded UInt8 declared type for equals could in principle have the same issue, but arrayRemove is the only such call site in src/Functions/array/.

Session: cron:clickhouse-ci-task-worker:20260518-153000

groeneai · 2026-05-18T16:20:46Z

cc @rschu1ze @Avogar — could you review this? Fix for STID 2508-1fb5 (an arrayRemove/FunctionIf/assertTypeEquality chassert that fires when an Array(Variant(...)) is compared with an incompatible second argument while variant_throw_on_type_mismatch = 0). It's the follow-up to #101105 that @alexey-milovidov asked for on #94517. Touches src/Functions/array/arrayRemove.cpp and adds a new 04256_* stateless test.

clickhouse-gh · 2026-05-18T16:36:56Z

Workflow [PR], commit [1fcf1c2]

Summary: ✅

Performance Comparison: Performance dashboard

AI Review

Summary

This PR fixes arrayRemove for impossible Variant comparisons when variant_throw_on_type_mismatch is disabled by translating the expected Nullable(Nothing)/ColumnNothing comparison result into a false UInt8 filter, while preserving a LOGICAL_ERROR for any other unexpected non-UInt8 result. The added stateless test covers the original [NULL, X] trigger, the multi-alternative Variant case from #95839, and the default throwing behavior. I found no remaining correctness issues.

Final Verdict

Status: ✅ Approve

groeneai · 2026-06-04T21:50:52Z

CI ledger — covered SHA: `2006f54cbb3c41b600ae7e1b9faf259ec095c157`

CI fully finished (Mergeable Check completed; 0 pending checks).

Check	Test	Disposition
`Unit tests (asan_ubsan)`	(check-level)	infra/chronic UBSan UntrackedMemoryCounter — task 2026-06-03-ci-p1-chronic-ubsan-in-untrackedmemoryc
`Stateless tests (amd_tsan, parallel, 1/2)`	(check-level, no test_name in CIDB)	likely the obfuscator chronic flaky pattern; no specific PR-caused failures captured.

No PR-caused failures captured. Awaiting human review.

Session: cron:our-pr-ci-monitor:20260604-213000

antaljanosbenjamin

Good bot.

…House#95839 Extend the 04256 regression test with the exact query reported on issue ClickHouse#95839 (STID 2508-1fb5): arrayRemove over a multi-alternative Array(Variant(Array(Int64), Array(Float64))) with no NULL element, paired with Array(Array(String)). This exercises the same assertTypeEquality chassert path as the existing [NULL, X] cases but via a distinct Variant-construction trigger, guarding against the regression the arrayRemove fix addresses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-13T05:32:44Z

Pre-PR validation gate (commit `c8f5c4e` — added #95839 regression query)

#	Question	Answer
a	Deterministic repro?	Yes. `SET variant_throw_on_type_mismatch=0; SELECT arrayRemove([[9223372036854775807, -9223372036854775808], [0.9999, 7]], [['hello']]);` aborts deterministically on a debug binary without the fix (built locally from `master` arrayRemove.cpp).
b	Root cause explained?	`[[...Int64...], [...Float64...]]` builds `Array(Variant(Array(Int64), Array(Float64)))`; none of the Variant alternatives is compatible with `Array(Array(String))`. Under `variant_throw_on_type_mismatch=0`, `equals` returns `Nullable(Nothing)` instead of throwing; the old code stripped `Nullable` to a `ColumnNothing` but declared it `UInt8`, so `FunctionIf::executeGeneric` -> `IColumn::insertFrom` tripped the `assertTypeEquality` chassert (STID 2508-1fb5, issue #95839). Identical mechanism to the existing `[NULL, X]` cases, different Variant-construction trigger.
c	Fix matches root cause?	Unchanged from this PR's existing fix (substitute a constant-zero `UInt8` column when the post-`removeNullable` column is not `UInt8`). This commit only adds a regression query that exercises the same code path.
d	Test intent preserved / new tests added?	Added `alexey-milovidov`'s #95839 query to `04256_arrayRemove_variant_no_throw_on_mismatch.sql` (+1 reference line). It is a distinct multi-alternative-Variant, no-NULL trigger not previously covered by the `[NULL, X]` cases. Existing assertions untouched.
e	Both directions demonstrated?	Yes. Without fix (master arrayRemove.cpp, debug build): SIGABRT with `'(isConst() \|\| ...) ? getDataType() == rhs.getDataType() : typeid(*this) == typeid(rhs)'` via `assertTypeEquality` -> `insertFrom` -> `FunctionIf` -> `arrayRemove.cpp:25`. With fix: returns `[[9223372036854775807,-9223372036854775808],[0.9999,7]]`. Test runner: 30/30 OK with randomization.
f	Fix is general, not a narrow patch?	The fix already guards the single `arrayRemove` call-site that produced a column whose runtime type disagreed with its declared type. This commit widens test coverage (two distinct Variant-construction triggers now exercised); no new sibling code path identified for `arrayRemove`.

Session id: cron:clickhouse-worker-slot-0:20260613-050800

groeneai · 2026-06-13T10:16:04Z

CI fully finished on HEAD c8f5c4e0660ad27d0509192338a1e38e4dcad792 (commit added the issue #95839 regression query + em-dash cleanup). All checks complete (Finish Workflow passed; only CH Inc sync / Mergeable Check reflect the items below). Triaged every failure; none is caused by this PR (diff is src/Functions/array/arrayRemove.cpp + the 04256 stateless test only):

Fast test (arm_darwin) — 04336_parallel_blocks_marshalling_low_cardinality_native_format + 04327_rewrite_aggregate_function_with_if_distributed. Chronic arm_darwin infra flake (connect-timeout / ConnectionPool warning, return code 23); 60 and 21 unrelated PRs in 30d, 0 master, both tests unrelated to arrayRemove. Tracked separately.
Stress test (arm_tsan) — future_error "The associated promise has been destructed" (STID 2508-38c6). A pre-existing trunk teardown race in threadPoolCallbackRunnerUnsafe (a dropped-on-shutdown packaged_task), independent of this PR (also fired on master via serverfuzz). A root-cause fix is already in flight in Fix broken-promise server abort in threadPoolCallbackRunnerUnsafe on shutdown #107383.
CH Inc sync / Mergeable Check — aggregators reflecting the above; no Fast (real test) / Bugfix / Style failure attributable to this PR.

The arrayRemove fix and both-directions validation are unchanged from the prior clean CI runs.

The recovery added for the Variant no-throw-on-mismatch case caught any non-UInt8 result from equals() and silently turned it into an all-zero filter, which would make arrayRemove quietly keep every element if equals() ever returned an unexpected type, masking a broken comparison contract. Restrict the constant-zero substitution to TypeIndex::Nothing (the documented Nullable(Nothing) result of an impossible Variant comparison with variant_throw_on_type_mismatch=0) and throw LOGICAL_ERROR for any other non-UInt8 result so a genuine contract violation surfaces instead of being hidden. Addresses clickhouse-gh[bot] review on PR ClickHouse#105248. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-13T13:26:33Z

Pre-PR validation gate (commit `1fcf1c2b163`)

#	Question	Answer
a	Deterministic repro?	Yes. With the previous broad branch removed, the bot's concern (silent all-zero filter for any non-UInt8 `equals` result) is reproduced by reasoning about the contract; the two known `Nothing`-producing repros (`SET variant_throw_on_type_mismatch=0; SELECT arrayRemove([NULL,[257,65537]],[['hello']])` and `SELECT arrayRemove([[9223372036854775807,-9223372036854775808],[0.9999,7]],[['hello']])`) deterministically take the `TypeIndex::Nothing` branch.
b	Root cause explained?	The recovery branch was too broad: `if (else_col->getDataType() != TypeIndex::UInt8)` converted any non-UInt8 `equals` result into a constant-zero filter. Only `Nullable(Nothing)` (impossible Variant comparison with `variant_throw_on_type_mismatch=0`, stripped to `ColumnNothing` by `removeNullable`) is a legitimate recovery case; any other non-UInt8 type would be a broken `equals` contract that the code silently swallowed (keeping every element).
c	Fix matches root cause?	Yes. The substitution is now gated on `else_col->getDataType() == TypeIndex::Nothing`; any other non-UInt8 result throws `LOGICAL_ERROR` instead of being masked. This is exactly the bot's request.
d	Test intent preserved / new tests added?	Preserved. Existing test `04256_arrayRemove_variant_no_throw_on_mismatch` (incl. the #95839 multi-alternative-Variant case) continues to exercise the `Nothing` recovery path; no assertions weakened. The new `LOGICAL_ERROR` branch is unreachable under any normal `equals` result, so no new test is added for it (it is a defensive contract guard, not a user-facing behavior).
e	Both directions demonstrated?	Yes. Built debug binary (Build ID `e983a5c...`). `clickhouse local`: the two `Nothing`-path repros return the array unchanged; default `variant_throw_on_type_mismatch=1` throws clean `ILLEGAL_TYPE_OF_ARGUMENT` (no abort); ordinary `arrayRemove` cases unaffected. Test runner: `04256` 30/30 OK with randomization; `arrayRemove`/`assertTypeEquality`/`variant_throw` suites 15/15 OK.
f	Fix is general, not a narrow patch?	Yes. This narrows a recovery branch rather than guarding a symptom: the legitimate recovery (`Nothing`) is handled and every other deviation now surfaces as `LOGICAL_ERROR` at the single point where `arrayRemove` consumes the `equals` result. No sibling `arrayRemove` path builds an `if`-filter this way.

Session id: cron:clickhouse-worker-slot-3:20260613-131600

clickhouse-gh · 2026-06-13T18:21:46Z

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 6/8 (75.00%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

groeneai mentioned this pull request May 18, 2026

Enable optimize_or_like_chain by default. #94517

Draft

nikitamikhaylov added the can be tested Allows running workflows for external contributors label May 18, 2026

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

groeneai mentioned this pull request Jun 2, 2026

Support constant haystack with non-constant needle in LIKE/ILIKE/match #100479

Merged

1 task

Merge commit '3fa1ed2d9d9cc4f472cd43e2e4eaabac12ec36db' into HEAD

2006f54

antaljanosbenjamin self-assigned this Jun 5, 2026

antaljanosbenjamin approved these changes Jun 5, 2026

View reviewed changes

antaljanosbenjamin reviewed Jun 5, 2026

View reviewed changes

Merge branch 'master' into groeneai/fix-2508-1fb5-arrayremove-variant

fd87fdd

alexey-milovidov mentioned this pull request Jun 13, 2026

Skip macOS-incompatible distributed tests 04327 and 04336 #107376

Merged

clickhouse-gh Bot reviewed Jun 13, 2026

View reviewed changes

Comment thread src/Functions/array/arrayRemove.cpp Outdated

groeneai mentioned this pull request Jun 13, 2026

Fix data loss in REPLACE PARTITION from a source with no parts (#23727) #104939

Merged

1 task

alexey-milovidov added this pull request to the merge queue Jun 13, 2026

Merged via the queue into ClickHouse:master with commit 8427ed3 Jun 13, 2026
166 checks passed

robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 13, 2026

alexey-milovidov mentioned this pull request Jun 14, 2026

Filesystem cache: encode segment size in the cache file name #107415

Open

1 task

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix arrayRemove logical error when comparing incompatible Variant types#105248

Fix arrayRemove logical error when comparing incompatible Variant types#105248
alexey-milovidov merged 5 commits into
ClickHouse:masterfrom
groeneai:groeneai/fix-2508-1fb5-arrayremove-variant

groeneai commented May 18, 2026 •

edited by robot-clickhouse

Loading

Uh oh!

groeneai commented May 18, 2026

Uh oh!

groeneai commented May 18, 2026

Uh oh!

clickhouse-gh Bot commented May 18, 2026 •

edited

Loading

Uh oh!

groeneai commented Jun 4, 2026

Uh oh!

antaljanosbenjamin left a comment

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

clickhouse-gh Bot commented Jun 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

groeneai commented May 18, 2026 • edited by robot-clickhouse 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

Version info

Uh oh!

groeneai commented May 18, 2026

Pre-PR Validation Gate

Uh oh!

groeneai commented May 18, 2026

Uh oh!

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

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

groeneai commented Jun 4, 2026

CI ledger — covered SHA: 2006f54cbb3c41b600ae7e1b9faf259ec095c157

Uh oh!

antaljanosbenjamin left a comment

Choose a reason for hiding this comment

Uh oh!

groeneai commented Jun 13, 2026

Pre-PR validation gate (commit c8f5c4e — added #95839 regression query)

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

Uh oh!

groeneai commented Jun 13, 2026

Pre-PR validation gate (commit 1fcf1c2b163)

Uh oh!

clickhouse-gh Bot commented Jun 13, 2026

LLVM Coverage Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

groeneai commented May 18, 2026 •

edited by robot-clickhouse

Loading

clickhouse-gh Bot commented May 18, 2026 •

edited

Loading

CI ledger — covered SHA: `2006f54cbb3c41b600ae7e1b9faf259ec095c157`

Pre-PR validation gate (commit `c8f5c4e` — added #95839 regression query)

Pre-PR validation gate (commit `1fcf1c2b163`)