[BugFix] Fix lake fast-path ADD INDEX: CHAR/NGRAMBF correctness, durability across loads & compaction#75910
[BugFix] Fix lake fast-path ADD INDEX: CHAR/NGRAMBF correctness, durability across loads & compaction#75910meegoo wants to merge 12 commits into
Conversation
|
@codex review |
Module-risk briefing —
|
There was a problem hiding this comment.
Pull request overview
This PR fixes correctness issues in the lake ADD INDEX fast path so the generated bitmap/bloom/NGRAMBF indexes are byte-identical (and behaviorally consistent) with the standard segment-rewrite path.
Changes:
- In the ADD INDEX fast path, build
TabletIndexPBviaTabletIndex::init_from_thrift()+to_schema_pb()soindex_propertiesare preserved (e.g.,gram_num,case_sensitive,bloom_filter_fpp) and validate indexed columns exist before calling the uncheckedfield_index()path. - Re-pad
TYPE_CHARslices back to the declared column width (with'\0') before feeding bitmap/bloom writers, avoiding mismatches caused by decoder trimming.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| be/src/storage/lake/schema_change.cpp | Uses TabletIndex conversion to preserve index properties and adds pre-validation for missing columns in ADD INDEX fast path. |
| be/src/storage/lake/add_index_schema_change.cpp | Adds TYPE_CHAR repadding in the index feed path and plumbs declared CHAR width into bitmap/bloom builders. |
meegoo
changed the title
…loom (index_properties) Two type-specific correctness bugs in the lake (shared-data) ADD INDEX / SET bloom_filter_columns fast path (IDG .idx sidecar): 1. CHAR columns: the page decoder strnlen-trims trailing '\0' padding when the fast path reads the source column back to build the index, so the bitmap dictionary / bloom filter were built from UNPADDED values. The standard segment-write path and the query predicate both use the CHAR value padded to the declared column width, so the fast-path index never matched at query time -> bitmap over-pruned to an empty result, bloom false-negatived (missing rows). Re-pad CHAR values back to the declared width with '\0' in feed_index_from_column so the fast-path index is byte-identical to the standard path. 2. NGRAMBF / plain bloom: do_process_add_index_only built TabletIndexPB by hand and dropped index_properties, so gram_num / case_sensitive / bloom_filter_fpp were lost -> NGRAMBF built with the wrong gram_num (default 4) never matched the query predicate's ngrams (no pruning). Build the pb via TabletIndex::init_from_thrift + to_schema_pb so all properties (and the schema-resolved column unique ids) are propagated exactly like the standard path. Verified on a shared-data cluster with the real basic_types_data: CHAR bitmap/bloom over-prune reproduced; largeint bitmap and bigint bloom already work (their CI failures were cluster-instability flakes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: meegoo <meegoo.sr@gmail.com>
…ompaction
The lake ADD INDEX / SET bloom_filter_columns fast path (IDG) wrote .idx
sidecars for existing segments and edited the tablet-metadata schema content
(table_indices + per-column has_bitmap_index/is_bf_column), but REUSED the same
schema_id/schema_version. Every lake by-id schema cache assumes id==content
(get_cached_schema GS{id} metacache; GlobalTabletSchemaMap dedup, whose
check_schema_unique_id ignores index flags; historical_schemas; SCHEMA_{id}
file), so data loaded AFTER the index — and compaction output — kept resolving
the cached pre-index schema and built no index. Compaction additionally deletes
the input rowsets' IDG entries, losing the index permanently.
Fix: FE allocates a new schema_id+schema_version for the fast-path add-index
(once, at job build; persisted for idempotent replay) and stamps it onto the
base MaterializedIndexMeta (applyCatalogMutation, mirroring
LakeTableAsyncFastSchemaChangeJob), and forwards it to BE via
TAlterTabletReqV2 -> OpAddIndex. BE apply_add_index sets the new
id/version on the tablet-metadata schema and writes SCHEMA_{new_id}. The new id
makes every by-id cache miss and pick up the now-indexed schema, so future
loads and compaction build the index inline. This is a pure index-add
(columns unchanged): existing rowsets carry no rowset_to_schema pin and resolve
to metadata->schema(); their segments' missing footer index is served by the
.idx sidecar until compaction rebuilds it inline.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: meegoo <meegoo.sr@gmail.com>
Signed-off-by: meegoo <meegoo.sr@gmail.com>
…ath ngram not applied at read) The lake IDG fast-path builds an .idx ngram bloom for NGRAMBF, but the query read path never surfaces that IDG ngram entry, so LIKE predicates never prune (verified on-cluster: a footer-defined ngram filters ~16k rows while the fast-path ngram does a full scan with no BloomFilterFilterRows counter). Until the BE fast-path ngram read is implemented, exclude NGRAMBF from the fast-path classifier so ADD INDEX ... USING NGRAMBF takes the legacy LakeTableSchemaChangeJob (which rewrites segments with a correct footer ngram, matching GIN/VECTOR handling). BITMAP keeps the fast path; plain bloom_filter_columns is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: meegoo <meegoo.sr@gmail.com>
…in the lake fast path
A column may carry at most one of {plain bloom filter, ngram bloom filter}. The
regular schema-change path enforces this via IndexAnalyzer.analyseBfWithNgramBf,
but the lake bloom_filter_columns fast path (tryBuildLakeAddBloomFilterJob) did
not — so 'SET ("bloom_filter_columns"="c")' on a column that already has an
NGRAMBF index was silently accepted instead of erroring 'should only have one
bloom filter index or ngram bloom filter index'. Run the same validation before
taking the fast path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: meegoo <meegoo.sr@gmail.com>
meegoo
force-pushed
the
fix/lake-idg-index-typebugs
branch
from
50fe12d to
98971d8
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98971d8719
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ma-id bump Two pre-existing tests encoded the old fast-path contract that this PR intentionally changed: - SchemaChangeIndexFastPathClassifierTest: NGRAMBF is now routed to the legacy path (the fast-path .idx ngram bloom is not consulted at query time), so it is no longer an accepted add-index fast-path type and isSupportedIndexType(NGRAMBF) is false. - SchemaChangeHandlerLakeIndexFastPathTest: the tryBuild*() builders now read the base index meta's schema version to allocate a new schema id/version (durability fix). stubLakeTable() must stub getBaseIndexMetaId()/getIndexMetaByMetaId(), otherwise the builders NPE and return null (breaking the *_HappyPath and force-cancel-allowlist tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BNsDjnXjbty1eDHzZnFAEQ Signed-off-by: meegoo <meegoo.sr@gmail.com>
…ablets The lake fast-path ADD INDEX / bloom-filter durability fix allocates one new schema id/version and bumps only the base index meta (applyCatalogMutation). But populateAlterRequest stamped that id on every dispatched tablet, including rollup / sync-MV index tablets whose FE meta keeps its old id — leaving those tablets' BE schema id ahead of the catalog on a table with rollups. Gate the stamp on indexMetaId == baseIndexMetaId so only base-index tablets carry the new id; rollup tablets keep their existing schema id (their pre-fix behavior). The dispatch already resolves a per-tablet indexMetaId, so plumb it into populateAlterRequest and capture the base index meta id on the job. The common no-rollup case is unchanged (base is the only index). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BNsDjnXjbty1eDHzZnFAEQ Signed-off-by: meegoo <meegoo.sr@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5f29b4a7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…volved tables Full hardening of the fast-path ADD INDEX / bloom-filter schema-id bump so it is durable across the two cases the single-id / base-only design missed: 1. Rollup / sync-MV indexes (FE). The dispatch fans an AlterReplicaTask out to every visible materialized index's tablets, but only the base index meta was bumped, so rollup tablets' BE schema id diverged from their unchanged FE meta. Allocate a distinct schema id/version per affected index meta (one getNextId() each; version = that meta's own version + 1), stamp each tablet with its own index's id (via the indexMetaId now passed to populateAlterRequest), and bump every affected meta in applyCatalogMutation. Keeps FE and BE schema ids aligned per index. Mirrors LakeTableAsyncFastSchemaChangeJob's per-index handling. 2. Non-PK tables that already fast-evolved (BE). When rowset_to_schema is populated, existing rowsets are pinned to a historical schema and both the read path and compaction's get_output_rowset_schema resolve through historical_schemas, bypassing metadata->schema() — so the schema-id bump was silently skipped there. apply_add_index now archives the indexed schema under the new id and repoints the pins that referenced the pre-index current schema to it, so those rowsets read via the .idx sidecar and compaction rebuilds the index inline. Rowsets pinned to older (fewer-column) schemas keep their pin; empty-map (fresh / never-evolved) tables are unchanged. Guarded on historical_schemas so replay is idempotent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BNsDjnXjbty1eDHzZnFAEQ Signed-off-by: meegoo <meegoo.sr@gmail.com>
Signed-off-by: meegoo <meegoo.sr@gmail.com>
meegoo
force-pushed
the
fix/lake-idg-index-typebugs
branch
from
27e86fd to
224bf80
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ved-table repoint, CHAR repad) The BE Incremental Coverage report flagged the new lake fast-path ADD INDEX durability code as uncovered. Add focused unit tests: - meta_file_test: apply_add_index with new_schema_id set — (1) empty rowset_to_schema stamps schema id/version only; (2) fast-evolved table (non-empty rowset_to_schema) archives the indexed schema into historical_schemas[new_id] and repoints the pre-index-current-schema pins to it while leaving older-schema pins untouched, and is idempotent on replay. - add_index_schema_change_test: do_process_add_index_only carries the FE-allocated new_index_schema_id/version into OpAddIndex; empty-columns index is rejected; and a CHAR column drives feed_index_from_column's repad_char path (char_pad_len = column.length()) for both BITMAP and BLOOM_FILTER — the fixture gains a CHAR c4 column (value column, mirrors the existing VARCHAR c2 and the CHAR write in rowset_test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BNsDjnXjbty1eDHzZnFAEQ Signed-off-by: meegoo <meegoo.sr@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc5547c43b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…INDEX FE Incremental Coverage flagged the new per-index schema-id code: - LakeTableIndexFastPathJobTest: applyCatalogMutation now bumps EACH affected index meta (shallowCopy -> setSchemaId/setSchemaVersion -> put -> rebuildFullSchema) — tested with a base + rollup meta; and populateAlterRequest stamps a tablet's task with ITS index meta's new schema id (and skips a tablet whose index has no allocated entry). - AlterReplicaTaskTest: setNewIndexSchema + toThrift carry new_index_schema_id / new_index_schema_version into TAlterTabletReqV2 under only_add_index. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BNsDjnXjbty1eDHzZnFAEQ Signed-off-by: meegoo <meegoo.sr@gmail.com>
|
| // schema_id but changes its content; every by-id schema cache would go | ||
| // stale). Per-index keeps FE/BE schema ids aligned across all the | ||
| // tablets the job dispatches to (dispatch covers every visible index). | ||
| for (Long affectedIndexMetaId : olapTable.getIndexMetaIdToMeta().keySet()) { |
There was a problem hiding this comment.
This allocates a schema bump for every index meta (base + rollup / sync-MV), and dispatch fans ADD INDEX tasks to every meta's tablets. shouldUseAddIndexFastPath only gates on isCloudNativeTableOrMaterializedView, so a table with rollups takes this path. BE do_process_add_index_only returns InternalError when an index meta's schema lacks the indexed column (the field-existence guard), and the fast path deliberately does not fall back to legacy — so a rollup that omits the indexed column would fail the whole alter. Can you confirm this multi-index-meta case is handled (e.g. rollups always contain the indexed column, or such tables are excluded upstream)? All the new tests use a single base index, so this path is currently uncovered.
…han historical entry GC Two review follow-ups on apply_add_index STEP 4: - Gate create_schema_file on the schema id actually changing in this apply, so re-applying the same OpAddIndex (metadata replay on restart) does not repeat the remote object-store write. First apply behavior is unchanged. - Document that a fully-repointed old_schema_id can leave its historical_schemas entry unreferenced, and that apply_opcompaction's historical_schemas GC reclaims it on the next compaction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BNsDjnXjbty1eDHzZnFAEQ Signed-off-by: meegoo <meegoo.sr@gmail.com>
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 1 / 1 (100.00%) file detail
|
[BE Incremental Coverage Report]✅ pass : 42 / 44 (95.45%) file detail
|
Why I'm doing:
The lake (shared-data) ADD INDEX /
SET("bloom_filter_columns"=...)fast path (IDG.idxsidecar; PR #71689 / #75147 / #75337) had several correctness/durability defects, surfaced by thetest_create_bitmap_index/test_create_bloom_filter/test_create_ngram_bloom_filterStarRocksTest cases. Verified on a shared-data cluster with the realbasic_types_data(65,546 rows).What I'm doing:
Five fixes: https://github.com/StarRocks/StarRocksTest/issues/11510
CHAR padding. The page decoder
strnlen-trims trailing\0forTYPE_CHAR, so the fast path (which reads the source column back through aColumnIterator) fed unpadded CHAR values to the bitmap dictionary / bloom filter, while the standard path and the query predicate use the value padded to the declared width. Result: bitmap over-pruned to empty, bloom false-negatived (missing rows).feed_index_from_columnnow re-pads CHAR to the declared width. (VARCHAR is untouched.)NGRAMBF / bloom
index_properties.do_process_add_index_onlyhand-builtTabletIndexPBand droppedindex_properties(gram_num/case_sensitive/bloom_filter_fpp). It now builds the pb viaTabletIndex::init_from_thrift+to_schema_pb, propagating all properties.Durability across new loads + compaction. The fast path changed the schema content (
table_indices+ per-columnhas_bitmap_index/is_bf_column) but reused the sameschema_id/version. Every lake by-id schema cache (GS{id}metacache;GlobalTabletSchemaMapdedup, whosecheck_schema_unique_idignores index flags;historical_schemas;SCHEMA_{id}file) then kept returning the stale pre-index schema, so data loaded after the index — and compaction output — built no index (compaction also deletes the input.idx, losing it permanently). FE now allocates a newschema_id+version(once, persisted for idempotent replay) and stamps it FE→BE (TAlterTabletReqV2→OpAddIndex→apply_add_index), so every cache misses and picks up the indexed schema. Existing rowsets carry norowset_to_schemapin and resolve tometadata->schema(); their segments' missing footer index is served by the.idxsidecar until compaction rebuilds it inline.NGRAMBF routed to the legacy path. The fast-path
.idxngram bloom is not consulted at query time (verified: a footer ngram filters ~16k rows while the fast-path ngram does a full scan, noBloomFilterFilterRows). Until the BE fast-path ngram read is implemented,SchemaChangeIndexFastPathClassifierexcludes NGRAMBF (same treatment as GIN/VECTOR), soADD INDEX ... USING NGRAMBFtakes the legacyLakeTableSchemaChangeJob(which rewrites a correct footer ngram). BITMAP keeps the fast path.bloom_filter_columnson an ngram column now rejected in the fast path. A column may carry at most one of {plain bloom, ngram bloom}; the regular path enforces this viaIndexAnalyzer.analyseBfWithNgramBf, but the bloom fast path skipped it. Added the same validation before taking the fast path.Validation
Re-ran the previously-failing StarRocksTest cases against a cluster running this PR (build of
50fe12d): all in-scope fast-path index cases pass — bitmap (char, largeint), bloom (bigint, char), all NGRAMBF variants, drop-index, and the PK cloud-native/native bf & bitmap cases. Remaining reds are out of this PR's scope: non-index suites (iceberg / range-distribution-split / rename-column), and the shared-nothinglocalpersistent-index PK cases (which cannot run on a shared-data cluster).What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check:
Known limitations / follow-ups
RewriteSimpleAggToMetaScanRule's conservative design) disables the shared-data min/max/count meta-scan rewrite for the table — the same effect as any other lake schema change (e.g. ADD COLUMN). Kept intentionally: the version monotonicity is what lets a concurrently-created partition's tablets reconcile to the indexed schema on their first load.apply_add_indexpersists the newSCHEMA_{id}file synchronously on the publish path (one extra object-store PUT per tablet, once per ADD INDEX). Best-effort and non-fatal; kept as-is — a one-time DDL cost that serves cold by-id readers.