airborne12 opened a new pull request, #68082: URL: https://github.com/apache/doris/pull/68082
### What problem does this PR solve? Issue Number: N/A Related PR: https://github.com/apache/doris/pull/67859 Problem Summary: This backports source commit `8d9e9c3f6d0ff2186a6ac52bb572a6fbee457938` to `branch-4.1`. An all-NULL VARIANT column can legitimately produce a zero-byte V2/V3 index container. The old empty-file path closed only selected writer implementations, so a local writer could abort and delete the file during destruction. `IndexBuilder` also rejected an empty source container and could write an orphan container after the output schema lost its last index. The backport: 1. Closes empty containers through the `FileWriter` interface while preserving the two-phase close protocol and error propagation. 2. Treats an empty source index container like a missing one when there is nothing to carry over. 3. Avoids writing an index container when the output rowset schema owns no inverted or ANN index. `branch-4.1` has no SNII format, so the source PR's SNII dispatch and SNII-only unit test are not applicable. The V2/V3 behavior uses this branch's split `has_inverted_index()` / `has_ann_index()` predicates and older block APIs. ### Release note Persist legitimate empty V2/V3 inverted-index files, allow index rebuilds to consume them, and avoid orphan index files after the last index is gone. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason - Behavior changed: - [ ] No. - [x] Yes. Empty index containers are persisted and accepted, while schema-invisible orphan containers are no longer written. - Does this need documentation? - [x] No. This restores the existing rowset/index-file invariant. - [ ] Yes. ### Validation - `BUILD_TYPE=ASAN ./build.sh --be -j32`: passed. - `./build.sh --fe -j16`: passed, including Checkstyle. - Focused ASAN BE unit-test filter covering 18 parameterized `EmptyIndexFileTest` cases, five `IndexBuilderTest` cases, the null-array VARIANT case, and the S3 empty-file case: 25/25 passed twice, with no sanitizer report. - `test_empty_index_file_lifecycle`: passed 1/1 on an isolated classic cluster. - `test_variant_empty_index_file`: passed 1/1 on the same cluster. - Clang-format 16 selected-file check and full merge-tree preflight: passed. - PR English check across all eight changed source files: passed with no exceptions. - Targeted changed-line clang-tidy checks: passed. A full changed-file run is limited by the pre-existing unmatched `NOLINTEND` in `be/src/core/types.h` and legacy whole-function diagnostics in branch-4.1; no new changed-line diagnostic remains. ### Backport audit `git range-diff 8d9e9c3^! origin/branch-4.1...HEAD` was reviewed. Positional differences come from the older branch layout and APIs. The two regression-test file patches have stable patch IDs identical to the source. Every source hunk is accounted for below. | Source file | Source hunk | Disposition | |---|---|---| | `be/src/storage/index/index_file_writer.cpp` | `@@ -30 +29,0 @@` | Ported: remove the now-unneeded concrete S3 writer include. | | same | `@@ -511,4 +510,10 @@ Status IndexFileWriter::begin_close()` | Ported: close every non-null, non-closed `FileWriter`; the branch has no preceding SNII path. | | same | `@@ -559,4 +564,5 @@ Status IndexFileWriter::finish_close()` | Ported: preserve asynchronous finish and idempotently skip an already closed writer. | | `be/src/storage/task/index_builder.cpp` | `@@ -389 +389,8 @@ Status IndexBuilder::update_inverted_index_info()` | Adapted: accept `INVERTED_INDEX_BYPASS` exactly like missing input; comments use branch terminology. | | same | `@@ -449,2 +456,17 @@ Status IndexBuilder::handle_single_rowset(RowsetMetaSharedPtr output_rowset_meta` | Adapted: gate the drop rewrite with `has_inverted_index() || has_ann_index()` because this branch lacks the combined helper and SNII. | | same | `@@ -519 +540,0 @@ Status IndexBuilder::handle_single_rowset(RowsetMetaSharedPtr output_rowset_meta` | Adapted: defer filesystem resolution until after the no-index early return. | | same | `@@ -520,0 +542,12 @@ Status IndexBuilder::handle_single_rowset(RowsetMetaSharedPtr output_rowset_meta` | Adapted: add the same no-index output gate with the branch's split predicates. | | same | `@@ -524,0 +558,3 @@ Status IndexBuilder::handle_single_rowset(RowsetMetaSharedPtr output_rowset_meta` | Adapted: resolve `fs()` only for the V2/V3 path that will write a file; there is no SNII dispatch here. | | `be/test/io/fs/s3_file_writer_test.cpp` | `@@ -1533 +1533 @@ TEST_F(S3FileWriterTest, test_empty_file)` | Ported; also scopes `enable_file_cache=false` because the branch-4.1 fixture otherwise reaches an uninitialized cache factory. | | same | `@@ -1537,0 +1538,2 @@ TEST_F(S3FileWriterTest, test_empty_file)` | Ported: retain the writer pointer and object path for lifecycle/object assertions. | | same | `@@ -1545,2 +1547,14 @@ TEST_F(S3FileWriterTest, test_empty_file)` | Ported: verify two-phase close, idempotence, one zero-byte PUT, and no multipart upload. | | `be/test/storage/index/index_builder_test.cpp` | `@@ -23,0 +24 @@` | Ported: add the set container used to inspect output index IDs. | | same | `@@ -536,0 +538,123 @@ protected:` | Adapted: use branch-4.1 `create_block()` and scoped column mutation; preserve empty-vs-missing source coverage. Specific complexity suppressions document the assertion-heavy test helper. | | same | `@@ -709 +833,3 @@ TEST_F(IndexBuilderTest, DropInvertedIndexTest)` | Adapted: preserve the branch fixture while asserting that dropping the last index leaves no `.idx` file. | | same | `@@ -1027,0 +1154,20 @@ TEST_F(IndexBuilderTest, BuildInvertedIndexAfterWritingDataTest)` | Ported: build over both an empty and a missing source container and verify the requested output index. | | same | `@@ -3509,0 +3656,22 @@ TEST_F(IndexBuilderTest, DropOneIndexNotAffectOtherIndexesOnSameColumnTest)` | N/A: this is the source SNII-only no-surviving-index test; branch-4.1 defines neither the SNII storage format nor its builder path. The equivalent V2/V3 no-index behavior is covered by `NonExistentColumnIndexTest` and the drop tests. | | `be/test/storage/index/inverted/empty_index_file_test.cpp` | `@@ -22,0 +23 @@` | Adapted: add the branch's local filesystem header for persistence checks. | | same | `@@ -23,0 +25 @@` | Ported: add `IndexFileReader` for empty/missing error-code assertions. | | same | `@@ -25,0 +28 @@` | Ported: add the inverted-index descriptor used to resolve the compound-file path. | | same | `@@ -33 +36 @@ constexpr int64_t NUM_STREAM = 3;` | Adapted: convert the older non-parameterized fixture to V2/V3 parameterized coverage and retain the branch mock-stream baseline. | | same | `@@ -76,0 +80,52 @@ protected:` | Ported into the older fixture: an opaque `FileWriter` records begin/finish close state and failures without relying on a concrete writer type. | | same | `@@ -104,9 +159,11 @@ TEST_P(EmptyIndexFileTest, PreservesZeroByteFileWhenNoLogicalIndexes) {` | Adapted: initialize the branch's `StreamSinkFileWriter` and verify one EOS per stream with no data. | | same | `@@ -119,0 +177,117 @@ TEST_P(EmptyIndexFileTest, PreservesZeroByteFileWhenNoLogicalIndexes) {` | Ported into the older fixture: generic writer lifecycle/error/null tests plus local persistence and empty-vs-missing reader contracts for V2/V3. | | `be/test/storage/variant/index_storage_variant_debug_point_test.cpp` | `@@ -150,5 +150,2 @@ protected:` | N/A as a behavior change: branch-4.1 already expected an index file for the null-array case. The branch-specific debug-point expectation is preserved and an explanatory comment was added. | | `regression-test/suites/inverted_index_p0/test_empty_index_file_lifecycle.groovy` | Entire new-file hunk `@@ -0,0 +1,178 @@` | Ported verbatim; stable patch ID `557150e08942a2234b48414952ab35eaaf718596`. | | `regression-test/suites/inverted_index_p0/test_variant_empty_index_file.groovy` | All four source hunks | Ported verbatim; stable patch ID `d701fcc160d6289125fd56f93fee10c669487c24`. | ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label -- 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]
