[feat](fe) Support named bloom filter indexes in DDL and schema change#64652
[feat](fe) Support named bloom filter indexes in DDL and schema change#64652hoshinojyunn wants to merge 1 commit into
Conversation
hoshinojyunn
requested review from
924060929,
CalvinKirs,
dataroaring,
englefly,
gavinchou,
luwei16,
morningman,
morrySnow and
starocean999
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 29361 ms |
TPC-DS: Total hot run time: 175787 ms |
ClickBench: Total hot run time: 25.23 s |
hoshinojyunn
force-pushed
the
master
branch
2 times, most recently
from
39129a2 to
0e0cf67
Compare
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
hoshinojyunn
force-pushed
the
master
branch
2 times, most recently
from
80d0150 to
1be7525
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found three issues that should be fixed before merging.
Checkpoint conclusions:
- Task goal and implementation: the named BLOOMFILTER syntax is wired through parser/catalog/schema-change paths, but materialization is incomplete for non-base materialized indexes.
- Tests: good coverage for parser, FE validation, BE tablet metadata, and regression DDL; missing rollup/schema-change materialization and backup/restore signature coverage for named-only bloom filters.
- Compatibility and persistence: named-only
bloom_filter_fppis not reflected in the table signature used by restore schema comparison. - Parallel paths: local/cloud schema-change and create-table/add-partition paths need the same effective bloom-filter-column handling.
- Performance/concurrency/lifecycle: no separate blocking issue found in locking, lifecycle, or performance-sensitive code.
- User focus: no additional user-provided focus points were present.
| tColumns = (List<TColumn>) tCols; | ||
| } else { | ||
| tColumns = new ArrayList<>(); | ||
| Set<String> namedBfColumns = Index.extractBloomFilterColumns(indexes); |
There was a problem hiding this comment.
Named bloom filter columns are derived only from indexes, but several existing non-base-index callers still pass null here. For example, SchemaChangeJobV2.createShadowIndexReplica() only passes indexes for the base index, while addShadowIndexToCatalog() writes the new index list into every changed shadow index meta when indexChange is true. ALTER TABLE ... ADD INDEX bf(v1) USING BLOOMFILTER on a table with a rollup containing v1 will therefore create the rollup shadow tablets without is_bloom_filter_column or bloom_filter_fpp, but the catalog then advertises the bloom filter index for that rollup. The same base-only indexes pattern exists in the cloud schema-change path and initial partition creation. Please pass the effective named bloom-filter column set to every materialized index schema that contains those columns, or fold named bloom-filter columns into the bfColumns argument, and add a rollup/schema-change test.
There was a problem hiding this comment.
Reasonable. The real bug is in schema change shadow-tablet materialization for non-base indexes.
Changed:
- Added Index.collectBloomFilterColumns(...) and Index.filterBloomFilterColumnsBySchema(...)
- In SchemaChangeJobV2 and CloudSchemaChangeJobV2, materialize per-shadow-schema shadowBfColumns and pass them down instead of relying on tabletIndexes != null
- Added FE UTs for local/cloud schema change rollup shadow replicas and a rollup regression case
| bfFpp = PropertyAnalyzer.analyzeBloomFilterFpp(properties); | ||
| if (bfColumns != null && bfFpp == 0) { | ||
| boolean hasLegacyBf = bfColumns != null; | ||
| boolean hasNamedBf = !Index.extractBloomFilterColumns(createTableInfo.getIndexes()).isEmpty(); |
There was a problem hiding this comment.
This makes bfFpp meaningful for named-only bloom filters, but OlapTable.getSignature() still appends bfFpp only when the legacy bfColumns set is non-empty. RestoreJob uses that signature to decide whether an existing table has the same schema before restore, so two tables with the same named BLOOMFILTER index but different PROPERTIES("bloom_filter_fpp"=...) compare equal and restore can keep tablets built with the wrong FPP. Please include named bloom filters in the signature condition, for example by using the effective getCopiedBfColumns()/has-any-BF logic, and add coverage for named-only FPP in the signature/restore path.
There was a problem hiding this comment.
Reasonable. Named-only BLOOMFILTER tables should also make bfFpp part of the table signature.
Changed:
- Updated OlapTable.getSignature(...) to use getCopiedBfColumns() instead of the legacy-only member field
- Added OlapTableTest.testNamedBloomFilterFppAffectsSignature
| } catch (Exception ex) { | ||
| throw new AnalysisException("invalid ngram bf index params:" + ex.getMessage(), ex); | ||
| } | ||
| } else if (indexType == IndexType.BLOOMFILTER) { |
There was a problem hiding this comment.
This BLOOMFILTER branch runs only after the generic BITMAP/INVERTED/BLOOMFILTER/NGRAM_BF checks above, including the inverted-index V1 VARIANT rejection. As a result, a named BLOOMFILTER on a VARIANT column is rejected when inverted_index_storage_format resolves to V1/DEFAULT under those rules, even though the legacy bloom_filter_columns analyzer accepts VARIANT through Column.isSupportBloomFilter() and the new tests only cover the null format. Please gate the V1/VARIANT restriction to INVERTED or otherwise skip it for BLOOMFILTER, and add V1/default-format coverage.
There was a problem hiding this comment.
Reasonable. BLOOMFILTER should not inherit the inverted-index-only VARIANT + V1/DEFAULT rejection.
Changed:
- Narrowed the VARIANT + V1/DEFAULT restriction in both IndexDefinition.checkColumn(...) overloads so it does not apply to BLOOMFILTER
- Kept NGRAM_BF behavior unchanged
- Added VARIANT + BLOOMFILTER + V1/DEFAULT coverage for both overloads
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 29246 ms |
TPC-DS: Total hot run time: 175546 ms |
ClickBench: Total hot run time: 25.24 s |
| DCHECK_EQ(index.columns.size(), 1); | ||
| if (iequal(tcolumn.column_name, index.columns[0])) { | ||
| column->set_is_bf_column(true); | ||
| has_bf_columns = true; |
There was a problem hiding this comment.
has_bf_columns is used to set the bloom filter function (FPP) in the following text. In the current design, the FPP of the named BF index depends on the bloom_filter_fpp defined in the table properties. Therefore, when checking the indexes of tablet_meta, if an index of type BLOOMFILTER exists, has_bf_columns is set to true so that bloom_filter_fpp can be set correctly in the following text (a better approach would be to handle NGRAM_BF and BLOOMFILTER separately).
| partitionId, shadowTablet, | ||
| tbl.getPartitionInfo().getTabletType(partitionId), | ||
| shadowSchemaHash, originKeysType, shadowShortKeyColumnCount, bfColumns, | ||
| shadowSchemaHash, originKeysType, shadowShortKeyColumnCount, |
There was a problem hiding this comment.
just format, do not changing this line
| } | ||
| } | ||
| if (found == null) { | ||
| if (containsIgnoreCase(olapTable.getCopiedLegacyBfColumns(), indexName)) { |
There was a problem hiding this comment.
Why preventing dropping bf index?
There was a problem hiding this comment.
This function actually checks whether the user's DROP INDEX statement intends to delete the 'bf' column defined in the table properties. It prevents DROP INDEX from being misused with ALTER TABLE SET ("bloom_filter_column"=...) and prompts the user for the correct deletion method.
yx-keith
left a comment
yx-keith
left a comment
There was a problem hiding this comment.
- Named BF is not materialized on non-base (rollup/MV) indexes — correctness bug
SchemaChangeJobV2 passes indexes only for the base index:
// SchemaChangeJobV2#runPendingJob
List tabletIndexes = originIndexId == tbl.getBaseIndexId() ? indexes : null;
Since named BF columns are derived only from indexes, and the bfColumns stored on the schema-change job is legacy-only (in createJob, bfColumns ends up as originalLegacyBloomFilterColumns), a rollup/MV that contains the column gets neither signal. So ALTER TABLE ... ADD INDEX bf(v1) USING BLOOMFILTER on a table whose rollup contains v1 creates rollup shadow tablets without is_bf_column / bloom_filter_fpp, while the catalog still advertises the BF index for that rollup. The same base-only indexes pattern exists on the cloud schema-change and initial create-table / add-partition paths.
Suggested fix: fold the effective named BF columns into the bfColumns argument for every materialized index that contains them (or pass the relevant indexes to each), and add a rollup/schema-change materialization test.
- getSignature() omits named-only bfFpp — restore can keep wrong-FPP tablets
// OlapTable#getSignature
if (bfColumns != null && !bfColumns.isEmpty()) { // legacy field only
for (String bfCol : bfColumns) { sb.append(bfCol); }
sb.append(bfFpp);
}
For a named-only table the legacy bfColumns field is empty, so bfFpp is excluded from the signature. RestoreJob uses this signature to decide schema equality, so two tables with the same named USING BLOOMFILTER index but different PROPERTIES("bloom_filter_fpp"=...) compare equal and restore can retain tablets built with the wrong FPP.
Suggested fix: include named BF in the signature condition (e.g. use the effective getCopiedBfColumns() / has-any-BF logic), and add named-only FPP coverage on the signature/restore path.
- VARIANT + inverted-index-V1 rule wrongly rejects named BLOOMFILTER
In IndexDefinition.checkColumn, the V1/DEFAULT VARIANT rejection lives inside the shared BITMAP/INVERTED/BLOOMFILTER/NGRAM_BF block and is not gated to INVERTED:
boolean notSupportInvertedIndexForVariant =
(invertedIndexFileStorageFormat == V1 || == DEFAULT)
&& (Config.isCloudMode() || !Config.enable_inverted_index_v1_for_variant);
if (colType.isVariantType() && notSupportInvertedIndexForVariant) { throw ...; }
So a named BLOOMFILTER on a VARIANT column is rejected when the format resolves to V1/DEFAULT, even though the legacy bloom_filter_columns analyzer accepts VARIANT via Column.isSupportBloomFilter(). The tests pass null for the format, which skips this branch and hides the inconsistency.
Suggested fix: gate the V1/VARIANT restriction to INVERTED (or skip it for BLOOMFILTER), and add V1/DEFAULT-format coverage.
Smaller points
CloudSchemaChangeJobV2.java — the change there is whitespace-only; please revert it to keep the diff surgical.
tablet_meta.cpp has_bf_columns = true is correct (it gates set_bf_fpp), but it would read cleaner to handle NGRAM_BF and BLOOMFILTER separately rather than sharing the flag.
Assumption / semantic boundary:
A named bloom filter index and a property-managed bloom filter index cannot be
defined on the same column. The two metadata forms are managed separately, and
users must use `DROP INDEX` for named bloom filter indexes and
`ALTER TABLE SET("bloom_filter_columns" = ...)` for property-managed bloom
filter indexes.
Problem Summary:
Previously Doris only supported bloom filter indexes through table properties such as
`"bloom_filter_columns"` and `"bloom_filter_fpp"`. That made bloom filter indexes
behave differently from other index types and prevented users from managing them with
named `INDEX` / `CREATE INDEX` / `DROP INDEX` syntax.
This change adds named `USING BLOOMFILTER` syntax in the Nereids parser and FE DDL
pipeline, keeps the existing table-property bloom filter behavior for legacy metadata,
and enforces that legacy bloom filter columns and named bloom filter indexes cannot be
defined on the same column. The schema change path now distinguishes legacy bloom
filter management from named bloom filter management, while FE->BE materialization
continues to apply the table-level bloom filter fpp to both forms.
The patch also fixes bloom filter materialization on shadow columns and ensures tablet
metadata marks named bloom filter columns correctly on the BE side. FE unit tests and
bloom filter regression tests are added to cover parser analysis, semantic validation,
schema change checks, FE->BE task generation, and named bloom filter DDL behavior.
|
run buildall |
4 participants
Assumption / semantic boundary:
A named bloom filter index and a property-managed bloom filter index cannot be defined on the same column. The two metadata forms are managed separately, and users must use
DROP INDEXfor named bloom filter indexes andALTER TABLE SET("bloom_filter_columns" = ...)for property-managed bloom filter indexes.Problem Summary:
Previously Doris only supported bloom filter indexes through table properties such as
"bloom_filter_columns"and"bloom_filter_fpp". That made bloom filter indexes behave differently from other index types and prevented users from managing them with namedINDEX/CREATE INDEX/DROP INDEXsyntax.This change adds named
USING BLOOMFILTERsyntax in the Nereids parser and FE DDL pipeline, keeps the existing table-property bloom filter behavior for legacy metadata, and enforces that legacy bloom filter columns and named bloom filter indexes cannot be defined on the same column. The schema change path now distinguishes legacy bloom filter management from named bloom filter management, while FE->BE materialization continues to apply the table-level bloom filter fpp to both forms.The patch also fixes bloom filter materialization on shadow columns and ensures tablet metadata marks named bloom filter columns correctly on the BE side. FE unit tests and bloom filter regression tests are added to cover parser analysis, semantic validation, schema change checks, FE->BE task generation, and named bloom filter DDL behavior.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Assumption / semantic boundary:
A named bloom filter index and a property-managed bloom filter index cannot be
defined on the same column. They are tracked as two separate metadata forms.
DROP INDEXonly applies to named bloom filter indexes, whileALTER TABLE SET("bloom_filter_columns" = ...)only applies toproperty-managed bloom filter indexes.
Bloom filter indexes in Doris were historically managed only by table properties:
"bloom_filter_columns""bloom_filter_fpp"That created three problems:
named
INDEX/CREATE INDEX/DROP INDEXsyntax.definition, which made the metadata model awkward once named bloom filter indexes
were introduced.
named bloom filter indexes, including schema change shadow columns.
This PR introduces named bloom filter indexes with
USING BLOOMFILTER, supports bothinline table-definition syntax and standalone
CREATE INDEX, and keeps compatibilitywith legacy table-property bloom filters. To avoid semantic ambiguity, a column cannot
be managed by both legacy bloom filter properties and a named bloom filter index at
the same time.
Main changes
Parser and analysis
BLOOMFILTERkeyword in the Nereids lexer/parser.USING BLOOMFILTERin table index definitions andCREATE INDEX.IndexDefinitionsemantic checks for bloom filter indexes.as a table-level property.
FE DDL and schema change semantics
as separate metadata sources.
ALTER TABLE SET("bloom_filter_columns" = ...)semantics forproperty-managed bloom filter creation and deletion.
DROP INDEXto named bloom filter indexes and return a clearer error ifthe target refers to a legacy property-managed bloom filter column.
FE->BE materialization
applies to a column.
columns inherit the expected bloom filter metadata.
Tests
Supported examples
CREATE TABLEproperty-managed bloom filter index, but not both.
DROP INDEXto remove named bloom filter indexes.ALTER TABLE SET("bloom_filter_columns" = ...)to add or removeproperty-managed bloom filter indexes.
Release note
Users can now create and manage bloom filter indexes with named index syntax while
continuing to use legacy table-property bloom filter definitions. Doris rejects
conflicting legacy and named bloom filter definitions on the same column.
Test Execution
Build
./build.sh --be --feSuccessfully build DorisFE unit tests
./run-fe-ut.sh --run org.apache.doris.catalog.ColumnBloomFilterMaterializationTest,org.apache.doris.catalog.CreateTableWithBloomFilterIndexTest,org.apache.doris.cloud.datasource.CloudInternalCatalogBloomFilterMaterializationTest,org.apache.doris.nereids.parser.NereidsParserTest,org.apache.doris.nereids.trees.plans.commands.IndexDefinitionTest,org.apache.doris.task.AgentTaskTest,org.apache.doris.alter.SchemaChangeHandlerTestTests run: 191, Failures: 0, Errors: 0, Skipped: 0BE unit tests: bloom filter focused
./run-be-ut.sh --run --filter='*BloomFilter*:*bloom_filter*:*test_write_bf_with_finalize'Running 90 tests from 12 test suites,90 tests passedBE unit tests: tablet metadata / schema / protobuf conversion
./run-be-ut.sh --run --filter='TabletSchemaTest.*:TabletMetaTest.*:PbConvert.*:TabletIndexTest.*:TabletSchemaIndexTest.*'Running 48 tests from 5 test suites,48 tests passedRegression tests
./run-regression-test.sh --run -d bloom_filter_p0 -s test_bloom_filterTest 1 suites, failed 0 suites, fatal 0 scripts, skipped 0 scriptsALTER TABLE SET("bloom_filter_fpp"=...)by asserting BE
BloomFilterIndexWriter::createreceives0.03, then0.02during schemachange rewrite, then
0.03again for newly inserted rowsets after the fpp restore../run-regression-test.sh --run -d bloom_filter_p0 -s test_bloom_filter_named_indexTest 1 suites, failed 0 suites, fatal 0 scripts, skipped 0 scriptsUSING BLOOMFILTERinline DDL, standaloneCREATE INDEX,DROP INDEX, conflict checks with legacybloom_filter_columns, and table-levelbloom_filter_fpppropagation to named bloom filter indexes.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)