Skip to content

Add test: No test combines multiple text indexes + LWD + vertical merge#101110

Merged
zlareb1 merged 13 commits intoClickHouse:masterfrom
clickgapai:qa-bot/coverage-pr92925
Mar 31, 2026
Merged

Add test: No test combines multiple text indexes + LWD + vertical merge#101110
zlareb1 merged 13 commits intoClickHouse:masterfrom
clickgapai:qa-bot/coverage-pr92925

Conversation

@clickgapai
Copy link
Copy Markdown
Contributor

@clickgapai clickgapai commented Mar 29, 2026

Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment.

Retrospective finding from a historical scan of PR #92925 (merged 2025-12-23). Confirmed on current codebase — close with a note if already fixed.

Adds test coverage for 1 untested code path(s), found during automated review of PR #92925.

1. No test combines multiple text indexes + LWD + vertical merge
src/Storages/MergeTree/MergeTask.cpp:640, src/Storages/MergeTree/MergeTask.cpp:1604

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

Not applicable — test-only change.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Found during review of PR #92925.

@clickgapai clickgapai mentioned this pull request Mar 29, 2026
Merged
@zlareb1 zlareb1 added the can be tested Allows running workflows for external contributors label Mar 29, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 29, 2026 •
edited
Loading

Workflow [PR], commit [c01c6d4]

Summary: ❌

job_name test_name status info comment
Stateless tests (amd_asan_ubsan, distributed plan, parallel, 1/2) failure
03668_shard_join_in_reverse_order FAIL cidb, issue
Stress test (arm_tsan) failure
Hung check failed, possible deadlock found FAIL cidb
Stress test (arm_ubsan) failure
Logical error: '(isConst() || isSparse() || isReplicated()) ? getDataType() == rhs.getDataType() : typeid(*this) == typeid(rhs)' (STID: 2508-37b9) FAIL cidb

AI Review

Summary

This PR adds a new stateless regression test that combines multiple text indexes, lightweight delete (LWD), and forced vertical merge, and validates result stability before/after OPTIMIZE ... FINAL. I did not find correctness, safety, performance, or compliance issues in the current diff. High-level verdict: ready to merge.

Missing context
  • ⚠️ No CI logs/results were provided in the review context, so runtime validation status could not be independently confirmed here.
ClickHouse Rules
Item Status Notes
Deletion logging ➖
Serialization versioning ➖
Core-area scrutiny ✅
No test removal ✅
Experimental gate ➖
No magic constants ✅
Backward compatibility ✅
SettingsChangesHistory.cpp ➖
PR metadata quality ✅
Safe rollout ✅
Compilation time ✅
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 29, 2026
@zlareb1
Copy link
Copy Markdown
Member

zlareb1 commented Mar 29, 2026

@clickgapai change the test file name according to existing clickhouse codebase

@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 29, 2026

Thanks, will keep that in mind.

@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 29, 2026

That's a cleaner approach — updating the test.

@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 29, 2026

Done — pushed the update. Let me know if there's anything else.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 29, 2026 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 29, 2026

Renamed test file to follow ClickHouse conventions: 99901_ → 04068_.

@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 29, 2026

Fixed the test file — removed markdown fences and non-standard tags that were accidentally included.

@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 29, 2026

Renamed test file — removed qa_prN_ prefix to match ClickHouse naming conventions.

clickhouse-gh[bot]
clickhouse-gh bot reviewed Mar 29, 2026
View reviewed changes
@alexey-milovidov alexey-milovidov changed the title Add test: No test combines multiple text indexes + LWD + vertical merg Add test: No test combines multiple text indexes + LWD + vertical merge Mar 29, 2026
@alexey-milovidov alexey-milovidov self-assigned this Mar 30, 2026
@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 30, 2026

CI failure in Stress test (amd_msan) appears to be infrastructure. Rerunning.

@groeneai groeneai mentioned this pull request Mar 30, 2026
Open
@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 30, 2026

CI failure in Stress test (arm_ubsan) appears unrelated to this PR (our change only adds a test file in tests/queries/0_stateless/). Retriggering CI (2/2).

@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 30, 2026

CI failure in Stateless tests (amd_asan_ubsan, distributed plan, parallel, 1/2) persists after 2 retries. This appears to be a pre-existing issue in master, unrelated to our change. Not retriggering further. If you'd like me to make changes or retrigger, please comment.

@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 30, 2026

CI failure in Stress test (arm_ubsan) persists after 2 retries. This appears to be a pre-existing issue in master, unrelated to our change. Not retriggering further. If you'd like me to make changes or retrigger, please comment.

@clickgapai
Copy link
Copy Markdown
Contributor Author

clickgapai commented Mar 30, 2026

CI failure in Stress test (arm_tsan) persists after 2 retries. This appears to be a pre-existing issue in master, unrelated to our change. Not retriggering further. If you'd like me to make changes or retrigger, please comment.

@zlareb1 zlareb1 enabled auto-merge March 31, 2026 18:58
@zlareb1 zlareb1 added this pull request to the merge queue Mar 31, 2026
Merged via the queue into ClickHouse:master with commit eebd24c Mar 31, 2026
164 of 167 checks passed
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clickhouse-gh clickhouse-gh[bot] clickhouse-gh[bot] left review comments

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

can be tested Allows running workflows for external contributors pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@clickgapai @zlareb1 @CLAassistant @alexey-milovidov @robot-clickhouse-ci-1