github-actions[bot] commented on code in PR #64652:
URL: https://github.com/apache/doris/pull/64652#discussion_r3448333325
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -2707,9 +2707,11 @@ private boolean createOlapTable(Database db,
CreateTableInfo createTableInfo) th
}
bfFpp = PropertyAnalyzer.analyzeBloomFilterFpp(properties);
- if (bfColumns != null && bfFpp == 0) {
+ boolean hasLegacyBf = bfColumns != null;
+ boolean hasNamedBf =
!Index.extractBloomFilterColumns(createTableInfo.getIndexes()).isEmpty();
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/task/CreateReplicaTask.java:
##########
@@ -319,11 +320,14 @@ public TCreateTabletReq toThrift() {
tColumns = (List<TColumn>) tCols;
} else {
tColumns = new ArrayList<>();
+ Set<String> namedBfColumns =
Index.extractBloomFilterColumns(indexes);
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/IndexDefinition.java:
##########
@@ -231,6 +235,12 @@ public void checkColumn(ColumnDefinition column, KeysType
keysType,
} catch (Exception ex) {
throw new AnalysisException("invalid ngram bf index
params:" + ex.getMessage(), ex);
}
+ } else if (indexType == IndexType.BLOOMFILTER) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
