airborne12 commented on code in PR #67134:
URL: https://github.com/apache/doris/pull/67134#discussion_r3869979725
##########
be/src/storage/index/snii/format/core_metadata.cpp:
##########
@@ -207,13 +207,16 @@ Status decode_core_pb(const
doris::snii::SniiCoreMetadataPB& input, CoreMetadata
return corrupted("core metadata: scoring index requires a norms
region");
}
}
- if (has_scoring_tier ||
- (out->common_grams_metadata.has_value() &&
- out->common_grams_metadata->scoring_coverage ==
ScoringCoverage::kComplete)) {
+ // The scoring tier no longer implies CommonGrams: an ordinary analyzed
index
Review Comment:
Re-evaluated this against the actual production baseline rather than the
unpublished Apache merge-base.
The original claim is technically reproducible against Apache merge-base
`c899ceada941807a5114e48a23a2c0a74bd664bd`: that decoder validates metadata for
every scoring T3 and rejects a metadata-free segment. However, that reader is
not deployed. The actual production baseline is SelectDB
[`branch-hotfix-selectdb-cloud-4.1.7-minimax-rows`](https://github.com/selectdb/selectdb-core/commits/branch-hotfix-selectdb-cloud-4.1.7-minimax-rows/)
at `ef649d804bc5a214829474715b4a3c4a2c696cd6`, with CommonGrams disabled. Its
reader accepts only `IndexConfig` 0/1, while this PR writes plain scoring as
config 2; its protobuf layout also predates the norms and `sum_total_term_freq`
layout used by this PR. Persisting plain metadata therefore cannot make a new
rowset readable by the actual production reader: decoding fails earlier on the
index config or layout. Conversely, Apache master is not a production
rolling-upgrade baseline.
Revised assessment under this production premise:
- Severity if the stated scenario existed: **9/10**
- Confidence that the comment scenario matches the real rollout: **1/10**
- Probability of encountering that scenario in production: **0/10**
- Normalized priority: **4.40/10**, below the fix threshold
The proposed metadata/fingerprint/compaction identity work is therefore not
a low-cost compatibility correction. I removed the plain scoring metadata,
builtin analyzer fingerprint, plain-T3 compaction identity changes, and the
test that used the wrong old-reader baseline. CommonGrams-disabled plain
scoring remains metadata-free, and the original SNII compaction behavior is
unchanged.
If a future mixed-version rollout from `ef649d8` is required, it needs a
separate format/capability strategy, such as a writer gate, old-reader
backport, or format-version bump, plus a cross-version golden test. Plain
metadata alone is insufficient.
The amended follow-up is local commit `cc8c7735981` and has not been pushed.
Verification after the rollback: 48 focused tests pass;
clang-format/check-format and the ASAN BE build pass.
--
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]
