Skip NULL elements in text index over Array(LowCardinality(Nullable))

[groeneai]

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 NOT_IMPLEMENTED error (Method getDataAt is not supported for Nullable(String) in case if value is NULL) when a text index is built on Array(LowCardinality(Nullable(String))) (including Nested fields stored that way) and an indexed array contains a NULL element. NULL array elements are now skipped during index construction, matching Array(Nullable(String)).

Description

Closes: #110039

The array branch of MergeTreeIndexAggregatorText::update detected element nullability with IColumn::isNullable(), which returns false for ColumnLowCardinality(Nullable). As a result getDataAt() was called on a NULL array element and threw Code: 48 NOT_IMPLEMENTED. This failed the INSERT, and when data already existed it left a permanently retrying MATERIALIZE INDEX mutation and wedged merges/OPTIMIZE (which rebuild the index) until the index was dropped.

Array(Nullable(String)) and scalar LowCardinality(Nullable(String)) were unaffected; the scalar branch already uses isColumnNullableOrLowCardinalityNullable(). The fix uses the same helper in the array branch so NULL LowCardinality(Nullable) elements are skipped, on INSERT, MATERIALIZE INDEX, and merge/OPTIMIZE rebuilds alike.

Regression test 04401_text_index_array_lowcardinality_nullable covers both fiddle cases from the issue (the direct Array(LowCardinality(Nullable(String))) and Nested forms) plus the MATERIALIZE INDEX and OPTIMIZE FINAL merge-rebuild paths.

Version info

• Merged into: 26.7.1.813 (included in 26.7 and later)

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. CREATE TABLE t (id UInt64, a Array(LowCardinality(Nullable(String))), INDEX ix a TYPE text(tokenizer='array')) ENGINE=MergeTree ORDER BY id; INSERT INTO t VALUES (1,[NULL]); fails 100% with Code: 48 NOT_IMPLEMENTED on the unpatched binary.
b	Root cause explained?	The array branch of MergeTreeIndexAggregatorText::update uses IColumn::isNullable(), which returns false for ColumnLowCardinality(Nullable), so data_is_nullable is false, the null-skip is bypassed, and getDataAt() is called on a NULL element → ColumnNullable::getDataAt throws NOT_IMPLEMENTED.
c	Fix matches root cause?	Yes. Replaces column_data.isNullable() with isColumnNullableOrLowCardinalityNullable(column_data) — the exact predicate the scalar branch already uses.
d	Test intent preserved / new tests added?	New test 04401_text_index_array_lowcardinality_nullable added, covering INSERT, has() query, OPTIMIZE FINAL merge rebuild, Nested, and MATERIALIZE INDEX. No existing test weakened.
e	Demonstrated both directions?	Yes. Unpatched binary (Build ID 71b8f06…) throws NOT_IMPLEMENTED; patched binary (Build ID bbe0369…) passes all paths.
f	Fix general, not a narrow patch?	Yes. The scalar branch was already correct; the array branch was the only remaining site using the weaker isNullable(). Verified no other index-construction site uses the bare isNullable() check for null-skipping.
g	Generalizes across inputs / type wrappers?	Yes. isColumnNullableOrLowCardinalityNullable covers both Nullable and LowCardinality(Nullable); Array(Nullable(String)) (already working) and Array(LowCardinality(Nullable(String))) both take the null-skip path. Non-nullable LC and plain String are unaffected (helper returns false, same as before).
h	Backward compatible?	Yes. Only skips NULL elements that previously threw; no format, setting, or default change. Data that used to error now indexes correctly.
i	Invariants and contracts preserved?	Yes. getDataAt() is now only called on non-NULL elements, honoring ColumnNullable/ColumnLowCardinality contracts. Granule construction, offsets, and incrementCurrentRow() bookkeeping are unchanged.

Session id: cron:clickhouse-worker-slot-6:20260710-210200

[groeneai]

cc @CurtizJ @rschu1ze @ahmadov for review — one-line fix in the text-index aggregator's array branch (use isColumnNullableOrLowCardinalityNullable instead of isNullable so NULL LowCardinality(Nullable) array elements are skipped), plus a regression test covering INSERT / MATERIALIZE INDEX / merge / Nested.

[clickhouse-gh]

Workflow [PR], commit [9ff7d35]

Summary: ✅

• Performance Comparison: Performance dashboard

---

AI Review

Summary

This PR fixes text index construction for Array(LowCardinality(Nullable(String))) rows containing NULL elements by using the same nullability helper in both the direct array path and the postprocessor/tokenizeToArray path, and it adds focused regressions for INSERT, MATERIALIZE INDEX, OPTIMIZE FINAL, and Nested. I did not find any unresolved correctness or coverage gaps in the current patch.

Final Verdict

• Status: ✅ Approve

[groeneai]

Done — pushed 9ff7d35.

Switched the tokenizeToArray array branch (src/Interpreters/ITokenizer.cpp:495) from data.isNullable() to isColumnNullableOrLowCardinalityNullable(data), matching the fix in MergeTreeIndexText.cpp. So when a postprocessor is present (postprocessor->hasActions()), NULL elements of an Array(LowCardinality(Nullable(String))) are skipped there too instead of hitting getDataAt() and throwing NOT_IMPLEMENTED.

I grepped the array-element path repo-wide (.isNullable() in ITokenizer.cpp, MergeTreeIndexText.cpp, MergeTreeIndexBloomFilterText.cpp): line 495 was the only remaining site, so the whole class is now covered.

Regression: extended 04401_text_index_array_lowcardinality_nullable.sql with a postprocessor = lower(a) variant over Array(LowCardinality(Nullable(String))) containing NULL elements, exercising INSERT, OPTIMIZE FINAL (merge rebuild), and MATERIALIZE INDEX. Confirmed both directions on a debug build: baseline (no fix) throws Code: 48 NOT_IMPLEMENTED on the INSERT; with the fix it passes.

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. CREATE TABLE ... a Array(LowCardinality(Nullable(String))), INDEX idx a TYPE text(tokenizer='splitByNonAlpha', postprocessor=lower(a)); INSERT ... VALUES (1,['Hello',NULL,'World']) throws Code: 48 NOT_IMPLEMENTED every time on the pre-fix binary.
b	Root cause explained?	With a postprocessor, update() routes through tokenizeToArray(). Its array branch computed data_is_nullable = data.isNullable(), which is false for ColumnLowCardinality(Nullable), so the NULL-skip guard was bypassed and data.getDataAt(j) was called on a NULL element → ColumnLowCardinality::getDataAt → ColumnNullable::getDataAt throws NOT_IMPLEMENTED.
c	Fix matches root cause?	Yes — replaced data.isNullable() with isColumnNullableOrLowCardinalityNullable(data), the same helper the MergeTreeIndexText.cpp array branch already uses, so the guard correctly detects LC(Nullable).
d	Test intent preserved / new tests added?	Added a postprocessor regression case (INSERT / OPTIMIZE FINAL / MATERIALIZE INDEX) to the existing 04401 test; existing cases unchanged.
e	Both directions demonstrated?	Yes. Baseline binary: INSERT throws Code 48. Fixed binary: full test passes (pp_array_lc_nullable 2, pp_has_hello 2, pp_after_optimize 2, pp_materialize_lc_nullable 1).
f	Fix is general across code paths?	Yes — grepped the array-element .isNullable() sites in the tokenizer/index files; line 495 was the only remaining one. The direct array branch (addDocumentsFromArray) was already fixed in this PR.
g	Fix generalizes across inputs?	The helper covers Nullable, LowCardinality(Nullable), and (via the existing branch) plain types; test covers plain Array(Nullable(String)) control + Array(LowCardinality(Nullable(String))) with single-NULL, mixed, and all-NULL rows.
h	Backward compatible?	Yes — pure bug fix; previously the query threw, now it succeeds. No setting/format/default change.
i	Invariants and contracts preserved?	Yes — only the nullability predicate is widened; the loop's per-element isNullAt(j) skip and offset bookkeeping are unchanged, so token positions and array offsets stay consistent.

Session id: cron:clickhouse-worker-slot-2:20260711-122100

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	79.90%	85.80%	+5.90%
Functions	92.20%	92.70%	+0.50%
Branches	72.00%	77.90%	+5.90%

Changed lines: Changed C/C++ lines covered: 9/9 (100.00%) · Uncovered code

Full report · Diff report

[groeneai]

CI finish ledger — 9ff7d35

All checks green (153 success / 18 skipped, Finish Workflow + Mergeable Check pass). No failed checks to attribute.

Check / test	Reason	Owner / fixing PR
CH Inc sync	—	CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260711-173000

[alexey-milovidov]

Good.