Skip to content
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

[Refactor](inverted index) refactor inverted index file writer for v1/v2 index write #42328

Merged
merged 10 commits into from
Nov 14, 2024

Conversation

airborne12
Copy link
Member

Proposed changes

The original write_v1 and write_v2 methods in the inverted index file writer were overly long and difficult to maintain. This PR refactors these methods to improve code maintainability and readability.

@doris-robot
Copy link

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

@airborne12
Copy link
Member Author

run buildall

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@airborne12
Copy link
Member Author

run buildall

Copy link
Contributor

github-actions bot commented Nov 5, 2024

clang-tidy review says "All clean, LGTM! 👍"

@airborne12
Copy link
Member Author

run buildall

Copy link
Contributor

github-actions bot commented Nov 6, 2024

clang-tidy review says "All clean, LGTM! 👍"

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.92% (9863/26011)
Line Coverage: 29.05% (81938/282068)
Region Coverage: 28.27% (42198/149246)
Branch Coverage: 24.85% (21407/86134)
Coverage Report: http://coverage.selectdb-in.cc/coverage/83c58f15157571770657d41ec88370c265433858_83c58f15157571770657d41ec88370c265433858/report/index.html

@airborne12
Copy link
Member Author

run p1

@airborne12
Copy link
Member Author

run cloud_p1

@airborne12
Copy link
Member Author

run buildall

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@airborne12
Copy link
Member Author

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment on lines +29 to +30
namespace doris {
namespace segment_v2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]

Suggested change
namespace doris {
namespace segment_v2 {
namespace doris::segment_v2 {

be/test/olap/rowset/segment_v2/inverted_index_file_writer_test.cpp:377:

- } // namespace segment_v2
- } // namespace doris
+ } // namespace doris

Copy link
Contributor

@csun5285 csun5285 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 148 and line 163 is duplicated.

@airborne12
Copy link
Member Author

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment on lines +29 to +30
namespace doris {
namespace segment_v2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]

Suggested change
namespace doris {
namespace segment_v2 {
namespace doris::segment_v2 {

be/test/olap/rowset/segment_v2/inverted_index_file_writer_test.cpp:424:

- } // namespace segment_v2
- } // namespace doris
+ } // namespace doris

@airborne12
Copy link
Member Author

run buildall

@airborne12
Copy link
Member Author

run buildall

@airborne12
Copy link
Member Author

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment on lines +29 to +30
namespace doris {
namespace segment_v2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]

Suggested change
namespace doris {
namespace segment_v2 {
namespace doris::segment_v2 {

be/test/olap/rowset/segment_v2/inverted_index_file_writer_test.cpp:509:

- } // namespace segment_v2
- } // namespace doris
+ } // namespace doris

Copy link
Contributor

@csun5285 csun5285 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

PR approved by anyone and no changes requested.

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.99% (9903/26068)
Line Coverage: 29.17% (82719/283555)
Region Coverage: 28.30% (42505/150219)
Branch Coverage: 24.87% (21554/86670)
Coverage Report: http://coverage.selectdb-in.cc/coverage/dc539c3e638ee8e05371cc96bd375fab06985ec3_dc539c3e638ee8e05371cc96bd375fab06985ec3/report/index.html

@airborne12
Copy link
Member Author

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment on lines +29 to +30
namespace doris {
namespace segment_v2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]

Suggested change
namespace doris {
namespace segment_v2 {
namespace doris::segment_v2 {

be/test/olap/rowset/segment_v2/inverted_index_file_writer_test.cpp:513:

- } // namespace segment_v2
- } // namespace doris
+ } // namespace doris

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.98% (9903/26075)
Line Coverage: 29.17% (82733/283638)
Region Coverage: 28.29% (42509/150250)
Branch Coverage: 24.87% (21557/86688)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ab8b8fed66493c7de82e7e0a111bb566c73e05af_ab8b8fed66493c7de82e7e0a111bb566c73e05af/report/index.html

@airborne12
Copy link
Member Author

run buildall

@airborne12
Copy link
Member Author

run buildall

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@airborne12
Copy link
Member Author

run buildall

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.98% (9902/26073)
Line Coverage: 29.17% (82725/283622)
Region Coverage: 28.30% (42512/150245)
Branch Coverage: 24.87% (21554/86680)
Coverage Report: http://coverage.selectdb-in.cc/coverage/00b6857966c97d91bd23353b782f1c9e981e0ea1_00b6857966c97d91bd23353b782f1c9e981e0ea1/report/index.html

@airborne12
Copy link
Member Author

run buildall

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.97% (9901/26073)
Line Coverage: 29.16% (82713/283622)
Region Coverage: 28.29% (42501/150245)
Branch Coverage: 24.86% (21549/86680)
Coverage Report: http://coverage.selectdb-in.cc/coverage/aea3c3e44e706eea6e9835951e378e1594dba8c7_aea3c3e44e706eea6e9835951e378e1594dba8c7/report/index.html

Copy link
Member

@eldenmoon eldenmoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 14, 2024
@airborne12 airborne12 merged commit 581faa3 into apache:master Nov 14, 2024
24 of 26 checks passed
@airborne12 airborne12 deleted the enhance-case branch November 14, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. dev/3.0.x reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants