airborne12 opened a new pull request, #67859:
URL: https://github.com/apache/doris/pull/67859
### What problem does this PR solve?
Issue Number: N/A
Related PR: #67702 (closed)
Problem Summary:
A segment whose schema owns an inverted index always owns an index file, even
when no logical index had anything to write: an all-NULL VARIANT column
extracts
no subcolumn, so no CLucene directory is ever opened and the container is
closed
with nothing in it. That empty container is committed by `close()`, not by
`create_file()`, and `IndexFileWriter`'s empty-file path only closed three
named
implementations -- `StreamSinkFileWriter`, `S3FileWriter`,
`PackedFileWriter`.
`LocalFileWriter` and `HdfsFileWriter` were left open, and `~LocalFileWriter`
aborts -- and DELETES -- a writer it was never asked to close. So on local
storage the file silently disappeared and `show_nested_index_file` reported
`E-6003 ... not found` where `E-6004 ... is empty` was the truth.
Closing through `FileWriter::state()` instead of a type whitelist fixes
that, but
on its own it is not enough: it makes one latent `IndexBuilder` invariant
violation real, and it leaves a second `IndexBuilder` bug -- already
reachable
today on the sink-node and remote paths, where the empty file does persist --
untouched.
**`IndexBuilder` writes a container for every segment it rewrites**,
including
when the ALTER leaves the schema with no index at all: dropping the last
index,
or adding an index whose column does not exist. Until now the local writer's
destructor happened to delete those files. Once they are closed they persist
as
orphans that nothing ever links, copies, uploads, removes or checksums --
`BetaRowset` decides whether to touch the container by asking
`has_inverted_or_ann_index()` of the rowset's own schema, and both
segment-writer
paths (`SegmentFlusher::_create_segment_writer`,
`VerticalBetaRowsetWriter`) already gate creation the same way.
`IndexBuilder`
was the one caller that did not.
**`IndexBuilder` refuses to read an empty container.**
`update_inverted_index_info()` tolerates `E-6003` ("this rowset predates the
index") but not `E-6004` ("this rowset's index is empty"), even though both
mean
the same thing there -- there is nothing to carry over, build everything
from the
raw columns. `BUILD INDEX` / `ADD INDEX` / `DROP INDEX` on a table with an
all-NULL VARIANT index therefore fails the whole ALTER with
`[E-6004]inverted index file ... is empty`.
So: three changes, one invariant.
> a rowset's schema owns an inverted/ANN index **iff** every one of its
segments
> owns an index file, possibly zero length
1. `IndexFileWriter::begin_close()` / `finish_close()` close the empty
container
through `FileWriter` for any non-null, non-CLOSED writer, preserving the
two-phase asynchronous protocol and propagating close errors.
2. `IndexBuilder::handle_single_rowset()` writes no container when the output
rowset schema owns no index -- on the drop path and the add path alike.
3. `IndexBuilder::update_inverted_index_info()` reads an empty container
exactly
the way it reads a missing one.
Consumers already accept `E-6004`: queries downgrade unconditionally
(`SegmentIterator::_downgrade_without_index`), index compaction marks the
segment
ineligible and rebuilds, `CollectionStatistics` skips it. SNII is untouched
--
its container always carries a header, so it is never zero length, and its
`has_container` probe stays `E-6003`-only.
#67702 made change 1 and then updated
`regression-test/plugins/plugin_curl_requester.groovy` and
`test_drop_index_with_format_v2.groovy` to expect `E-6004` after dropping
every
index. That expectation describes the orphan file, so this PR leaves both at
`E-6003` and removes the orphan instead.
### Release note
Persist empty V2/V3 inverted index files through every FileWriter
implementation
instead of only the remote ones; stop failing ALTER on a segment whose index
file
is legitimately empty; stop leaving an unreferenced index file behind when
the
last index of a table is dropped.
### Check List (For Author)
- Test: Unit Test / Regression test
- `be/test/storage/index/inverted/empty_index_file_test.cpp`:
implementation-
independent lifecycle coverage. A `RecordingFileWriter` that implements
`FileWriter` and nothing else pins the dispatch; plus already-closed,
synchronous-close, both error paths, null writer, and local zero-length
persistence after destruction followed by the reader-side `E-6004`
contract (and its `E-6003` counterpart).
- `be/test/io/fs/s3_file_writer_test.cpp`: exactly one `put_object`, zero
`upload_part`, one empty object at the expected key, and no second PUT
on a
retried finish.
- `be/test/storage/index/index_builder_test.cpp`:
`BuildIndexOverEmptyIndexFileTest` builds an index over a rowset whose
container has been emptied -- it fails with `[E-6004] ... is empty`
without
change 3. `DropInvertedIndexTest` / `DropAnnIndexTest` /
`NonExistentColumnIndexTest` already assert zero `.idx` files after the
rewrite; change 2 is what keeps them true.
-
`regression-test/suites/inverted_index_p0/test_variant_empty_index_file.groovy`
now covers the local write path as well as memtable-on-sink-node, and
enforces the expected query error instead of passing when none is
raised.
-
`regression-test/suites/inverted_index_p0/test_empty_index_file_lifecycle.groovy`
walks one empty-index table through load, full compaction, light schema
change, direct schema change, `ADD INDEX` + `BUILD INDEX`, dropping
one of
two indexes and dropping the last one, asserting the invariant at each
step, for v2 and v3 and for both write paths.
- Behavior changed: Yes. Empty index files are now persisted on local storage
too; a rowset whose schema has no index never carries an index file.
- Does this need documentation: No
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Tp6z4cVB4buRW8JwU1KRdM
--
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]
