Introduce magic_enum into tiflash #5843

Ziy1-Tan · 2022-09-11T07:34:04Z

What problem does this PR solve?

Issue Number: ref #5758

Problem Summary:

What is changed and how it works?

Neargye/magic_enum is a static reflection enumeration library, It make us to work with any enum type without any macro or boilerplate code without loss of performance;
I introduce Neargye/magic_enum into tiflash via git submodule;
The tests are located in dbms/src/Common/tests/gtest_magic_enum.cpp, which perfectly makes the conversion between the enum type and the enum name.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2022-09-11T07:34:06Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

breezewish
fuzhe1989

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2022-09-11T07:34:13Z

Welcome @Ziy1-Tan!

It looks like this is your first PR to pingcap/tiflash 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to pingcap/tiflash. 😃

sre-bot · 2022-09-11T07:34:19Z

All committers have signed the CLA.

Ziy1-Tan · 2022-09-11T07:38:29Z

Hey, @ywqzzy @JaySon-Huang. Do I need more tests, or do I need to modify the existing enum related code with magic_enum, what part should I do next? I would really appreciate if you guys could give me some guidance:).

Lloyd-Pottiger

@Ziy1-Tan Thanks for your excellent work! Could you please add a little bit more tests like the use of different api which not only test the correctness of the submodule, but also give an example of usage. And feel free to modify the existing enum related code.

Lloyd-Pottiger · 2022-09-12T02:53:18Z

dbms/src/Common/tests/gtest_magic_enum.cpp

+#include <gtest/gtest.h>
+
+#include <magic_enum.hpp>
+using namespace magic_enum;


Suggested change

using namespace magic_enum;

Nice one! I'll consider to add more tests using api which may replace existing enum related code.

I evaluated the enum usage in existing projects and added the magic API test cases for accessing and converting enum types, I can add some more advanced APIs in the future if needed.

breezewish · 2022-09-12T06:06:01Z

@Ziy1-Tan I think you can pick one of the enum in the code base which self-implemented some features that magic_enum provides, and use magic_enum to simplify it. This should be sufficient as a working usage example and why we would like to introduce magic_enum.

For example, you may consider ThreadType:

tiflash/dbms/src/Storages/DeltaMerge/DeltaMergeStore.h

Line 160 in 51b9ae4

enum ThreadType

which does not need a wide refactor and you may use magic_enum to drop the toString(ThreadType).

Ziy1-Tan · 2022-09-12T06:16:43Z

@Ziy1-Tan I think you can pick one of the enum in the code base which self-implemented some features that magic_enum provides, and use magic_enum to simplify it. This should be sufficient as a working usage example and why we would like to introduce magic_enum.

For example, you may consider ThreadType:

tiflash/dbms/src/Storages/DeltaMerge/DeltaMergeStore.h

Line 160 in 51b9ae4

enum ThreadType

which does not need a wide refactor and you may use magic_enum to drop the toString(ThreadType).

Good! I'll try it.

JaySon-Huang · 2022-09-13T04:22:34Z

I think using the v0.8.1 43a9272 instead of the 1b1194b from master is better for tracking the third-party library version

breezewish

Cool!

breezewish · 2022-09-14T15:31:13Z

@Ziy1-Tan Could you rebase your change over the latest master? We introduced several enum change recently.., which brings some conflict with this PR.

breezewish · 2022-09-14T15:31:40Z

@Lloyd-Pottiger Please take a look for the recent change

Ziy1-Tan · 2022-09-14T15:35:35Z

@Lloyd-Pottiger Please take a look for the recent change

OK, Let me check it.

dbms/src/Storages/DeltaMerge/DeltaMergeStore_InternalSegment.cpp

.gitmodules

Lloyd-Pottiger · 2022-09-14T16:31:35Z

dbms/src/Common/tests/gtest_magic_enum.cpp

+        ASSERT_EQ(entry.first, magic_enum::enum_cast<Mode>(entry.second));
+    }
+}
+} // namespace DB::tests


could you please add for some unusual case like magic_enum::enum_name(-1), magic_enum::enum_name(999999999) ?

magic_enum::enum_name based on reflection allows us to catch errors before compiling and can only pass enums instead of integers. So it's not easy to test. But I test ohters API in unusal cases.

Ziy1-Tan · 2022-09-15T02:58:48Z

/cc @Lloyd-Pottiger PTAL.

Lloyd-Pottiger · 2022-09-15T03:31:36Z

/run-all-tests

sre-bot · 2022-09-15T03:55:48Z

Coverage for changed files

Filename                                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/tests/gtest_magic_enum.cpp                                14                 0   100.00%           1                 0   100.00%          24                 0   100.00%           2                 0   100.00%
Storages/DeltaMerge/DeltaMergeStore.cpp                         925               160    82.70%          48                 2    95.83%        1378               233    83.09%         494               160    67.61%
Storages/DeltaMerge/DeltaMergeStore.h                            17                 3    82.35%          16                 3    81.25%          50                11    78.00%           0                 0         -
Storages/DeltaMerge/DeltaMergeStore_InternalBg.cpp              343               261    23.91%          13                 4    69.23%         483               262    45.76%         176               118    32.95%
Storages/DeltaMerge/DeltaMergeStore_InternalSegment.cpp         416               206    50.48%           4                 0   100.00%         335                81    75.82%         112                57    49.11%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                          1715               630    63.27%          82                 9    89.02%        2270               587    74.14%         784               335    57.27%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18744      8126             56.65%    217087  83475        61.55%

full coverage report (for internal network access only)

JaySon-Huang

LGTM

Thank you for your work!

Lloyd-Pottiger · 2022-09-15T05:50:11Z

/merge

ti-chi-bot · 2022-09-15T05:50:12Z

@Lloyd-Pottiger: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-09-15T05:50:13Z

This pull request has been accepted and is ready to merge.

Commit hash: 21657a5

sre-bot · 2022-09-15T06:32:30Z

Coverage for changed files

Filename                                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/tests/gtest_magic_enum.cpp                                14                 0   100.00%           1                 0   100.00%          24                 0   100.00%           2                 0   100.00%
Storages/DeltaMerge/DeltaMergeStore.cpp                         925               160    82.70%          48                 2    95.83%        1378               233    83.09%         494               157    68.22%
Storages/DeltaMerge/DeltaMergeStore.h                            17                 3    82.35%          16                 3    81.25%          50                11    78.00%           0                 0         -
Storages/DeltaMerge/DeltaMergeStore_InternalBg.cpp              343               261    23.91%          13                 4    69.23%         483               262    45.76%         176               118    32.95%
Storages/DeltaMerge/DeltaMergeStore_InternalSegment.cpp         416               206    50.48%           4                 0   100.00%         335                81    75.82%         112                57    49.11%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                          1715               630    63.27%          82                 9    89.02%        2270               587    74.14%         784               332    57.65%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18750      8129             56.65%    217175  83488        61.56%

full coverage report (for internal network access only)

ti-chi-bot added the release-note-none label Sep 11, 2022

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 11, 2022

introduce magic_enum into tiflash

010ea08

Ziy1-Tan force-pushed the feat_magic_enum branch from c6a160c to 010ea08 Compare September 11, 2022 07:38

fuzhe1989 approved these changes Sep 11, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 11, 2022

Lloyd-Pottiger reviewed Sep 12, 2022

View reviewed changes

replace DeltaMergeStore::toString with magic_enum:enum_name

fb092dc

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 14, 2022

introduce magic_enum into tiflash

904ceb4

breezewish approved these changes Sep 14, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 14, 2022

Merge branch 'master' into feat_magic_enum

a3c5c74

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2022

Lloyd-Pottiger reviewed Sep 14, 2022

View reviewed changes

dbms/src/Storages/DeltaMerge/DeltaMergeStore_InternalSegment.cpp Outdated Show resolved Hide resolved

Update dbms/src/Storages/DeltaMerge/DeltaMergeStore_InternalSegment.cpp

a00c767

Lloyd-Pottiger reviewed Sep 14, 2022

View reviewed changes

.gitmodules Outdated Show resolved Hide resolved

add more test

3b90131

Lloyd-Pottiger reviewed Sep 14, 2022

View reviewed changes

add edge test cases

21657a5

JaySon-Huang approved these changes Sep 15, 2022

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 15, 2022

ti-chi-bot added 2 commits September 15, 2022 13:50

Merge branch 'master' into feat_magic_enum

5f0f6d5

Merge branch 'master' into feat_magic_enum

6e52ac3

ti-chi-bot merged commit f4e976b into pingcap:master Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce magic_enum into tiflash #5843

Introduce magic_enum into tiflash #5843

Ziy1-Tan commented Sep 11, 2022 •

edited by breezewish

ti-chi-bot commented Sep 11, 2022 •

edited

ti-chi-bot commented Sep 11, 2022

sre-bot commented Sep 11, 2022 •

edited

Ziy1-Tan commented Sep 11, 2022

Lloyd-Pottiger left a comment

Lloyd-Pottiger Sep 12, 2022

Ziy1-Tan Sep 12, 2022

Ziy1-Tan Sep 14, 2022

breezewish commented Sep 12, 2022

Ziy1-Tan commented Sep 12, 2022 •

edited

JaySon-Huang commented Sep 13, 2022

breezewish left a comment

breezewish commented Sep 14, 2022

breezewish commented Sep 14, 2022

Ziy1-Tan commented Sep 14, 2022

Lloyd-Pottiger Sep 14, 2022

Ziy1-Tan Sep 14, 2022

Ziy1-Tan Sep 15, 2022 •

edited

Lloyd-Pottiger Sep 15, 2022

Ziy1-Tan commented Sep 15, 2022

Lloyd-Pottiger commented Sep 15, 2022

sre-bot commented Sep 15, 2022

JaySon-Huang left a comment •

edited

Lloyd-Pottiger commented Sep 15, 2022

ti-chi-bot commented Sep 15, 2022

ti-chi-bot commented Sep 15, 2022

sre-bot commented Sep 15, 2022

Introduce magic_enum into tiflash #5843

Introduce magic_enum into tiflash #5843

Conversation

Ziy1-Tan commented Sep 11, 2022 • edited by breezewish

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 11, 2022 • edited

ti-chi-bot commented Sep 11, 2022

sre-bot commented Sep 11, 2022 • edited

Ziy1-Tan commented Sep 11, 2022

Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

Lloyd-Pottiger Sep 12, 2022

Choose a reason for hiding this comment

Ziy1-Tan Sep 12, 2022

Choose a reason for hiding this comment

Ziy1-Tan Sep 14, 2022

Choose a reason for hiding this comment

breezewish commented Sep 12, 2022

Ziy1-Tan commented Sep 12, 2022 • edited

JaySon-Huang commented Sep 13, 2022

breezewish left a comment

Choose a reason for hiding this comment

breezewish commented Sep 14, 2022

breezewish commented Sep 14, 2022

Ziy1-Tan commented Sep 14, 2022

Lloyd-Pottiger Sep 14, 2022

Choose a reason for hiding this comment

Ziy1-Tan Sep 14, 2022

Choose a reason for hiding this comment

Ziy1-Tan Sep 15, 2022 • edited

Choose a reason for hiding this comment

Lloyd-Pottiger Sep 15, 2022

Choose a reason for hiding this comment

Ziy1-Tan commented Sep 15, 2022

Lloyd-Pottiger commented Sep 15, 2022

sre-bot commented Sep 15, 2022

Coverage for changed files

Coverage summary

JaySon-Huang left a comment • edited

Choose a reason for hiding this comment

Lloyd-Pottiger commented Sep 15, 2022

ti-chi-bot commented Sep 15, 2022

ti-chi-bot commented Sep 15, 2022

sre-bot commented Sep 15, 2022

Coverage for changed files

Coverage summary

Ziy1-Tan commented Sep 11, 2022 •

edited by breezewish

ti-chi-bot commented Sep 11, 2022 •

edited

sre-bot commented Sep 11, 2022 •

edited

Ziy1-Tan commented Sep 12, 2022 •

edited

Ziy1-Tan Sep 15, 2022 •

edited

JaySon-Huang left a comment •

edited