airborne12 commented on code in PR #66877:
URL: https://github.com/apache/doris/pull/66877#discussion_r3848876210
##########
be/src/storage/index/snii/snii_index_reader.cpp:
##########
@@ -1100,11 +1126,35 @@ Status SniiIndexReader::_try_count_only_fastpath(
RETURN_IF_ERROR(
::doris::snii::query::count_only_term_df(*logical_reader,
physical_term, &count));
}
+ // df bounds the fabricated bitmap, so it has to be inside a document
domain
+ // that is itself real. Two steps, because they fail differently.
+ const auto& stats = logical_reader->stats();
+ if (count > stats.doc_count || count > stats.indexed_doc_count) {
Review Comment:
I built both shapes you describe and ran them against the real write path
rather than reasoning about them. The short answer is that neither is
producible by any Doris writer, and the fix you propose is not a fix but a
removal of the shortcut.
**`df` is not an independent field — it is counted off the posting stream.**
`entry->df = total_docs_` (`logical_index_writer.cpp:169` and `:188`), where
`total_docs_` is accumulated by the same `fill()` loop that emits the posting,
and `:393` asserts `DCHECK_EQ(emitted_stats.df, total_docs_)`.
Measured, not inferred. A probe drives the production path end to end —
`OlapBlockDataConvertor` → `IndexColumnWriter::create` → `IndexFileWriter`
(SNII) → read back through `IndexFileReader::open_snii_index` — and compares
`DictEntry::df` against `term_query`'s result for every term. Both `DictEntry`
encodings:
```
200 rows (enc = kSlim) common df=184 |posting|=184
posting&nulls=0
seven df=26 |posting|=26
posting&nulls=0
rare df=6 |posting|=6
posting&nulls=0
20000 rows (enc = kWindowed) common df=18461 |posting|=18461
posting&nulls=0
seven df=2638 |posting|=2638
posting&nulls=0
rare df=596 |posting|=596
posting&nulls=0
```
**The `{2, 12}` shape on a 10-row segment is also unreachable**: posting
docids come from the writer's `_rid`, which is exactly the number of rows
appended, so a posting cannot name a row the segment does not have.
**And every compaction re-checks it.** `posting_cursor.cpp` rejects a
df/posting disagreement at `:581`, `:714`, `:788` and `:828`, so even a
hypothetical mismatched image would fail the next time it is merged rather than
propagate.
One clarification on the framing: the decode path does not validate `df`
either. `term_query` → `internal::read_docid_posting` never reads `entry.df` —
it returns whatever the posting bytes hold. So this is not "the row-accurate
path checks and the shortcut does not"; neither checks, they trust different
fields of the same CRC-protected image.
On the requested remedy — "validate both `df == payload cardinality` and
every posting member against the real segment domain" — that requires decoding
the posting, which is precisely the path the shortcut exists to skip. It
removes the optimization rather than hardening it. Your second option, a format
capability that guarantees both invariants independently, is the coherent
design, but it needs a new on-disk bit and cannot help indexes already written.
I am happy to pursue that separately if the consensus is that this shortcut
must survive a hostile-but-CRC-valid image; it is out of scope for a
reader-side bounds fix.
Leaving this thread open rather than resolving it, since that is a
maintainer call.
--
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]
