airborne12 commented on code in PR #67859:
URL: https://github.com/apache/doris/pull/67859#discussion_r3988506048
##########
be/src/storage/task/index_builder.cpp:
##########
@@ -386,7 +386,14 @@ Status IndexBuilder::update_inverted_index_info() {
st = Status::Error<ErrorCode::INIT_FAILED>(
"debug point: reader init error");
})
- if (!st.ok() &&
!st.is<ErrorCode::INVERTED_INDEX_FILE_NOT_FOUND>()) {
+ // A missing container (the rowset predates every index) and an
+ // EMPTY one (the schema owns an index, but no logical index
had
+ // anything to write -- an all-NULL VARIANT column) both mean
the
+ // same thing here: there is nothing to carry over. Both leave
the
+ // reader un-inited, so get_all_directories() yields an empty
map
+ // and every requested index is built from the raw columns.
+ if (!st.ok() &&
!st.is<ErrorCode::INVERTED_INDEX_FILE_NOT_FOUND>() &&
+ !st.is<ErrorCode::INVERTED_INDEX_BYPASS>()) {
Review Comment:
Thanks — the test criticism is right and I have rewritten it; the code claim
does not hold.
**Why a zero-byte index file cannot be a former home of another index's
data.** Every non-VARIANT inverted index opens its directory eagerly inside
`IndexColumnWriter::create()`
(`be/src/storage/segment/column_writer.cpp:565-573`, comment: *"After create()
(which runs the writer's init()) and before any value lands"*). So if a
rowset's schema owns any such index, `_indices_dirs` is non-empty and the file
is non-empty. Zero bytes implies no logical index ever opened a directory,
which implies there was never any index content in that file. The only other
way to reach zero bytes is truncation, i.e. corruption — and there the change
turns a loud ALTER failure into a rebuild of the requested index, while queries
on the absent index downgrade correctly (a per-index miss in a present
container also returns `INVERTED_INDEX_FILE_NOT_FOUND`) and the next ordinary
compaction rebuilds it from the raw columns. No data loss.
**The test was misleading, though.** Truncating a populated `k1` index reads
as if losing `k1` were acceptable, and it is not a shape production can
produce. Rewritten in 2be3081: `build_k2_index_over_unwritten_k1()` builds the
rowset *before* the schema gains `k1`, so `k1` has no content anywhere and
nothing can be lost, then places either a zero-byte index file or none at all.
Two tests now assert the two shapes side by side and both yield output index
set `{2}`:
```
IndexBuilderTest.BuildIndexOverEmptyIndexFileTest -> {2}
IndexBuilderTest.BuildIndexOverMissingIndexFileTest -> {2}
```
That comparison is the point of the change: an empty index file is being
read the way a missing one has been read for years, not given new semantics.
Reverting **only** the `INVERTED_INDEX_BYPASS` line shows the asymmetry
precisely:
```
[ FAILED ] IndexBuilderTest.BuildIndexOverEmptyIndexFileTest
[E-6004]inverted index file .../15695_0.idx is empty.
[ OK ] IndexBuilderTest.BuildIndexOverMissingIndexFileTest
```
Rebuilding every output-schema index missing from the source would change
the long-standing `INVERTED_INDEX_FILE_NOT_FOUND` path too — that is a separate
design decision about BUILD INDEX semantics, not part of this fix.
--
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]
