-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix](inverted index) fix error when writing empty index file #51984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures that copying an empty inverted index segment no longer throws an exception and adds a unit test to cover this scenario.
- Gracefully skip empty index files (
CL_ERR_EmptyIndexSegment) inInvertedIndexFileWriter::copyFile - Introduce
CopyFileEmptyFileTestto verify empty-file behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| be/src/olap/rowset/segment_v2/inverted_index_file_writer.cpp | Added a check for CL_ERR_EmptyIndexSegment to log a warning and return instead of throwing |
| be/test/olap/rowset/segment_v2/inverted_index_file_writer_test.cpp | Added CopyFileEmptyFileTest to ensure copyFile handles empty files without error |
Comments suppressed due to low confidence (2)
be/test/olap/rowset/segment_v2/inverted_index_file_writer_test.cpp:1569
- After invoking
copyFile, add an assertion to verify that the output stream remains empty or has a length of zero, to confirm the empty-file case is handled correctly.
EXPECT_NO_THROW({ writer.copyFile("_0.frq", mock_dir.get(), output, buffer, sizeof(buffer)); });
be/src/olap/rowset/segment_v2/inverted_index_file_writer.cpp:269
- [nitpick] Add a brief comment explaining that empty index segments are intentionally skipped here to prevent throwing when no data is present.
if (err.number() == CL_ERR_EmptyIndexSegment) {
| }); | ||
| if (!open) { | ||
| if (err.number() == CL_ERR_EmptyIndexSegment) { | ||
| LOG(WARNING) << "InvertedIndexFileWriter::copyFile: " << fileName << " is empty"; |
Copilot
AI
Jun 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider including the full file path or segment identifiers in the log message for better traceability when debugging.
| LOG(WARNING) << "InvertedIndexFileWriter::copyFile: " << fileName << " is empty"; | |
| LOG(WARNING) << "InvertedIndexFileWriter::copyFile: " << dir->getFullPath(fileName) << " is empty"; |
eldenmoon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test for v1, v2
CREATE TABLE `test` (
`k` int NOT NULL,
`v` string,
INDEX idx_v_a (v) USING INVERTED PROPERTIES("parser" = "english")
) ENGINE=OLAP
UNIQUE KEY(`k`)
DISTRIBUTED BY HASH(`k`) BUCKETS 1
PROPERTIES (
"replication_num" = "1"
);
insert into test values (427009, '');
zzzxl1993
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…#51984) Related PR: apache#51393 Problem Summary: This PR ensures that copying an empty inverted index segment no longer throws an exception.
…#51984) Related PR: apache#51393 Problem Summary: This PR ensures that copying an empty inverted index segment no longer throws an exception.
…#51984) Related PR: apache#51393 Problem Summary: This PR ensures that copying an empty inverted index segment no longer throws an exception.
…file apache#51984 (apache#52690) cherry pick from apache#51984 --------- Co-authored-by: airborne12 <jiangkai@selectdb.com>
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #51393
Problem Summary:
This PR ensures that copying an empty inverted index segment no longer throws an exception.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)