ClickHouse / ClickHouse/ClickHouse
New mapContainsValue shadow guard untestable via file(): 05210 passes without it
- Dominant language
- C++
- Stars
- 49.9k
- Forks
- 9k
- Avg merge
- 21h 32m
- Merged PRs (30d)
- 515
Description
### Describe what's wrong
**`05210_map_contains_value_case_insensitive_collision.sql` reports green whether or not the `sourceHasColumnCaseInsensitive` guard exists. `mapContainsValue` is never rewritten for a `file()` source at all, so both arms return `0` for a reason unrelated to the guard, and the test's comment describes a mechanism the code cannot reach.**
- **Root cause:** The guard at [`src/Analyzer/Passes/FunctionToSubcolumnsPass.cpp:644`](https://github.com/ClickHouse/ClickHouse/blob/7fe32951360/src/Analyzer/Passes/FunctionToSubcolumnsPass.cpp#L644) was copied from `optimizeTupleOrVariantElement` (line 515), where it is live because file-backed storages opt in to tuple-element rewrites via `supportsOptimizationToTupleElementSubcolumns()`. They do not opt in to any other subcolumn rewrite. The shipped test asserts only a scalar result and has no positive-control arm, so it cannot tell `guard fired` from `rewrite never ran`.
Analysis details (evidence, affected locations, impact)
**Why we believe this is a bug:** `FunctionToSubcolumnsVisitorFirstPass::enterImpl` -> `getTypedNodesForOptimization` ([`src/Analyzer/Passes/FunctionToSubcolumnsPass.cpp:1060`](https://github.com/ClickHouse/ClickHouse/blob/7fe32951360/src/Analyzer/Passes/FunctionToSubcolumnsPass.cpp#L1060)) -> `storageAllowsTransformer` (1016-1020). For a `file()` source `StorageFile::supportsOptimizationToSubcolumns()` is `false` ([`src/Storages/StorageFile.h:131`](https://github.com/ClickHouse/ClickHouse/blob/7fe32951360/src/Storages/StorageFile.h#L131)), so the only transformer admitted is `{TypeIndex::Tuple, "tupleElement"}`. The `{TypeIndex::Map, "mapContainsValue"}` lambda at 635-654 — and with it the guard at 644 — is never entered.
**Affected locations:**
- [`src/Analyzer/Passes/FunctionToSubcolumnsPass.cpp:644`](https://github.com/ClickHouse/ClickHouse/blob/7fe32951360/src/Analyzer/Passes/FunctionToSubcolumnsPass.cpp#L644) — the added sourceHasColumnCaseInsensitive guard for mapContainsValue
- [`src/Analyzer/Passes/FunctionToSubcolumnsPass.cpp:1020`](https://github.com/ClickHouse/ClickHouse/blob/7fe32951360/src/Analyzer/Passes/FunctionToSubcolumnsPass.cpp#L1020) — storageAllowsTransformer admits only {Tuple, tupleElement} when supportsOptimizationToSubcolumns() is false
- [`src/Storages/StorageFile.h:131`](https://github.com/ClickHouse/ClickHouse/blob/7fe32951360/src/Storages/StorageFile.h#L131) — StorageFile::supportsOptimizationToSubcolumns() returns false
- [`src/Storages/StorageURL.h:373`](https://github.com/ClickHouse/ClickHouse/blob/7fe32951360/src/Storages/StorageURL.h#L373) — same for url()
- [`src/Storages/ObjectStorage/StorageObjectStorage.h:111`](https://github.com/ClickHouse/ClickHouse/blob/7fe32951360/src/Storages/ObjectStorage/StorageObjectStorage.h#L111) — same for s3()/azure/hdfs/Iceberg/Delta
- [`tests/queries/0_stateless/05210_map_contains_value_case_insensitive_collision.sql:11`](https://github.com/ClickHouse/ClickHouse/blob/7fe32951360/tests/queries/0_stateless/05210_map_contains_value_case_insensitive_collision.sql#L11) — the non-discriminating assertion
**Impact:** A new test file lands that cannot fail when the code it names is deleted, and the reviewer who asked for this guard class gets false assurance that the shadow hazard is now covered for `Map` subcolumn rewrites. CI's own changed-line coverage for this PR marks the bail-out `return` in this block as never executed, across the whole stateless suite.
### Does it reproduce on most recent release?
Yes — confirmed on current `master` (commit `7fe32951360`).
### How to reproduce
[▶ Run on ClickHouse Fiddle](https://fiddle.clickhouse.com/0712b776-eb2f-4f66-926c-b1bb59bbc455)
Reproducer
```sql
-- Test: the case-insensitive shadow guard for the mapContainsValue -> has(m.values, ...) rewrite.
DROP TABLE IF EXISTS t_05213;
SET enable_analyzer = 1;
SET optimize_functions_to_subcolumns = 1;
INSERT INTO FUNCTION file(currentDatabase() || '_05213_v.orc', ORC, 'm Map(String, UInt64), `M.values` Array(UInt64)')
SELECT map('key', toUInt64(1)), [toUInt64(4)]
SETTINGS engine_file_truncate_on_insert = 1;
INSERT INTO FUNCTION file(currentDatabase() || '_05213_k.orc', ORC, 'm Map(String, UInt64), `M.keys` Array(String)')
SELECT map('key', toUInt64(1)), ['zzz']
SETTINGS engine_file_truncate_on_insert = 1;
DROP TABLE IF EXISTS t_05213;
CREATE TABLE t_05213 (id UInt64, m Map(String, UInt64)) ENGINE = MergeTree ORDER BY id;
INSERT INTO t_05213 VALUES (0, {'key': 1});
SELECT 'rewrite_over_file', countIf(explain LIKE '%m.values%') > 0
FROM (EXPLAIN actions = 1
SELECT countIf(mapContainsValue(m, toUInt64(4)))
FROM file(currentDatabase() || '_05213_v.orc', ORC, 'm Map(String, UInt64), `M.values` Array(UInt64)')
SETTINGS input_format_orc_case_insensitive_column_matching = 1);
SELECT 'rewrite_over_mergetree', countIf(explain LIKE '%m.values%') > 0
FROM (EXPLAIN actions = 1 SELECT countIf(mapContainsValue(m, toUInt64(1))) FROM t_05213);
SELECT 'unguarded_sibling_shadowed', countIf(mapContainsKey(m, 'zzz'))
FROM file(currentDatabase() || '_05213_k.orc', ORC, 'm Map(String, UInt64), `M.keys` Array(String)')
SETTINGS input_format_orc_case_insensitive_column_matching = 1;
DROP TABLE IF EXISTS t_05213;
```
### Expected behavior
Expected output of the reproducer above:
```
rewrite_over_file 1
rewrite_over_mergetree 1
unguarded_sibling_shadowed 1
```
### Error message and/or stacktrace
Actual output of the reproducer above on `master` (`7fe32951360`):
```
rewrite_over_file 0
rewrite_over_mergetree 1
unguarded_sibling_shadowed 0
```
Suggested fix
Either drop `05210` together with the `sourceHasColumnCaseInsensitive` call at line 644 (the hazard is unreachable for `Map` rewrites through `file`/`url`/object storage), or give `05210` the positive control that `04873_tuple_subcolumn_pushdown_case_insensitive_collision.sql:22-24` already has — an `EXPLAIN QUERY TREE` arm over a non-colliding schema asserting the rewrite fires. That arm returns `0` for `mapContainsValue` today, which is exactly the evidence the current test is inert.
Additional context
**Open risks:**
- `mapKeys`, `mapValues`, `mapContainsKey`, `length`/`empty` (`.size0`), `isNull`/`isNotNull`/`count` (`.null`) and `arrayElement` (`.key_*`) have no case-insensitive shadow guard. Audited: all are SAFE today for the same reason this one is inert — `storageAllowsTransformer` blocks them on every storage that has a case-insensitive column matcher. If `StorageObjectStorage::supportsOptimizationToSubcolumns()` is ever flipped back to true (the in-source comment at StorageObjectStorage.h:107-110 ties it to a Parquet PREWHERE limitation that may be lifted), all of them become exposed and only `mapContainsValue` would be protected.
Found during automated review of [PR #119172](https://github.com/ClickHouse/ClickHouse/pull/119172). Severity P3 · Finding `h_pr119172_001`
cc @fallintoplace @alexey-milovidov (from #119172)
Contributor guide
Research direction
Start with src/Analyzer/Passes/FunctionToSubcolumnsPass.cpp at lines 644 and 1020, then inspect StorageFile.h, StorageURL.h, and StorageObjectStorage.h to confirm which transformers each storage admits. Run tests/queries/0_stateless/05210_map_contains_value_case_insensitive_collision.sql and compare it with 04873_tuple_subcolumn_pushdown_case_insensitive_collision.sql. Done means the test distinguishes a live guard from an unreachable rewrite, or the dead guard and misleading test are removed consistently.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, sql
- Domain
- databases, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 56/100