Repository navigation
Conversation
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: An all-NULL Variant column creates no logical index directory,
but its V2/V3 FileWriter has already been created. The empty-index close path
only closed StreamSink, S3 and Packed writers, leaving LocalFileWriter and
opaque wrappers open. Local writer destruction could consequently remove the
empty file. Dispatch both close phases through FileWriter for every non-null,
non-CLOSED writer, preserving the asynchronous protocol and returning close
errors. Add implementation-independent lifecycle coverage, verify Local empty
file persistence and exactly one StreamSink EOS/S3 empty-object commit, and
make the Variant regression enforce its expected query error.
### Release note
Persist empty V2/V3 index files for all FileWriter implementations, including
wrappers, while preserving V1 and already-closed writer behavior.
### Check List (For Author)
- Test: Unit Test / Regression test / Manual test
- 64 focused ASAN BE tests passed; the new baseline suite first demonstrated 13 failures before the fix.
- test_variant_empty_index_file regression passed on the locally built cluster.
- ASAN BE/FE product build, clang-format 16, check-format and build hygiene passed.
- clang-tidy was attempted; an existing unmatched NOLINTEND in be/src/core/types.h:576 prevents a complete pass.
- Behavior changed: Yes; explicitly close empty index files for every writer implementation and skip already-closed writers.
- Does this need documentation: No
luwei16
requested review from
Gabriel39,
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
morningman
as code owners
September 9, 2026 04:10
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
luwei16
changed the title
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Dropping the last inverted index rebuilds the V2 index file without any logical index directories. Explicitly closing the writer preserves an empty file, so the reader now returns E-6004 instead of E-6003. Check the empty-file status and message in this regression, while keeping the missing-file expectation for indexes that have not been built.
### Release note
None
### Check List (For Author)
- Test: Regression test
- test_drop_index_with_format_v2 and test_add_build_index_with_format_v2 passed locally on both Apache Doris master and SelectDB 3.1.
- Behavior changed: No (test expectations follow the empty-file close fix)
- Does this need documentation: No
Contributor
TPC-H: Total hot run time: 16815 ms |
Contributor
TPC-DS: Total hot run time: 81593 ms |
Contributor
ClickBench: Total hot run time: 14.52 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16970 ms |
Contributor
TPC-DS: Total hot run time: 82902 ms |
Contributor
ClickBench: Total hot run time: 14.84 s |
airborne12
added a commit
that referenced
this pull request
Sep 16, 2026
…an ones (#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: An all-NULL Variant column creates no logical index directory, but its V2/V3 FileWriter has already been created. The empty-index close path only closed StreamSink, S3 and Packed writers, leaving LocalFileWriter and opaque wrappers open. Local writer destruction could consequently remove the empty file. Dispatch both close phases through FileWriter for every non-null, non-CLOSED writer, preserving the asynchronous protocol and returning close errors. Add implementation-independent lifecycle coverage, verify Local empty file persistence and exactly one StreamSink EOS/S3 empty-object commit, and make the Variant regression enforce its expected query error.
Release note
Persist empty V2/V3 index files for all FileWriter implementations, including wrappers, while preserving V1 and already-closed writer behavior.
Check List (For Author)