Token/ngram/text indexes take a column named `lower(s)` for the expression

Đang mở
  • Có gì không ổn?
#131,444 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

Chưa có ai nhận issue này.

Đánh giá

Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức phù hợp với người mới
22/100
Loại issue
Lỗi
Độ rõ ràng
Đặc tả rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
cpp, sql
Lĩnh vực
databases

Hướng nghiên cứu

Start with getKeyIndex and traverseTreeEquals in src/Storages/MergeTree/MergeTreeIndexBloomFilterText.cpp, then hasIndexForColumn and traverseFunctionNode in MergeTreeIndexConditionText.h and .cpp. Compare them with the structural check findIndexPosition and ExpressionMatcher in MergeTreeIndexBloomFilter.cpp, which #130814 added. Done means the reproducer's three tables return 1 for each query, and the cases are added to 05620_bloom_filter_index_map_keys_column_name.sql.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

ai_p2
Describe what's wrong
  • Queries on mapKeys(m), lower(s) and similar expressions silently lose rows with tokenbf_v1, ngrambf_v1 or text indexes.
  • Trigger: a stored column literally named mapKeys(m) or lower(s) carries one of these indexes.
  • count() returns 0 instead of 1; use_skip_indexes = 0 correctly returns 1.
  • bloom_filter was fixed by #130814 but these three index types still have the bug.

Root cause: MergeTreeConditionBloomFilterText::getKeyIndex and MergeTreeIndexConditionText::hasIndexForColumn resolve the index key by matching the result name only, so a stored column named like the expression's canonical name is mistaken for that expression. #130814 fixed only MergeTreeIndexConditionBloomFilter with a structural check and left these peers unchanged.

Analysis details (evidence, affected locations, impact)

Why we believe this is a bug: MergeTreeConditionBloomFilterText::traverseTreeEquals resolves the key with getKeyIndex(column_name), getKeyIndex("mapKeys(<m>)") and getKeyIndex("lower(<s>)") (MergeTreeIndexBloomFilterText.cpp:628-630, :652, :721); getKeyIndex (:325) is a plain search of the header names. MergeTreeIndexConditionText::traverseFunctionNode does the same via hasIndexForColumn = header.has(name) (MergeTreeIndexConditionText.h:292, .cpp:1306-1309). #130814 (where this was found; not confirmed as the cause)'s structural check (findIndexPosition -> ExpressionMatcher::isSame, findMapIndexPosition, MergeTreeIndexBloomFilter.cpp:455-510) exists only in the bloom_filter condition.

Affected locations:

Impact: Silent row loss for predicates on the expression whenever the table also stores a column with the expression's canonical name and indexes it with tokenbf_v1, ngrambf_v1 or text. For text, the direct-read rewrite (__text_index_il_hasToken_*) answers hasToken(lower(s), 'abc') from the stored column's index. The same schema is correct under bloom_filter since #130814, so the result now depends on the index type.

Does it reproduce on most recent release?

Confirmed on master (dc415c941c4f); not checked on a release.

How to reproduce

Needs a table with both a Map/String column and a separate column literally named mapKeys(m) or lower(s), indexed with tokenbf_v1, ngrambf_v1 or text.

▶ Run on ClickHouse Fiddle

Reproducer 
-- Test: a tokenbf_v1, ngrambf_v1 or text index on a column literally named `mapKeys(m)` or `lower(s)` is not used for that expression.

DROP TABLE IF EXISTS t_tok;
DROP TABLE IF EXISTS t_ngram;
DROP TABLE IF EXISTS t_text;

CREATE TABLE t_tok
(
    m Map(String, String),
    s String,
    `mapKeys(m)` String,
    `lower(s)` String,
    INDEX ik `mapKeys(m)` TYPE tokenbf_v1(256, 2, 0),
    INDEX il `lower(s)` TYPE tokenbf_v1(256, 2, 0)
)
ENGINE = MergeTree ORDER BY tuple();

INSERT INTO t_tok VALUES (map('k', 'v'), 'ABC', 'x', 'x');

SELECT count() FROM t_tok WHERE m['k'] = 'v';
SELECT count() FROM t_tok WHERE lower(s) = 'abc';
SELECT count() FROM t_tok WHERE hasTokenCaseInsensitive(s, 'abc');

CREATE TABLE t_ngram
(
    m Map(String, String),
    s String,
    `mapKeys(m)` String,
    `lower(s)` String,
    INDEX ik `mapKeys(m)` TYPE ngrambf_v1(1, 256, 2, 0),
    INDEX il `lower(s)` TYPE ngrambf_v1(2, 256, 2, 0)
)
ENGINE = MergeTree ORDER BY tuple();

INSERT INTO t_ngram VALUES (map('k', 'v'), 'ABC', 'x', 'x');

SELECT count() FROM t_ngram WHERE m['k'] = 'v';
SELECT count() FROM t_ngram WHERE lower(s) LIKE '%ab%';

CREATE TABLE t_text
(
    m Map(String, String),
    s String,
    `mapKeys(m)` Array(String),
    `lower(s)` String,
    INDEX ik `mapKeys(m)` TYPE text(tokenizer = 'array'),
    INDEX il `lower(s)` TYPE text(tokenizer = 'splitByNonAlpha')
)
ENGINE = MergeTree ORDER BY tuple();

INSERT INTO t_text VALUES (map('k', 'v'), 'ABC', ['x'], 'x');

SELECT count() FROM t_text WHERE m['k'] = 'v';
SELECT count() FROM t_text WHERE has(mapKeys(m), 'k') SETTINGS optimize_functions_to_subcolumns = 0;
SELECT count() FROM t_text WHERE hasToken(lower(s), 'abc');

DROP TABLE t_tok;
DROP TABLE t_ngram;
DROP TABLE t_text;

Verified: this block reproduces on current master (dc415c941c4f) when pasted as is.

Expected behavior

Queries on m['k'], lower(s), hasToken, hasTokenCaseInsensitive and has(mapKeys(m), ...) should match rows regardless of whether a column happens to be named after the expression.

Expected output of the reproducer above:

1
1
1
1
1
1
1
1
Error message and/or stacktrace

The index wrongly binds to the stored column with the matching name and prunes the granule, returning 0 rows where 1 is expected.

Actual output of the reproducer above on master (dc415c941c4f):

0
0
0
0
0
0
0
0
Suggested fix

Port #130814 to the peers: pass index.expression->getActionsDAG().getOutputs() to MergeTreeConditionBloomFilterText and MergeTreeIndexConditionText, accept a name hit only when the index node is structurally the queried expression (reuse ExpressionMatcher, moved out of the anonymous namespace of MergeTreeIndexBloomFilter.cpp), and accept mapKeys(<m>)/mapValues(<m>)/lower(<s>) only when the index node is that function over the same input, like findMapIndexPosition. Trade-off: the shared matcher has to live in a header included by three index files. Add the cases to 05620_bloom_filter_index_map_keys_column_name.sql rather than a new file.

Additional context

Open risks:

  • KeyCondition-based minmax, primary key and PARTITION BY share the root cause; already listed in #123151 (groeneai comment), not re-tested here.
  • set index (MergeTreeIndexConditionSet) matching was not tested.

Found during automated review of PR #130814; the bug predates that PR (it reproduces on the master build just before it merged), so the introducing change is not identified yet. Severity P2 · Finding h_pr130814_001

Ngôn ngữ chính
C++
Star
50.9k
Fork
9.3k
Merge trung bình
14 giờ 39 phút
Pull request đã merge (30 ngày)
545

Chuẩn bị môi trường

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của ClickHouse/ClickHouse

Tất cả issue của ClickHouse/ClickHouse

Issue tương tự

Thêm issue về C++

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.