github-actions[bot] commented on code in PR #64652:
URL: https://github.com/apache/doris/pull/64652#discussion_r3568183478
##########
fe/fe-core/src/main/java/org/apache/doris/task/CreateReplicaTask.java:
##########
@@ -319,11 +319,14 @@ public TCreateTabletReq toThrift() {
tColumns = (List<TColumn>) tCols;
} else {
tColumns = new ArrayList<>();
+ Set<String> namedBfColumns =
Index.extractBloomFilterColumns(indexes);
for (int i = 0; i < columns.size(); i++) {
Review Comment:
This cache entry now depends on more than `columns`. On a cache miss the
serialized `TColumn` list is built with
`Index.extractBloomFilterColumns(indexes)`, but the pool lookup and put still
use only `columns` as the key. The local create-replica callers share one
object pool across all indexes while passing named indexes only for the base
index, and copied same-schema rollups can compare equal to the base schema
because `Column.equals()` ignores unique id. If an equal-schema non-base task
fills the cache first, the base tablet can reuse columns without its named-BF
flag; if the base fills it first, a non-base tablet can inherit named-BF flags
despite `indexes == null`. Please include the effective BF/index context in the
cache key or skip this cache whenever the named-index context differs.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/IndexDefinition.java:
##########
@@ -231,6 +236,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) {
+ Column bfColumn = new Column(indexColName,
column.getType().toCatalogDataType());
+ if (!bfColumn.isSupportBloomFilter()) {
Review Comment:
This accepts a named BLOOMFILTER on root VARIANT, but the BE writer never
materializes that index. The column is marked as BF-capable and this new branch
accepts it because `Column.isSupportBloomFilter()` returns true for VARIANT;
then `SegmentWriter`/`VerticalSegmentWriter` set `opts.need_bloom_filter` and
the index-level FPP, but both paths later run
`DISABLE_INDEX_IF_FIELD_TYPE(VARIANT)`, clearing `need_bloom_filter` before the
`VariantColumnWriter` is created. The sparse/doc writers then receive options
with BF disabled, so `SHOW INDEX` can advertise `idx_v` while inserts produce
no bloom-filter index for `v`; the new debug-point regression does not catch
this because no `BloomFilterIndexWriter::create` call means the check never
fires. Please either reject named BLOOMFILTER on root VARIANT or implement a
real VARIANT BF writer path and add coverage that fails when the writer is not
created.
##########
be/src/storage/tablet/tablet_schema.cpp:
##########
@@ -1849,6 +1851,31 @@ const TabletIndex*
TabletSchema::get_ngram_bf_index(int32_t col_unique_id) const
return nullptr;
}
+double TabletSchema::get_bloom_filter_fpp(int32_t col_unique_id) const {
+ const auto* bloom_filter_index = get_index(col_unique_id,
IndexType::BLOOMFILTER, "");
+ if (bloom_filter_index != nullptr) {
+ const auto& properties = bloom_filter_index->properties();
+ auto iter = properties.find("bloom_filter_fpp");
+ if (iter != properties.end()) {
+ StringParser::ParseResult parse_result =
StringParser::PARSE_FAILURE;
+ auto index_level_fpp = StringParser::string_to_float<Float64>(
+ iter->second.data(), iter->second.size(), &parse_result);
+ if (parse_result != StringParser::PARSE_SUCCESS) {
+ return Status::InvalidArgument("failed to parse '{}' as
double", iter->second);
+ }
Review Comment:
`get_bloom_filter_fpp()` returns `double`, so this error branch does not
propagate the `InvalidArgument` status. Because `Status` has an implicit
`operator bool()`, the non-OK status can convert to `false` and then to a
numeric FPP of `0.0`; the writer call sites assign that directly to
`bf_options.fpp`, and `BloomFilter::optimal_bit_num()` later uses the value in
`log(1 - pow(fpp, 1.0 / 8))`. That means malformed persisted/index metadata
silently creates an invalid bloom-filter configuration instead of failing
loudly. Please make this helper type-correct for errors, for example return
`Result<double>`/`Status` and propagate it from the segment, vertical, and
variant writer paths, or assert an invariant before returning a numeric value.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3294,6 +3314,13 @@ private boolean processAddIndex(CreateIndexOp
createIndexOp, OlapTable olapTable
// so here update column name in CreateIndexClause after checkColumn
for indexDef,
// there will use the column name in olapTable instead of the column
name in CreateIndexClause.
alterIndex.setColumns(indexDef.getColumnNames());
+ List<Index> conflictIndexes = new ArrayList<>(newIndexes);
+ conflictIndexes.add(alterIndex);
Review Comment:
This lets a mixed ALTER publish a named BLOOMFILTER without taking the full
schema-change path that materializes BF column flags. `processAddIndex()` adds
the BLOOMFILTER to `newIndexes`, but BLOOMFILTER is not light-add-supported; if
the same statement also adds an INVERTED/ANN/NGRAM index that is
light-supported, `lightIndexChange` becomes true and the final dispatch goes to
`modifyTableLightSchemaChange()` instead of `createJob()`. That updates FE
metadata with the full `newIndexes`, while the BE `IndexChangeJob` only builds
`alterIndexes` and `AlterInvertedIndexTask.toThrift()` serializes columns
without `ColumnToThrift.setIndexFlag()`. The base tablet can therefore
advertise the new BF index but keep old tablet schemas with no
`is_bloom_filter_column` flag or BLOOMFILTER build. Please force any ALTER
containing a BLOOMFILTER add/drop through the full create-replica/schema-change
path, or reject mixing it with light-index changes.
--
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]
