iframe-proxy

groeneai · 2026-07-02T13:50:35Z

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 a text index defined on mapValues(map) or mapKeys(map) being silently not used when a table was queried through a Distributed engine table with the analyzer. The index was used for the local table and via cluster()/remote(), but a query through a Distributed engine table skipped it (and failed with INDEX_NOT_USED under force_data_skipping_indices).

Description

Reported in #108874. A text index on mapValues(attributes) (or mapKeys(attributes)) is used when querying the local MergeTree table and via cluster()/remote(), but silently not used when the same table is queried through a Distributed engine table with the analyzer.

Root cause: StorageDistributed only serializes the query to shards and never reads columns locally, but it inherited the default supportsOptimizationToSubcolumns() = supportsSubcolumns() = true. The Distributed table has no secondary indices in its own metadata, so the index-safety guard in FunctionToSubcolumnsPass (which refuses to rewrite columns required by a secondary index) never sees attributes as index-relevant on the initiator. The pass therefore rewrites mapValues(attributes) to the subcolumn attributes.values and serializes that form into the shard query, where it no longer matches the index defined on mapValues(attributes).

IStorageCluster (which backs cluster()/remote()) already returns false from supportsOptimizationToSubcolumns() for exactly this reason (commit 0c9926a7045d); StorageDistributed was the missing case. This PR adds the same override.

Because the shard re-runs the analyzer against the real underlying table (with full index metadata), legitimate subcolumn optimizations still happen shard-side where the physical read actually occurs, so there is no lost optimization.

Reproducer (needs the analyzer and a 2-shard cluster):

SET enable_full_text_index = 1, enable_analyzer = 1;

CREATE TABLE logs
(
    attributes Map(String, String),
    INDEX attributes_vals_idx mapValues(attributes) TYPE text(tokenizer = 'array') GRANULARITY 1
) ENGINE = MergeTree ORDER BY tuple();

CREATE TABLE logs_dist AS logs
ENGINE = Distributed('test_cluster_two_shards_localhost', currentDatabase(), logs, rand());

INSERT INTO logs VALUES ({'ip': '192.168.1.1'});

-- Before this PR: Code 277 INDEX_NOT_USED. After: the index is used.
SELECT count() FROM logs_dist WHERE has(mapValues(attributes), '192.168.1.1')
  SETTINGS force_data_skipping_indices = 'attributes_vals_idx';

Added a regression test: 02346_text_index_bug108874.

…gine table A text index defined on mapValues(map) or mapKeys(map) was used for a local MergeTree table and via cluster()/remote(), but silently NOT used when the same table was queried through a Distributed engine table with the analyzer, failing with INDEX_NOT_USED under force_data_skipping_indices. StorageDistributed only serializes the query to shards and never reads columns locally, but it inherited the default supportsOptimizationToSubcolumns() = true. The Distributed table has no secondary indices in its own metadata, so FunctionToSubcolumnsPass on the initiator rewrote mapValues(attributes) to the subcolumn attributes.values and serialized that form to the shards, where it no longer matched the index expression. IStorageCluster (cluster()/remote()) already returns false here for the same reason; StorageDistributed was the missing case. The shard re-runs the analyzer against the real underlying table, so legitimate subcolumn optimizations still happen shard-side where the physical read occurs. Closes ClickHouse#108874. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-07-02T13:51:03Z

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. `SELECT count() FROM logs_dist WHERE has(mapValues(attributes), '192.168.1.1') SETTINGS force_data_skipping_indices = 'attributes_vals_idx'` throws `Code 277 INDEX_NOT_USED` every time on master (2-shard cluster, `enable_analyzer=1`, `enable_full_text_index=1`).
b	Root cause explained?	Yes. `StorageDistributed` inherited the default `supportsOptimizationToSubcolumns() = supportsSubcolumns() = true`. The `Distributed` table has no secondary indices in its own metadata, so `FunctionToSubcolumnsPass`'s index-safety guard never marks `attributes` as index-relevant on the initiator; it rewrites `mapValues(attributes)` to the subcolumn `attributes.values` and serializes that form to the shards, where it no longer matches the index defined on `mapValues(attributes)`. Confirmed by capturing the shard query: Distributed path sent `has(__table1.attributes.values, ...)` while `cluster()` sent `has(mapValues(__table1.attributes), ...)`; and `optimize_functions_to_subcolumns=0` fixes it.
c	Fix matches root cause?	Yes. `StorageDistributed::supportsOptimizationToSubcolumns()` now returns `false`, so the pass no longer rewrites functions to subcolumns for a `Distributed` engine table. This directly targets the rewrite identified in (b).
d	Test intent preserved / new tests added?	New regression test `02346_text_index_bug108874` added; no existing test weakened. It asserts the index is used through local, `cluster()`, and `Distributed`-engine paths via `force_data_skipping_indices` (which throws `INDEX_NOT_USED` when the index is not used).
e	Demonstrated in both directions?	Yes. With the fix reverted (byte-identical to the original binary), the new test FAILS with `INDEX_NOT_USED`; with the fix it PASSES. Verified through the `clickhouse-test` harness.
f	Fix is general, not a narrow patch?	Yes. Audited all remote storages: `IStorageCluster` already returns `false` (same reason, commit `0c9926a7045d`); `StorageDistributed` was the only remote storage still returning `true`. Fix covers both `mapValues` and `mapKeys` (verified) and any function-to-subcolumn rewrite, not just the one reported. `Buffer`/`Merge`/`MaterializedView` are not `isRemote` (they read locally, so the optimization is legitimate there).
g	Fix generalizes across inputs?	Yes. The change is at the storage-capability level, independent of the specific function/type. Verified `mapValues` and `mapKeys` both fixed; legitimate subcolumn optimizations (e.g. `length(arr)`) still fire shard-side (the shard re-runs the analyzer against the real table), so no optimization is lost for any input.
h	Backward compatible?	Yes. No setting default, on-disk/wire/replication format, or existing validation changed. The behavior change is: a query through a `Distributed` engine table now correctly uses skip indices that were previously missed (a strict improvement). No `SettingsChangesHistory` entry needed.
i	Invariants and contracts preserved?	Yes. `supportsOptimizationToSubcolumns()` is a per-storage capability hint; returning `false` for a storage that never reads columns locally is exactly its contract (mirrors `IStorageCluster`). No data-structure, concurrency, or block-structure invariant is touched. Subcolumn optimization still happens on the shard where the physical read occurs.

Session id: cron:clickhouse-worker-slot-3:20260702-125700

groeneai · 2026-07-02T13:51:15Z

cc @CurtizJ @novikd — could you review this? It fixes a text index on mapValues(map)/mapKeys(map) being silently skipped when a table is queried through a Distributed engine table with the analyzer. Root cause: StorageDistributed inherited supportsOptimizationToSubcolumns() = true, so FunctionToSubcolumnsPass rewrote mapValues(attributes) to the subcolumn attributes.values on the initiator and sent that to the shards, where it no longer matched the index. IStorageCluster already returns false here; this adds the same override to StorageDistributed.

clickhouse-gh · 2026-07-02T13:54:25Z

Workflow [PR], commit [320b49d]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
Stateless tests (arm_asan_ubsan, targeted)		FAIL
	03310_materialized_view_with_bad_select	ERROR	cidb
	Server died	FAIL	cidb, issue
	Segmentation fault (STID: 1367-3620)	FAIL	cidb
AST fuzzer (amd_debug, targeted, old_compatibility)		FAIL
	Logical error: Column A already added for reading (STID: 4792-5e4c)	FAIL	cidb

AI Review

Summary

This PR closes the original StorageDistributed bug and the follow-on wrapper leaks that still re-advertised supportsOptimizationToSubcolumns() on the initiator. I re-checked the current merged diff, the touched storages, and the existing review threads; the added StorageProxy, StorageMaterializedView, StorageMerge, and StorageBuffer changes match the remaining failure modes, and I did not find a new correctness gap that still survives in the current code.

Missing context / blind spots

⚠️ The current merged-head Praktika aggregate for 320b49dec25dfd805ac19c2c9ef1194b76ba1f07 had no finished test entries yet, so I could not validate the fresh CI run. I relied on code inspection plus the completed earlier report for 50ad567125d2539036f88702d3a8a73cca8c8124, whose reported failures were pre-existing trunk issues unrelated to this diff.

Final Verdict

✅ No new inline findings. I did not post a batched review comment.

A database with lazy_load_tables = 1 wraps each table in a StorageTableProxy. StorageProxy forwarded supportsSubcolumns() to the nested storage but not supportsOptimizationToSubcolumns(), which fell back to the IStorage default (tied to supportsSubcolumns() == true). A proxy around Distributed therefore re-advertised true, so FunctionToSubcolumnsPass rewrote mapValues(attributes) to a subcolumn on the initiator before serializing the shard query, and the text index defined on mapValues/mapKeys was missed again for a lazy-loaded Distributed engine table even after StorageDistributed opted out. Forward supportsOptimizationToSubcolumns() through the proxy to the nested storage. Extend the regression test with a lazy_load_tables = 1 case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

… Merge StorageDistributed and StorageProxy already opt out of the subcolumn optimization so a text index on mapValues/mapKeys is not lost through a Distributed engine table. Two more wrapper storages still tied the capability to the IStorage default (supportsSubcolumns() == true): - StorageMaterializedView::readImpl forwards the already-analyzed query tree to the target table without rebuilding it, so a MV whose target is Distributed could still rewrite mapValues(attributes) -> attributes.values on the initiator. Forward supportsOptimizationToSubcolumns() to the target. - StorageMerge over a Distributed child could serialize attributes.values to the shards. Add a fail-closed override that returns false if any child table opts out (same traverseTablesUntil pattern as supportsPrewhere). Extends 02346_text_index_bug108874 with MV-over-Distributed and Merge-over-Distributed cases (mapValues and mapKeys), asserting the index is used via force_data_skipping_indices.

StorageBuffer read() and getQueryProcessingStage() forward the already-analyzed query to the destination table, so it must not let the initiator rewrite functions to subcolumns when the destination opts out (e.g. Distributed). It kept the IStorage default (supportsOptimizationToSubcolumns() == supportsSubcolumns() == true), so a Buffer over a Distributed destination still rewrote mapValues(attributes) / mapKeys(attributes) before the query was split, and the shard query missed the text index (or threw INDEX_NOT_USED under force_data_skipping_indices). Add the fail-closed override mirroring supportsPrewhere(): forward to the destination, false when there is none. Extends the regression to a flushed Buffer over logs_dist. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-07-02T16:53:02Z

Fixed in 50ad567. StorageBuffer now overrides supportsOptimizationToSubcolumns() to forward to the destination table (false when there is none), mirroring its supportsPrewhere() — so a Buffer over a Distributed destination fails closed just like StorageMerge/StorageMaterializedView.

Verified both directions against test_cluster_two_shards_localhost with a flushed Buffer over logs_dist:

Without the override: SELECT ... FROM logs_buffer WHERE has(mapValues(attributes), '192.168.1.1') SETTINGS force_data_skipping_indices = 'attributes_vals_idx' throws Code: 277 INDEX_NOT_USED (the initiator rewrote mapValues(attributes) before splitting, so the shard query lost the index).
With the override: returns the expected count, index used.

Regression 02346_text_index_bug108874 extended with mapValues/mapKeys buffer-over-dist cases.

groeneai · 2026-07-02T16:53:21Z

Pre-PR validation gate (StorageBuffer follow-up, click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. Flushed `Buffer(currentDatabase(), logs_dist, ...)` over a Distributed destination; `SELECT count() FROM logs_buffer WHERE has(mapValues(attributes), '192.168.1.1') SETTINGS force_data_skipping_indices='attributes_vals_idx'` throws `Code: 277 INDEX_NOT_USED` on demand without the fix.
b	Root cause explained?	`StorageBuffer::read()`/`getQueryProcessingStage()` forward the already-analyzed query to the destination, but it inherited the IStorage default `supportsOptimizationToSubcolumns() == supportsSubcolumns() == true`. So `FunctionToSubcolumnsPass` rewrote `mapValues(attributes) -> attributes.values` on the initiator before the query was split; the shard query then carried a subcolumn that no longer matched the text index.
c	Fix matches root cause?	Yes. Adds the fail-closed `supportsOptimizationToSubcolumns()` override keyed off the destination table (mirrors `supportsPrewhere()`), so the initiator does not rewrite when the destination (e.g. Distributed) opts out.
d	Test intent preserved / new tests added?	Regression `02346_text_index_bug108874` extended with `mapValues/mapKeys buffer-over-dist` cases over a flushed Buffer on `logs_dist`, using `force_data_skipping_indices` so a missed index throws (proves the index is used).
e	Both directions demonstrated?	Yes. Without override: `INDEX_NOT_USED` (Build ID `02d29617…`). With override: count `2`, full test 5/5 pass (Build ID `b6cb8a4a…`).
f	Fix is general across code paths?	Yes. Completes the wrapper family: `StorageDistributed` (opt-out), `StorageMerge`/`StorageMaterializedView`/`StorageProxy` (forward), and now `StorageBuffer` (forward). No remaining IStorage-default wrapper over Distributed.
g	Fix generalizes across inputs?	Covered by the destination-forwarding design: the capability is whatever the destination reports, for any destination engine. Test exercises both `mapValues` and `mapKeys`.
h	Backward compatible?	Yes. Only tightens an in-memory capability flag used during analysis; no setting default, on-disk/wire format, or new data validation.
i	Invariants and contracts preserved?	Yes. Mirrors the existing `supportsPrewhere()` contract on the same class (fail closed when the destination is absent/unresolvable via `getDestinationTable()` which returns null, never throws for this purpose).

Session id: cron:clickhouse-worker-slot-5:20260702-163000

groeneai · 2026-07-02T22:25:11Z

CI finish ledger — `50ad567`

Every failure below has an owner (a fixing PR). Neither failure is caused by this PR's diff (which only adds supportsOptimizationToSubcolumns() fail-closed overrides on StorageBuffer/StorageProxy/StorageMaterializedView/StorageMerge/StorageDistributed + a text-index regression test) — both are pre-existing trunk bugs the fuzzer/stress runs happened to hit on this branch.

Check / test	Reason	Owner / fixing PR
AST fuzzer (amd_debug, targeted, old_compatibility) / `Column ... already added for reading` (STID 4792-5e4c)	trunk bug (text-index virtual column re-added when `make_distributed_plan` re-runs `optimizeTreeSecondPass`)	#108818 (ours, open)
Stateless tests (arm_asan_ubsan, azure, sequential) / `AddressSanitizer` (STID 0883-397d) + `Server died`	trunk bug (0883 family: use-after-free reading String `.size` subcolumn across granules during merge)	#109020 (ours, open)
CH Inc sync	-	CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260702-220000

novikd · 2026-07-03T14:52:16Z

@groeneai merge fresh master into the branch

…-text-index-mapvalues-distributed

groeneai · 2026-07-03T15:33:10Z

@novikd merged fresh master (eb26de7) into the branch. New head: 320b49dec25dfd805ac19c2c9ef1194b76ba1f07. No conflicts; diff unchanged (9 files, +159/-0, no submodule drift). CI is re-running.

clickhouse-gh · 2026-07-03T19:20:01Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.60%	85.50%	-0.10%
Functions	92.70%	92.70%	+0.00%
Branches	77.70%	77.70%	+0.00%

Changed lines: Changed C/C++ lines covered: 26/29 (89.66%) · Uncovered code

Full report · Diff report

groeneai · 2026-07-03T21:23:53Z

CI finish ledger — `320b49d`

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

PR-caused gate: ZERO. This PR's diff is supportsOptimizationToSubcolumns() fail-closed capability overrides + a regression test; neither failure relates to it. Both are pre-existing trunk bugs, each isolated to this PR's fuzzer/targeted run by coincidence of the randomized surface. Same two families were owned on the prior head (50ad567) by the same open fixing PRs.

Session id: cron:our-pr-ci-monitor:20260703-210000

groeneai mentioned this pull request Jul 2, 2026

Text index on mapValues(map) is silently not used when querying through a Distributed engine table (analyzer) #108874

Open

PedroTadim added the can be tested Allows running workflows for external contributors label Jul 2, 2026

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

clickhouse-gh Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread src/Storages/StorageDistributed.h

clickhouse-gh Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread src/Storages/StorageProxy.h

Comment thread src/Storages/StorageDistributed.h

clickhouse-gh Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread src/Storages/StorageMerge.h

EmeraldShift mentioned this pull request Jul 3, 2026

Send a serialized query plan to shards when reading from Distributed tables under make_distributed_plan #109248

Open

KochetovNicolai assigned novikd Jul 3, 2026

novikd approved these changes Jul 3, 2026

View reviewed changes

Merge remote-tracking branch 'origin/master' into groeneai/fix-108874…

320b49d

…-text-index-mapvalues-distributed

novikd mentioned this pull request Jul 3, 2026

Disable subcolumn function rewrites for Distributed #108632

Open

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

groeneai commented Jul 2, 2026

Changelog category (leave one):

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

Description

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

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

Uh oh!

AI Review

Summary

Missing context / blind spots

Final Verdict

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

groeneai commented Jul 2, 2026

CI finish ledger — 50ad567

Uh oh!

novikd commented Jul 3, 2026

Uh oh!

groeneai commented Jul 3, 2026

Uh oh!

clickhouse-gh Bot commented Jul 3, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI finish ledger — 320b49d

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clickhouse-gh Bot commented Jul 2, 2026 •

edited

Loading

CI finish ledger — `50ad567`

groeneai commented Jul 3, 2026 •

edited

Loading

CI finish ledger — `320b49d`