airborne12 commented on code in PR #66877:
URL: https://github.com/apache/doris/pull/66877#discussion_r3848877905
##########
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) {
+ return Status::Error<ErrorCode::INVERTED_INDEX_FILE_CORRUPTED, false>(
+ "SNII count fast path: term df {} exceeds document domain (doc
count {}, "
+ "indexed doc count {})",
+ count, stats.doc_count, stats.indexed_doc_count);
+ }
+ // Both limits above are CRC-valid fields of the SAME image as df, so an
image
+ // whose stats were inflated together with df clears them: on a real 10-row
+ // segment, df = doc_count = indexed_doc_count = 100 fabricates 100 ids,
and
+ // SegmentIterator -- which seeds _row_bitmap with [0, num_rows) and
intersects
+ // -- silently reports 10. The segment's own row count is the one bound the
+ // image cannot move. One-sided on purpose: an index covering FEWER rows
than
+ // the segment still fabricates ids inside [0, num_rows), so only an
oversized
+ // domain is corruption. This mirrors the equality SniiSegmentReader::
+ // load_inherited_index already demands of a rewrite.
+ if (stats.doc_count > _rows_of_segment) {
+ return Status::Error<ErrorCode::INVERTED_INDEX_FILE_CORRUPTED, false>(
+ "SNII count fast path: index doc count {} exceeds the segment
row count {}",
+ stats.doc_count, _rows_of_segment);
+ }
- // Null handling. df is the exact match count REGARDLESS of nulls: the
- // writer adds no tokens for a null doc (scalar add_nulls; a NULL array row
- // is an empty range), so postings -- and therefore df -- never include
- // null rows, exactly matching MATCH's "null never matches" semantics. The
+ // Null handling. df is the exact match count REGARDLESS of nulls: the
writer
Review Comment:
Two separate claims here; I ran both rather than reasoning about them. The
scalar half is falsified by measurement, the frame-validation half is real but
is not specific to this shortcut.
**Scalar posting/null disjointness — measured, does not occur.** Your second
example needs a CRC-valid scalar posting `{1,7}` overlapping a null frame
`{1,3}`. I built exactly the adversarial input: a `Nullable(String)` column
whose NULL row still carries a real nested value (`"alpha"`), i.e. what
`default_implementation_for_nulls` leaves behind, and drove it through the
production write path. Result:
```
REAL SCALAR WRITER posting(alpha) = [] null_bitmap = [0]
```
No token is emitted for the null row, because both
`ColumnWriter::append_nullable` overloads (`column_writer.cpp:386`, `:402`),
`ScalarColumnWriter::append_nullable` (`:656`) and
`IndexBuilder::_add_nullable` (`index_builder.cpp:1078`) split the batch into
runs and route null runs to `add_nulls()`. Confirmed again at scale: 20000 rows
with interleaved nulls, `posting & nulls` is empty for every term, in both
`DictEntry` encodings.
**The ARRAY counterpart, however, is real and reachable** — which is what
this PR gates. Measured:
```
array_concat(['alpha'], nullable_arr) row 0: null_map=1
nested=[alpha,zulu]
REAL ARRAY WRITER posting(alpha) = [0] null_bitmap =
[0]
```
So the shape exists, it comes from ordinary SQL
(`PreparedFunctionImpl::default_implementation_for_nulls` keeps nested values
on NULL rows), and the real writer does index the null row. The fast path now
declines ARRAY columns on any segment with a null bitmap.
**Frame vs Core consistency — you are right that the query path does not
check, and it is worse than you describe.** The writer cannot emit an
inconsistent frame (`stats.null_count`, the frame payload and `doc_count` all
come from the same `null_docids_` vector), so the shape needs a tampered image.
But the validation asymmetry is genuine:
`LogicalIndexReader::read_null_docids()` checks doc count, cardinality and
docid range (`logical_index_reader.cpp:619-635`) and its **only** caller is
compaction (`snii_index_compaction.cpp:250`), while
`SniiIndexReader::_read_null_bitmap()` checks nothing.
The part worth flagging: that unvalidated read is not the fast path's.
`read_null_bitmap()` and `query_with_null_bitmap()` — the source of the
`mask_out_null` the row-accurate MATCH path applies — go through the same
function. A corrupt frame therefore mis-masks the normal path too; the shortcut
is not uniquely exposed. Lifting those three checks into `_read_null_bitmap()`
is cheap (the frame is already fully decoded there) and would harden both
paths, so I would rather do it as its own change than fold a normal-MATCH
behaviour change into a reader-side bounds fix. Say the word and I will open it.
The probes above are currently scratch. If they are wanted as regressions —
the ARRAY one is the public-path case you asked for on the other thread — I can
land them here; that costs a force-push and a fresh buildall on a PR that is
currently green.
--
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]
