refactor(silo): apply clangd suggestions

Author: taepper

.clang-tidy

@@ -28,9 +28,9 @@ Checks: >-
 # positives for overridden 'T& operator=(T&)'.
 CheckOptions:
   - key: readability-identifier-length.IgnoredVariableNames
-    value: \_
-  - key: readability-identifier-length.IgnoredVariableNames
-    value: to
+    value: '_|to'
+  - key: readability-identifier-length.IgnoredParameterNames
+    value: '_|to'
   - key: readability-identifier-naming.NamespaceCase
     value: lower_case
   - key: readability-identifier-naming.ClassCase

src/main.test.cpp

@@ -10,6 +10,8 @@
 #include "silo/common/log.h"
 #include "silo/common/panic.h"
 
+namespace {
+
 int changeCwdToTestFolder() {
    // Look for the test data directory (`testBaseData`) in the current directory and up to
    // <search_depth> directories above the current directory. If found, change the current working
@@ -29,6 +31,8 @@ int changeCwdToTestFolder() {
    return 1;
 }
 
+}  // namespace
+
 int main(int argc, char* argv[]) {
    if (auto exit = changeCwdToTestFolder()) {
       return exit;

src/silo/database.cpp

@@ -23,7 +23,7 @@
 namespace silo {
 
 Database::Database(schema::DatabaseSchema database_schema)
-    : schema(database_schema),
+    : schema(std::move(database_schema)),
       table(std::make_shared<storage::Table>(schema.getDefaultTableSchema())) {}
 
 DatabaseInfo Database::getDatabaseInfo() const {

src/silo/database.h

@@ -1,8 +1,6 @@
 #pragma once
 
-#include <cstddef>
 #include <filesystem>
-#include <string>
 
 #include "silo/common/data_version.h"
 #include "silo/common/silo_directory.h"
@@ -23,7 +21,7 @@ class Database {
    DataVersion data_version_ = DataVersion::mineDataVersion();
 
   public:
-   Database(silo::schema::DatabaseSchema database_schema);
+   explicit Database(silo::schema::DatabaseSchema database_schema);
 
    virtual ~Database() = default;
 
@@ -33,7 +31,7 @@ class Database {
 
    [[nodiscard]] virtual DatabaseInfo getDatabaseInfo() const;
 
-   virtual DataVersion::Timestamp getDataVersionTimestamp() const;
+   [[nodiscard]] virtual DataVersion::Timestamp getDataVersionTimestamp() const;
 };
 
 }  // namespace silo

src/silo/database.test.cpp

@@ -2,16 +2,13 @@
 
 #include <filesystem>
 #include <fstream>
-#include <istream>
 #include <map>
 
 #include <gtest/gtest.h>
 
 #include "config/source/yaml_file.h"
-#include "silo/append/append.h"
 #include "silo/append/database_inserter.h"
 #include "silo/append/ndjson_line_reader.h"
-#include "silo/common/nucleotide_symbols.h"
 #include "silo/common/phylo_tree.h"
 #include "silo/config/preprocessing_config.h"
 #include "silo/database_info.h"
@@ -38,7 +35,7 @@ std::shared_ptr<silo::Database> buildTestDatabase() {
       );
 
    std::map<std::filesystem::path, silo::common::LineageTreeAndIdMap> lineage_trees;
-   for (auto filename : config.initialization_files.getLineageDefinitionFilenames()) {
+   for (const auto& filename : config.initialization_files.getLineageDefinitionFilenames()) {
       lineage_trees[filename] =
          silo::common::LineageTreeAndIdMap::fromLineageDefinitionFilePath(filename);
    }
@@ -52,10 +49,10 @@ std::shared_ptr<silo::Database> buildTestDatabase() {
    auto database = std::make_shared<silo::Database>(
       silo::Database{silo::initialize::Initializer::createSchemaFromConfigFiles(
          std::move(database_config),
-         std::move(reference_genomes),
-         std::move(lineage_trees),
-         std::move(phylo_tree_file),
-         /*without_unaligned_columns=*/false
+         reference_genomes,
+         lineage_trees,
+         phylo_tree_file,
+         /*without_unaligned_sequences=*/false
       )}
    );
    std::ifstream input(input_directory / "input.ndjson");

src/silo/database_info.h

@@ -1,15 +1,8 @@
 #pragma once
 
-#include <cinttypes>
-#include <map>
-#include <vector>
-
 #include <fmt/format.h>
 #include <nlohmann/json.hpp>
 
-#include "silo/common/format_number.h"
-#include "silo/common/nucleotide_symbols.h"
-
 namespace silo {
 
 struct DatabaseInfo {
@@ -28,7 +21,7 @@ void to_json(nlohmann::json& json, const silo::DatabaseInfo& databaseInfo);
 template <>
 class [[maybe_unused]] fmt::formatter<silo::DatabaseInfo> {
   public:
-   constexpr auto parse(format_parse_context& ctx) { return ctx.begin(); }
+   static constexpr auto parse(format_parse_context& ctx) { return ctx.begin(); }
    [[maybe_unused]] static auto format(silo::DatabaseInfo database_info, format_context& ctx)
       -> decltype(ctx.out()) {
       return fmt::format_to(ctx.out(), "{}", nlohmann::json{database_info}.dump());

src/silo/query_engine/actions/action.cpp

@@ -3,9 +3,7 @@
 #include <algorithm>
 #include <cctype>
 #include <chrono>
-#include <map>
 #include <memory>
-#include <random>
 #include <utility>
 
 #include <arrow/acero/exec_plan.h>
@@ -28,7 +26,6 @@
 #include "silo/query_engine/bad_request.h"
 #include "silo/query_engine/copy_on_write_bitmap.h"
 #include "silo/query_engine/exec_node/arrow_util.h"
-#include "silo/query_engine/exec_node/ndjson_sink.h"
 #include "silo/query_engine/exec_node/throttled_batch_reslicer.h"
 #include "silo/query_engine/exec_node/zstd_decompress_expression.h"
 #include "silo/storage/column/column_type_visitor.h"
@@ -57,7 +54,7 @@ std::optional<arrow::Ordering> Action::getOrdering() const {
    using arrow::compute::SortOrder;
 
    std::vector<arrow::compute::SortKey> sort_keys;
-   for (auto order_by_field : order_by_fields) {
+   for (const auto& order_by_field : order_by_fields) {
       auto sort_order = order_by_field.ascending ? SortOrder::Ascending : SortOrder::Descending;
       sort_keys.emplace_back(order_by_field.name, sort_order);
    }
@@ -224,7 +221,8 @@ QueryPlan Action::toQueryPlan(
    std::string_view request_id
 ) {
    validateOrderByFields(table->schema);
-   auto query_plan = toQueryPlanImpl(table, partition_filters, query_options, request_id);
+   auto query_plan =
+      toQueryPlanImpl(std::move(table), std::move(partition_filters), query_options, request_id);
    if (!query_plan.status().ok()) {
       SILO_PANIC("Arrow error: {}", query_plan.status().ToString());
    };
@@ -235,7 +233,7 @@ arrow::Result<arrow::acero::ExecNode*> Action::addSortNode(
    arrow::acero::ExecPlan* arrow_plan,
    arrow::acero::ExecNode* node,
    const std::vector<schema::ColumnIdentifier>& output_fields,
-   const arrow::Ordering ordering,
+   const arrow::Ordering& ordering,
    std::optional<size_t> /*num_rows_to_produce*/
 ) {
    arrow::AsyncGenerator<std::optional<arrow::ExecBatch>> generator;
@@ -277,14 +275,14 @@ arrow::Result<arrow::acero::ExecNode*> Action::addSortNode(
 
 namespace {
 
-uint64_t hash64(uint64_t x, uint64_t seed) {
-   x ^= seed;
-   x ^= x >> 33;
-   x *= 0xff51afd7ed558ccdULL;
-   x ^= x >> 33;
-   x *= 0xc4ceb9fe1a85ec53ULL;
-   x ^= x >> 33;
-   return x;
+uint64_t hash64(uint64_t value, uint64_t seed) {
+   value ^= seed;
+   value ^= value >> 33;
+   value *= 0xff51afd7ed558ccdULL;
+   value ^= value >> 33;
+   value *= 0xc4ceb9fe1a85ec53ULL;
+   value ^= value >> 33;
+   return value;
 }
 
 arrow::Result<arrow::acero::ExecNode*> removeRandomizeColumn(
@@ -341,7 +339,7 @@ arrow::Result<arrow::acero::ExecNode*> Action::addRandomizeColumn(
                return std::nullopt;
             }
 
-            auto input_batch = maybe_input_batch.value();
+            const auto& input_batch = maybe_input_batch.value();
             SILO_ASSERT(!input_batch.values.empty());
             auto rows_in_batch = input_batch.values.at(0).length();
             SILO_ASSERT_NE(rows_in_batch, arrow::Datum::kUnknownLength);
@@ -406,8 +404,8 @@ class ColumnToReferenceSequenceVisitor {
   public:
    template <Column ColumnType>
    std::optional<std::string> operator()(
-      const TableSchema& table_schema,
-      const ColumnIdentifier& column_identifier
+      const TableSchema& /*table_schema*/,
+      const ColumnIdentifier& /*column_identifier*/
    ) {
       return std::nullopt;
    }
@@ -419,7 +417,7 @@ std::optional<std::string> ColumnToReferenceSequenceVisitor::operator(
    const TableSchema& table_schema,
    const ColumnIdentifier& column_identifier
 ) {
-   auto metadata =
+   auto* metadata =
       table_schema.getColumnMetadata<SequenceColumnPartition<Nucleotide>>(column_identifier.name)
          .value();
    std::string reference;
@@ -435,7 +433,7 @@ std::optional<std::string> ColumnToReferenceSequenceVisitor::operator(
    const TableSchema& table_schema,
    const ColumnIdentifier& column_identifier
 ) {
-   auto metadata =
+   auto* metadata =
       table_schema.getColumnMetadata<SequenceColumnPartition<AminoAcid>>(column_identifier.name)
          .value();
    std::string reference;
@@ -451,7 +449,7 @@ std::optional<std::string> ColumnToReferenceSequenceVisitor::operator(
    const TableSchema& table_schema,
    const ColumnIdentifier& column_identifier
 ) {
-   auto metadata =
+   auto* metadata =
       table_schema.getColumnMetadata<ZstdCompressedStringColumnPartition>(column_identifier.name)
          .value();
    return metadata->dictionary_string;
@@ -465,18 +463,17 @@ arrow::Result<arrow::acero::ExecNode*> Action::addZstdDecompressNode(
    const silo::schema::TableSchema& table_schema
 ) const {
    auto output_fields = getOutputSchema(table_schema);
-   bool needs_decompression =
-      std::any_of(output_fields.begin(), output_fields.end(), [](const auto& column_identifier) {
-         return schema::isSequenceColumn(column_identifier.type);
-      });
+   bool needs_decompression = std::ranges::any_of(output_fields, [](const auto& column_identifier) {
+      return schema::isSequenceColumn(column_identifier.type);
+   });
    if (needs_decompression) {
       size_t sum_of_reference_genome_sizes = 0;
 
       std::vector<arrow::compute::Expression> column_expressions;
       std::vector<std::string> column_names;
-      for (auto column : getOutputSchema(table_schema)) {
+      for (const auto& column : getOutputSchema(table_schema)) {
          if (auto reference = storage::column::visit(column.type, ColumnToReferenceSequenceVisitor{}, table_schema, column)) {
-            column_expressions.push_back(exec_node::ZstdDecompressExpression::Make(
+            column_expressions.push_back(exec_node::ZstdDecompressExpression::make(
                arrow::compute::field_ref(arrow::FieldRef{column.name}), reference.value()
             ));
             sum_of_reference_genome_sizes += reference.value().length();
@@ -511,7 +508,7 @@ arrow::Result<arrow::acero::ExecNode*> Action::addZstdDecompressNode(
          "additional sink node to help backpressure application before zstd decompression"
       );
 
-      SILO_ASSERT_GT(sum_of_reference_genome_sizes, 0u);
+      SILO_ASSERT_GT(sum_of_reference_genome_sizes, 0U);
 
       // We aim for 64 MB batch size to give the plan time to apply backpressure
       auto maximum_batch_size =
@@ -520,7 +517,7 @@ arrow::Result<arrow::acero::ExecNode*> Action::addZstdDecompressNode(
       // Delay delivery of large number of batches to about 100 MB per second
       // A batch targets 64 MB, therefore we allow 1.5 batches per second.
       // Therefore, we should never emit more than one resliced batch per 0.667 seconds
-      constexpr std::chrono::milliseconds target_batch_rate{667};
+      constexpr std::chrono::milliseconds TARGET_BATCH_RATE{667};
 
       ARROW_ASSIGN_OR_RAISE(
          node,
@@ -531,7 +528,7 @@ arrow::Result<arrow::acero::ExecNode*> Action::addZstdDecompressNode(
             arrow::acero::SourceNodeOptions{
                schema_of_sequence_batches,
                exec_node::ThrottledBatchReslicer{
-                  batch_generator, maximum_batch_size, target_batch_rate, backpressure_monitor
+                  batch_generator, maximum_batch_size, TARGET_BATCH_RATE, backpressure_monitor
                }
             }
          )

src/silo/query_engine/actions/action.h

@@ -11,7 +11,6 @@
 
 #include "silo/config/runtime_config.h"
 #include "silo/query_engine/copy_on_write_bitmap.h"
-#include "silo/query_engine/filter/operators/operator.h"
 #include "silo/query_engine/query_plan.h"
 #include "silo/schema/database_schema.h"
 #include "silo/storage/table.h"
@@ -37,7 +36,7 @@ class Action {
    virtual ~Action() = default;
 
    // Returns the type of this action, which is used for logging the performance by Action type
-   virtual std::string_view getType() const = 0;
+   [[nodiscard]] virtual std::string_view getType() const = 0;
 
    QueryPlan toQueryPlan(
       std::shared_ptr<const storage::Table> table,
@@ -53,9 +52,9 @@ class Action {
       std::optional<uint32_t> randomize_seed
    );
 
-   std::optional<arrow::Ordering> getOrdering() const;
+   [[nodiscard]] std::optional<arrow::Ordering> getOrdering() const;
 
-   virtual std::vector<schema::ColumnIdentifier> getOutputSchema(
+   [[nodiscard]] virtual std::vector<schema::ColumnIdentifier> getOutputSchema(
       const silo::schema::TableSchema& table_schema
    ) const = 0;
 
@@ -81,7 +80,7 @@ class Action {
    ) const;
 
   private:
-   virtual arrow::Result<QueryPlan> toQueryPlanImpl(
+   [[nodiscard]] virtual arrow::Result<QueryPlan> toQueryPlanImpl(
       std::shared_ptr<const storage::Table> table,
       std::vector<CopyOnWriteBitmap> partition_filters,
       const config::QueryOptions& query_options,
@@ -92,7 +91,7 @@ class Action {
       arrow::acero::ExecPlan* arrow_plan,
       arrow::acero::ExecNode* node,
       const std::vector<schema::ColumnIdentifier>& output_fields,
-      const arrow::Ordering ordering,
+      const arrow::Ordering& ordering,
       std::optional<size_t> num_rows_to_produce
    );