ORC-1658: [C++] Unify identifiers naming style. #1855

ffacs · 2024-03-18T16:04:25Z

What changes were proposed in this pull request?

Add run_clang_tidy.py which comes from LLVM and clang-tidy configuration file.
Use clang-tidy to format the C++ code.

Why are the changes needed?

To close #1845

How was this patch tested?

UT passed

Was this patch authored or co-authored using generative AI tooling?

NO

1. cd builf && python3 ../run_clang_tidy.py -p . -fix 2. git ls-files --exclude-standard | grep ".*$cc\|hh$$" | xargs clang-format -i -style=file --dump-config 3. add lisence to run_clang_tidy.py and clang-tidy config file

ffacs · 2024-03-20T17:36:09Z

This patch I`ve done the following things:

run clang-tidy cd builf && python3 ../run_clang_tidy.py -p . -fix
format codes

git ls-files --exclude-standard | grep ".*\(cc\|hh\)$" | xargs clang-format -i -style=file --dump-config

add licence to run_clang_tidy.py and clang-tidy config file

Currently the patch can be built on my macOS(arm Apple clang version 15.0.0) and ubuntu(x64 clang-18) and passes UTs with mkdir build -p && cd build && cmake .. -DBUILD_JAVA=OFF -DCMAKE_BUILD_TYPE=DEBUG -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang -DCMAKE_EXPORT_COMPILE_COMMANDS=1 && make -j20 && make test .

wgtmac

Thanks for the work! I've left some comments. Most are about the requests to remove m prefix. This can be a separate patch.

wgtmac · 2024-03-21T09:17:12Z

c++/include/orc/sargs/Literal.hh

-    int32_t mScale;           // scale of decimal type
-    bool mIsNull;             // whether this literal is null
-    size_t mHashCode;         // precomputed hash code for the literal
+    LiteralVal mValue_;        // data value for this literal if not null


Is it possible to remove the m prefix?

wgtmac · 2024-03-21T09:21:47Z

c++/src/BloomFilter.hh

@@ -90,7 +90,7 @@ namespace orc {
    bool operator==(const BitSet& other) const;

   private:
-    std::vector<uint64_t> mData;
+    std::vector<uint64_t> mData_;


ditto, would be good to remove m prefix here and below.

wgtmac · 2024-03-21T09:32:52Z

c++/src/sargs/ExpressionTree.hh

-    std::vector<TreeNode> mChildren;
-    size_t mLeaf;
-    TruthValue mConstant;
+    Operator mOperator_;


wgtmac · 2024-03-21T09:33:23Z

c++/src/sargs/PredicateLeaf.hh

-    uint64_t mColumnId;
-    std::vector<Literal> mLiterals;
-    size_t mHashCode;
+    Operator mOperator_;


wgtmac · 2024-03-21T09:33:42Z

c++/src/sargs/SargsApplier.hh

-    const SchemaEvolution* mSchemaEvolution;
-    uint64_t mRowIndexStride;
-    WriterVersion mWriterVersion;
+    const Type& mType_;


wgtmac · 2024-03-21T09:34:12Z

c++/src/sargs/SearchArgument.hh

@@ -66,8 +66,8 @@ namespace orc {
    std::string toString() const override;

   private:
-    std::shared_ptr<ExpressionTree> mExpressionTree;
-    std::vector<PredicateLeaf> mLeaves;
+    std::shared_ptr<ExpressionTree> mExpressionTree_;


wgtmac · 2024-03-21T09:36:51Z

run_clang_tidy.py

+  sys.exit(return_code)
+
+if __name__ == '__main__':
+  main()


Is it possible to run this in the CI?

Is it possible to run this in the CI?

Absolutely. We can leverage clang-tidy/tool/clang-tidy-diff.py

Perhaps we can leverage Github Action like https://github.com/marketplace/actions/c-c-linter or similar actions from here: https://github.com/marketplace?category=&type=actions&verification=&query=sort%3Apopularity-desc+clang-tidy

I have added two actions in the past:

apache/skywalking-eyes@main for license header check
ORC-1395: Enforce license check via github action #1442

jidicula/clang-format-action@v4.9.0 for clang-format check
ORC-1307: Add .clang-format to enforce C++ code style #1308

It seems that we can use a single linter action to run both clang-tidy and clang-format.

stiga-huang · 2024-03-26T07:54:23Z

c++/include/orc/Exceptions.hh

-    explicit NotImplementedYet(const std::string& what_arg);
-    explicit NotImplementedYet(const char* what_arg);
+    explicit NotImplementedYet(const std::string& whatArg);
+    explicit NotImplementedYet(const char* whatArg);


Do we consider using snake case instead of mixed case (camel case) for variable names? Impala C++ Code follows a modified version of the Google C++ Style Guide at:
https://google.github.io/styleguide/cppguide.html#Variable_Names

I think either way is ok. Just remembered that when I first read the ORC C++ code, they look like Java code. :)

There are lots of public/protected variables using camel case, if we change them into snake case, the library codes would be not backwards compatible. Backwards compatibility is important, I think.

+1 for keeping the compatibility.

I see. That makes sense to me.

ffacs · 2024-03-28T10:03:41Z

@dongjoon-hyun Hi, do you have any suggestions about this patch?

dongjoon-hyun · 2024-03-28T16:58:32Z

@dongjoon-hyun Hi, do you have any suggestions about this patch?

Although I've been monitoring this PR and discussion, there is no other objection for the AS-IS direction including backward compatibility consideration.

wgtmac

I'm fine to merge this PR as is. We can add follow-ups to address remaining issues as this one is already very large.

dongjoon-hyun · 2024-04-02T17:19:05Z

Thank you!

github-actions bot added CPP INFRA labels Mar 18, 2024

run clang-tidy and clang-format

9d0c457

1. cd builf && python3 ../run_clang_tidy.py -p . -fix 2. git ls-files --exclude-standard | grep ".*$cc\|hh$$" | xargs clang-format -i -style=file --dump-config 3. add lisence to run_clang_tidy.py and clang-tidy config file

ffacs force-pushed the NamingStyle branch from b5ae81d to 9d0c457 Compare March 20, 2024 16:58

Fix license and format check

a0f7d79

wgtmac reviewed Mar 21, 2024

View reviewed changes

stiga-huang reviewed Mar 26, 2024

View reviewed changes

wgtmac changed the title ~~ORC-1658: [C++] uniform identifiers naming style.~~ ORC-1658: [C++] Unify identifiers naming style. Mar 27, 2024

wgtmac approved these changes Mar 29, 2024

View reviewed changes

wgtmac closed this in 03ce308 Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-1658: [C++] Unify identifiers naming style. #1855

ORC-1658: [C++] Unify identifiers naming style. #1855

ffacs commented Mar 18, 2024

ffacs commented Mar 20, 2024

wgtmac left a comment

wgtmac Mar 21, 2024

wgtmac Mar 21, 2024

wgtmac Mar 21, 2024

wgtmac Mar 21, 2024

wgtmac Mar 21, 2024

wgtmac Mar 21, 2024

wgtmac Mar 21, 2024

ffacs Mar 21, 2024

wgtmac Mar 27, 2024

stiga-huang Mar 26, 2024

ffacs Mar 26, 2024

wgtmac Mar 27, 2024

stiga-huang Mar 28, 2024

ffacs commented Mar 28, 2024

dongjoon-hyun commented Mar 28, 2024

wgtmac left a comment

dongjoon-hyun commented Apr 2, 2024

ORC-1658: [C++] Unify identifiers naming style. #1855

ORC-1658: [C++] Unify identifiers naming style. #1855

Conversation

ffacs commented Mar 18, 2024

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

ffacs commented Mar 20, 2024

wgtmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffacs commented Mar 28, 2024

dongjoon-hyun commented Mar 28, 2024

wgtmac left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Apr 2, 2024