[C++] Lint in CI and various MacOS build improvements

Makefile

@@ -63,7 +63,10 @@ cpptest:
 	@echo ">>> Running C++ Tests/Snippets <<<\n"
 	rm -rf cpp/recipe-test-build
 	mkdir cpp/recipe-test-build
-	cd cpp/recipe-test-build && cmake ../code -DCMAKE_BUILD_TYPE=Debug && cmake --build . && ctest --output-on-failure -j 1
+	cd cpp/recipe-test-build && \
+		cmake ../code -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_COMPILER_LAUNCHER=ccache && \
+		cmake --build . && \
+		ctest --output-on-failure -j 1
 	mkdir -p cpp/build
 	cp cpp/recipe-test-build/recipes_out.arrow cpp/build
 

cpp/CONTRIBUTING.md

@@ -107,6 +107,11 @@ cmake --build .
 ctest
 ```
 
+(Tip: If you are using MacOS with Homebrew, you may need to `unset CPATH` to make
+sure the compiler finds the conda include path instead of the homebrew one. Otherwise
+you may encounter linking errors due to mismatches between header file and library
+versions.)
+
 Then you can rerun all of the tests with `ctest` and you can rebuild and
 rerun individual tests much more quickly with something like
 `cmake --build . --target creating_arrow_objects && ctest creating_arrow_objects`.
@@ -151,6 +156,10 @@ This cookbook follows the same style rules as Arrow C++ which is the Google styl
 guide with a few exceptions described
 [here](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci)
 
+We do follow a stricter clang-tidy rule set than Arrow. In general, we use all 
+rules provided by clang-tidy and exclude ones that don't make sense for the
+project. See `cpp/code/.clang-tidy`.
+
 ## Simple
 
 The examples should be as simple as possible. If complex code (e.g. templates) can be

cpp/code/.clang-tidy

@@ -17,6 +17,13 @@
 ---
 Checks: '*,-llvmlibc*,-cert-err58-cpp,-modernize-use-trailing-return-type,-fuchsia-*,-cppcoreguidelines-*,
   -readability-magic-numbers,-clang-analyzer-cplusplus.NewDelete,-clang-analyzer-cplusplus.NewDeleteLeaks,
-  -readability-function-cognitive-complexity, -hicpp-special-member-functions'
+  -readability-function-cognitive-complexity,-readability-named-parameter,
+  -readability-convert-member-functions-to-static,-hicpp-special-member-functions,-hicpp-named-parameter,
+  -altera-*,-misc-no-recursion'
 WarningsAsErrors: '*'
 FormatStyle: 'file'
+CheckOptions:
+  - key: performance-unnecessary-value-param.AllowedTypes
+    value: 'shared_ptr'
+  - key: performance-for-range-copy.AllowedTypes
+    value: 'shared_ptr'

cpp/code/CMakeLists.txt

@@ -2,16 +2,12 @@ cmake_minimum_required(VERSION 3.19)
 project(arrow-cookbook)
 
 set(CMAKE_CXX_STANDARD 17)
-if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libstdc++")
-endif()
 
 # Add Arrow
 find_package(Arrow REQUIRED COMPONENTS dataset flight parquet)
 
-if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
-    set(CMAKE_CXX_CLANG_TIDY "clang-tidy")
-endif()
+# You may wish to comment this line out to temporarily ignore pendantic linting errors
+set(CMAKE_CXX_CLANG_TIDY "clang-tidy")
 
 # Create test targets
 enable_testing()

cpp/code/creating_arrow_objects.cc

@@ -65,17 +65,15 @@ arrow::Status CreatingArraysPtr() {
 /// For demonstration purposes, this only covers DoubleType and ListType
 class RandomBatchGenerator {
  public:
-  std::shared_ptr<arrow::Schema> schema;
-
-  RandomBatchGenerator(std::shared_ptr<arrow::Schema> schema) : schema(schema){};
+  explicit RandomBatchGenerator(std::shared_ptr<arrow::Schema> schema) : schema_(std::move(schema)), num_rows_(0) {};
 
   arrow::Result<std::shared_ptr<arrow::RecordBatch>> Generate(int32_t num_rows) {
     num_rows_ = num_rows;
-    for (std::shared_ptr<arrow::Field> field : schema->fields()) {
+    for (std::shared_ptr<arrow::Field> field : schema_->fields()) {
       ARROW_RETURN_NOT_OK(arrow::VisitTypeInline(*field->type(), this));
     }
 
-    return arrow::RecordBatch::Make(schema, num_rows, arrays_);
+    return arrow::RecordBatch::Make(schema_, num_rows, arrays_);
   }
 
   // Default implementation
@@ -87,7 +85,7 @@ class RandomBatchGenerator {
     auto builder = arrow::DoubleBuilder();
     std::normal_distribution<> d{/*mean=*/5.0, /*stddev=*/2.0};
     for (int32_t i = 0; i < num_rows_; ++i) {
-      builder.Append(d(gen_));
+      ARROW_RETURN_NOT_OK(builder.Append(d(gen_)));
     }
     ARROW_ASSIGN_OR_RAISE(auto array, builder.Finish());
     arrays_.push_back(array);
@@ -98,11 +96,11 @@ class RandomBatchGenerator {
     // Generate offsets first, which determines number of values in sub-array
     std::poisson_distribution<> d{/*mean=*/4};
     auto builder = arrow::Int32Builder();
-    builder.Append(0);
+    ARROW_RETURN_NOT_OK(builder.Append(0));
     int32_t last_val = 0;
     for (int32_t i = 0; i < num_rows_; ++i) {
       last_val += d(gen_);
-      builder.Append(last_val);
+      ARROW_RETURN_NOT_OK(builder.Append(last_val));
     }
     ARROW_ASSIGN_OR_RAISE(auto offsets, builder.Finish());
 
@@ -119,7 +117,8 @@ class RandomBatchGenerator {
     return arrow::Status::OK();
   }
 
- protected:
+ private:
+  std::shared_ptr<arrow::Schema> schema_;
   std::random_device rd_{};
   std::mt19937 gen_{rd_()};
   std::vector<std::shared_ptr<arrow::Array>> arrays_;

cpp/code/flight.cc

@@ -41,9 +41,8 @@
 #include "common.h"
 
 class ParquetStorageService : public arrow::flight::FlightServerBase {
- public:
   const arrow::flight::ActionType kActionDropDataset{"drop_dataset", "Delete a dataset."};
-
+ public:
   explicit ParquetStorageService(std::shared_ptr<arrow::fs::FileSystem> root)
       : root_(std::move(root)) {}
 
@@ -56,7 +55,9 @@ class ParquetStorageService : public arrow::flight::FlightServerBase {
 
     std::vector<arrow::flight::FlightInfo> flights;
     for (const auto& file_info : listing) {
-      if (!file_info.IsFile() || file_info.extension() != "parquet") continue;
+      if (!file_info.IsFile() || file_info.extension() != "parquet") {
+        continue;
+      }
       ARROW_ASSIGN_OR_RAISE(auto info, MakeFlightInfo(file_info));
       flights.push_back(std::move(info));
     }
@@ -71,8 +72,7 @@ class ParquetStorageService : public arrow::flight::FlightServerBase {
                               std::unique_ptr<arrow::flight::FlightInfo>* info) override {
     ARROW_ASSIGN_OR_RAISE(auto file_info, FileInfoFromDescriptor(descriptor));
     ARROW_ASSIGN_OR_RAISE(auto flight_info, MakeFlightInfo(file_info));
-    *info = std::unique_ptr<arrow::flight::FlightInfo>(
-        new arrow::flight::FlightInfo(std::move(flight_info)));
+    *info = std::make_unique<arrow::flight::FlightInfo>(std::move(flight_info));
     return arrow::Status::OK();
   }
 
@@ -163,7 +163,8 @@ class ParquetStorageService : public arrow::flight::FlightServerBase {
       const arrow::flight::FlightDescriptor& descriptor) {
     if (descriptor.type != arrow::flight::FlightDescriptor::PATH) {
       return arrow::Status::Invalid("Must provide PATH-type FlightDescriptor");
-    } else if (descriptor.path.size() != 1) {
+    }
+    if (descriptor.path.size() != 1) {
       return arrow::Status::Invalid(
           "Must provide PATH-type FlightDescriptor with one path component");
     }
@@ -244,7 +245,9 @@ arrow::Status TestPutGetDelete() {
   int64_t batches = 0;
   while (true) {
     ARROW_ASSIGN_OR_RAISE(auto batch, batch_reader->Next());
-    if (!batch) break;
+    if (!batch) {
+      break;
+    }
     ARROW_RETURN_NOT_OK(writer->WriteRecordBatch(*batch));
     batches++;
   }
@@ -270,7 +273,7 @@ arrow::Status TestPutGetDelete() {
   ARROW_RETURN_NOT_OK(client->DoGet(flight_info->endpoints()[0].ticket, &stream));
   std::shared_ptr<arrow::Table> table;
   ARROW_RETURN_NOT_OK(stream->ReadAll(&table));
-  arrow::PrettyPrintOptions print_options(/*indent=*/0, /*window=*/2);
+  arrow::PrettyPrintOptions print_options(/*indent_arg=*/0, /*window_arg=*/2);
   ARROW_RETURN_NOT_OK(arrow::PrettyPrint(*table, print_options, &rout));
   EndRecipe("ParquetStorageService::DoGet");
 
@@ -288,7 +291,9 @@ arrow::Status TestPutGetDelete() {
   while (true) {
     std::unique_ptr<arrow::flight::FlightInfo> flight_info;
     ARROW_RETURN_NOT_OK(listing->Next(&flight_info));
-    if (!flight_info) break;
+    if (!flight_info) {
+      break;
+    }
     rout << flight_info->descriptor().ToString() << std::endl;
     rout << "=== Schema ===" << std::endl;
     std::shared_ptr<arrow::Schema> info_schema;
@@ -353,7 +358,7 @@ arrow::Status TestCustomGrpcImpl() {
   StartRecipe("CustomGrpcImpl::StartServer");
   arrow::flight::Location server_location;
   ARROW_RETURN_NOT_OK(
-      arrow::flight::Location::ForGrpcTcp("0.0.0.0", 5000, &server_location));
+      arrow::flight::Location::ForGrpcTcp("0.0.0.0", 5001, &server_location));
 
   arrow::flight::FlightServerOptions options(server_location);
   auto server = std::unique_ptr<arrow::flight::FlightServerBase>(
@@ -374,7 +379,7 @@ arrow::Status TestCustomGrpcImpl() {
 
   StartRecipe("CustomGrpcImpl::CreateClient");
   auto client_channel =
-      grpc::CreateChannel("0.0.0.0:5000", grpc::InsecureChannelCredentials());
+      grpc::CreateChannel("0.0.0.0:5001", grpc::InsecureChannelCredentials());
 
   auto stub = HelloWorldService::NewStub(client_channel);