Skip to content

fix(clickhouse): null-guard index type enum in buildCreateTable - #2154

Open
Aias00 wants to merge 1 commit into
OtterMind:mainfrom
Aias00:fix/2148-clickhouse-index-npe
Open

fix(clickhouse): null-guard index type enum in buildCreateTable#2154
Aias00 wants to merge 1 commit into
OtterMind:mainfrom
Aias00:fix/2148-clickhouse-index-npe

Conversation

@Aias00

@Aias00 Aias00 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Related issue

Closes #2148

Summary

ClickHouseSqlBuilder.buildCreateTable called getByType then checked !PRIMARY.equals(result) — if result was null, !PRIMARY.equals(null) is true, so null.buildIndexScript() NPEs. Added if (mysqlIndexTypeEnum == null) continue; mirroring every sibling builder.

Verification

  • mvn compile -> BUILD SUCCESS.

Contributor declaration

  • I linked the Issue that defines this change.
  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: The fix, verification, and PR description were produced with Claude Code assistance.

@Aias00
Aias00 requested a review from openai0229 as a code owner July 26, 2026 06:45
Copilot AI review requested due to automatic review settings July 26, 2026 06:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@openai0229

Copy link
Copy Markdown
Contributor

The null guard prevents the immediate NPE, but this PR is not ready to merge because it silently drops an index from the generated ClickHouse DDL when TableIndex.type has no enum mapping. That can hide schema loss instead of making the unsupported input visible.

Please define the intended policy for an unknown index type: either fail with a descriptive error or log/handle the omission explicitly. Add automated generated-DDL tests for known, primary, null, and unknown index mappings, asserting the complete output. This builder also overlaps the open #2174 changes, so please rebase onto the latest main and resolve the overlap before another review.

@openai0229 openai0229 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.

The null guard still silently omits unknown indexes from generated DDL, which can hide schema loss, and the required DDL policy tests are missing. The current head does not address the detailed review comment already posted, so this remains blocked pending an updated commit and re-review.

getByType returns null for unrecognized types; !PRIMARY.equals(null) is
true so null.buildIndexScript() NPEs. Add if (== null) continue with a
log.warn so unknown index types are visible in logs instead of silently
dropped. Add 4 generated-DDL tests covering known, primary, null, and
unknown index-type mappings.

Fixes OtterMind#2148

Co-Authored-By: Claude <noreply@anthropic.com>
@Aias00
Aias00 force-pushed the fix/2148-clickhouse-index-npe branch from cf50229 to 3cbdd62 Compare July 28, 2026 13:40
@Aias00

Aias00 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto latest main. Unknown index types now log.warn instead of silent skip. 4 generated-DDL tests added: known, primary, null, unknown. Tests run: 4, Failures: 0.

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.

ClickHouseSqlBuilder NPE on null index type enum (missing null guard)

3 participants