Skip to content

fix(clickhouse): use compatible logs text index syntax - #4704

Merged
carderne merged 1 commit into
mainfrom
fix/clickhouse-text-index-migration
Aug 19, 2026
Merged

fix(clickhouse): use compatible logs text index syntax#4704
carderne merged 1 commit into
mainfrom
fix/clickhouse-text-index-migration

Conversation

@carderne

Copy link
Copy Markdown
Collaborator

Summary

Allow the logs search schema migration to run on ClickHouse versions that require text index options to be literals.

Root cause

The text index declared lowerUTF8(search_text) as a preprocessor option. Some ClickHouse versions reject that column expression while parsing index settings. The projected search_text is already normalized to lowercase before insertion, so removing the redundant preprocessor preserves search behavior.

Verified with the task events search integration tests.

@changeset-bot

changeset-bot Bot commented Aug 19, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 7f7f01b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 518ceb38-e55f-40f1-9985-83f43a01f61b

📥 Commits

Reviewing files that changed from the base of the PR and between b939045 and 7f7f01b.

📒 Files selected for processing (1)
  • internal-packages/clickhouse/schema/040_create_task_events_search_v2.sql

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (21)
  • GitHub Check: audit
  • GitHub Check: audit
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 12)
  • GitHub Check: internal / 🧪 Unit Tests: Internal
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 12)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 12)
  • GitHub Check: fk-cascade-guard / fk-cascade-guard
  • GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp
  • GitHub Check: runops-guard / runops-guard
  • GitHub Check: code-quality / code-quality
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (1)
internal-packages/clickhouse/schema/[0-9][0-9][0-9]_*.sql

📄 CodeRabbit inference engine (internal-packages/clickhouse/CLAUDE.md)

internal-packages/clickhouse/schema/[0-9][0-9][0-9]_*.sql: Migration file numbering: name files as 0(N+1)_descriptive_name.sql where N is the largest existing migration number in schema/; rebase and renumber if main adds migrations before opening a PR
DDL in migrations must be idempotent: use ALTER TABLE ... ADD COLUMN IF NOT EXISTS, CREATE TABLE IF NOT EXISTS, DROP TABLE IF EXISTS, ADD INDEX IF NOT EXISTS, DROP INDEX IF EXISTS, and CREATE MATERIALIZED VIEW IF NOT EXISTS forms to allow out-of-order and retry-safe application

Files:

  • internal-packages/clickhouse/schema/040_create_task_events_search_v2.sql
🔇 Additional comments (2)
internal-packages/clickhouse/schema/040_create_task_events_search_v2.sql (2)

39-39: 🗄️ Data Integrity & Integration

Add a forward migration for the changed index.

CREATE TABLE IF NOT EXISTS does not update an existing task_events_search_v2, so deployed tables retain the previous idx_search_text definition. If migration 040 ran in any target environment, add an idempotent migration that drops and re-adds the index, then materializes it for existing parts.


39-39: 🗄️ Data Integrity & Integration

Keep the current normalization path.

The projector and LIKE query apply matching lowercase, character, and colon normalization. Mixed-case and Unicode coverage already exists.

			> Likely an incorrect or invalid review comment.

Walkthrough

The ClickHouse idx_search_text bloom-filter index no longer applies lowerUTF8(search_text) before tokenization. It now uses only ngram tokenization on the raw search_text value.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the root cause and testing, but it omits the required issue, checklist, changelog, and screenshots sections. Add the required template sections, complete the checklist, reference the issue, and provide changelog and screenshot entries.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the ClickHouse text index syntax fix and matches the main schema migration change.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/clickhouse-text-index-migration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@carderne
carderne marked this pull request as ready for review August 19, 2026 11:29

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

@carderne
carderne enabled auto-merge (squash) August 19, 2026 11:35
@carderne
carderne merged commit 49aff3c into main Aug 19, 2026
44 checks passed
@carderne
carderne deleted the fix/clickhouse-text-index-migration branch August 19, 2026 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants