github-actions[bot] commented on code in PR #68039:
URL: https://github.com/apache/doris/pull/68039#discussion_r4024440173
##########
be/src/common/config.cpp:
##########
@@ -1408,6 +1408,10 @@ DEFINE_mBool(debug_inverted_index_compaction, "false");
DEFINE_mBool(inverted_index_ram_dir_enable, "true");
// wheather index by RAM directory when base compaction
DEFINE_mBool(inverted_index_ram_dir_enable_when_base_compaction, "true");
+// Norms cost one byte per segment row, including rows that hold no value for
the field. A segment
+// holds one index per variant path, so writing norms for them costs rows *
paths bytes. Turn this on
+// to leave norms out of indexes on a variant path, except those that set the
"norms" property.
Review Comment:
[P2] Document that the config overrides the property
This says indexes that set `norms` are exempt, but `create_field` makes
`skipped_by_config` win even over `norms=true`, and the new tests assert that
behavior. This is the nearest operator-facing description of a switch that
changes ranking semantics, so it can lead an operator to disable norms they
meant to retain. Please correct this comment and the duplicate in `config.h`
(plus the stale unit-test preamble), or change the precedence to match the text.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/InvertedIndexUtil.java:
##########
@@ -288,6 +292,12 @@ private static void
checkInvertedIndexProperties(Map<String, String> properties,
+ ", support_phrase must be true or false");
}
+ String norms = properties.get(INVERTED_INDEX_NORMS_KEY);
Review Comment:
[P1] Do not silently accept an unsupported SNII norms policy
This validation accepts `norms=false` for SNII, but SNII never reaches the
changed CLucene writer: its writer sets `_writes_norms` only from
analyzer/positions, and its direct-compaction policy does the same, so both the
property and the new variant config are silently ignored. SNII scoring
additionally rejects normless segments today. Please either implement the
policy end to end for SNII (fresh writes, both compaction paths, and scoring)
or reject/scope it here, with format-specific tests.
##########
regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy:
##########
@@ -0,0 +1,214 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// An analyzed index writes dense BM25 norms (.nrm, one byte per segment row),
on a variant path as
+// on any other column, and "norms" = "false" drops them. Norms on a variant
path cost rows * paths
+// bytes, so inverted_index_skip_norms_for_variant leaves them out there
whatever the property says.
+// This covers both an index declared with a field_pattern and a whole-column
+// index on a VARIANT column, whose per-subcolumn copies inherit the
properties of the index they
+// come from. BM25 scoring keeps working with and without norms.
+suite("test_variant_subcolumn_index_norms", "p0") {
Review Comment:
[P1] Run this config-changing suite non-concurrently
This suite is tagged only as `p0`, but `setBeConfigTemporary` below updates
every BE and has no cross-suite lock. Normal suites execute on the
`suiteParallel` pool, so another VARIANT-index suite can create segments while
this temporary no-norms policy is active (or race with restoration). The
regression-test README explicitly requires cluster-config changes to use
`nonConcurrent`; please add that group here.
##########
be/src/storage/index/inverted/similarity/bm25_similarity.cpp:
##########
@@ -41,6 +42,13 @@ BM25Similarity::BM25Similarity(float idf, float avgdl) :
_idf(idf), _avgdl(avgdl
}
void BM25Similarity::compute_tf_cache() {
+ // CLucene keeps a field's token count in its .nrm header, so avgdl is 0
when no segment of the
+ // field stores norms (e.g. variant subcolumn indexes). Every document
then has an unknown length:
+ // score without length normalization instead of computing 0 / 0.
+ if (_avgdl <= 0.0F) {
Review Comment:
[P1] Handle mixed normed and normless segments
This fallback runs only when the collection-wide `avgdl` is zero. During the
supported transition, old segments still have norms while new ones omit them:
collection statistics count documents from both but add zero tokens for the
normless segment, so any old segment keeps `avgdl > 0`. Both scoring paths then
feed a default 0/1 norm for the normless rows into this positive-avgdl cache,
treating them as extremely short documents and boosting them over otherwise
identical old rows. Please track missing norms and apply one consistent policy
to the whole mixed collection (or per segment), and add a mixed-generation
scoring test.
--
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]
