remove LogicalCollection::numberDocuments() (arangodb#18401)

leonardoadame · Mar 24, 2023 · 1a036c5 · 1a036c5
1 parent 01faf9b
commit 1a036c5
Show file tree

Hide file tree

Showing 11 changed files with 27 additions and 46 deletions.
diff --git a/arangod/Pregel/GraphStore/GraphLoader.cpp b/arangod/Pregel/GraphStore/GraphLoader.cpp
@@ -26,6 +26,11 @@
 #include <memory>
 #include <cstdint>
 
+#include "ApplicationFeatures/ApplicationServer.h"
+#include "Cluster/ServerState.h"
+#include "Cluster/ClusterFeature.h"
+#include "Cluster/ClusterInfo.h"
+#include "Logger/LogMacros.h"
 #include "Pregel/GraphFormat.h"
 #include "Pregel/Algos/ColorPropagation/ColorPropagationValue.h"
 #include "Pregel/Algos/DMID/DMIDValue.h"
@@ -36,22 +41,15 @@
 #include "Pregel/Algos/SCC/SCCValue.h"
 #include "Pregel/Algos/SLPA/SLPAValue.h"
 #include "Pregel/Algos/WCC/WCCValue.h"
-
 #include "Pregel/Worker/WorkerConfig.h"
-
-#include "Cluster/ServerState.h"
-#include "Cluster/ClusterFeature.h"
-#include "Cluster/ClusterInfo.h"
-#include "Logger/LogMacros.h"
 #include "Scheduler/SchedulerFeature.h"
+#include "StorageEngine/PhysicalCollection.h"
 #include "Transaction/Helpers.h"
 #include "Transaction/Methods.h"
 #include "Transaction/Options.h"
 #include "Transaction/StandaloneContext.h"
 #include "VocBase/LogicalCollection.h"
 
-#include "ApplicationFeatures/ApplicationServer.h"
-
 #define LOG_PREGEL(logId, level)          \
   LOG_TOPIC(logId, level, Logger::PREGEL) \
       << "[job " << config->executionNumber() << "] "
@@ -167,8 +165,7 @@ auto GraphLoader<V, E>::loadVertices(ShardID const& vertexShard,
 
   // tell the formatter the number of docs we are about to load
   LogicalCollection* coll = cursor->collection();
-  uint64_t numVertices =
-      coll->numberDocuments(&trx, transaction::CountType::Normal);
+  uint64_t numVertices = coll->getPhysical()->numberDocuments(&trx);
 
   requestVertexIds(numVertices);
   LOG_PREGEL("7c31f", DEBUG)

diff --git a/arangod/Replication/DatabaseInitialSyncer.cpp b/arangod/Replication/DatabaseInitialSyncer.cpp
@@ -2147,7 +2147,7 @@ Result DatabaseInitialSyncer::fetchCollectionSyncByRevisions(
       uint64_t numberDocumentsAfterSync =
           documentsFound + stats.numDocsInserted - stats.numDocsRemoved;
       uint64_t numberDocumentsDueToCounter =
-          coll->numberDocuments(trx.get(), transaction::CountType::Normal);
+          physical->numberDocuments(trx.get());
 
       setProgress(std::string("number of remaining documents in collection '") +
                   coll->name() +

diff --git a/arangod/RestHandler/RestReplicationHandler.cpp b/arangod/RestHandler/RestReplicationHandler.cpp
@@ -3745,7 +3745,7 @@ ResultT<std::string> RestReplicationHandler::computeCollectionChecksum(
     transaction::Methods trx(ctx);
     TRI_ASSERT(trx.status() == transaction::Status::RUNNING);
 
-    uint64_t num = col->numberDocuments(&trx, transaction::CountType::Normal);
+    uint64_t num = col->getPhysical()->numberDocuments(&trx);
     return ResultT<std::string>::success(std::to_string(num));
   } catch (...) {
     // Query exists, but is in use.

diff --git a/arangod/RocksDBEngine/RocksDBIncrementalSync.cpp b/arangod/RocksDBEngine/RocksDBIncrementalSync.cpp
@@ -1017,7 +1017,7 @@ Result handleSyncKeysRocksDB(DatabaseInitialSyncer& syncer,
           documentsFound + stats.numDocsInserted -
           (stats.numDocsRemoved - numberDocumentsRemovedBeforeStart);
       uint64_t numberDocumentsDueToCounter =
-          col->numberDocuments(trx.get(), transaction::CountType::Normal);
+          physical->numberDocuments(trx.get());
       syncer.setProgress(
           std::string("number of remaining documents in collection '") +
           col->name() + "': " + std::to_string(numberDocumentsAfterSync) +

diff --git a/arangod/RocksDBEngine/RocksDBIterators.cpp b/arangod/RocksDBEngine/RocksDBIterators.cpp
@@ -261,7 +261,7 @@ class RocksDBAnyIndexIterator final : public IndexIterator {
           TRI_ERROR_INTERNAL, "invalid iterator in RocksDBAnyIndexIterator");
     }
 
-    _total = collection->numberDocuments(trx, transaction::CountType::Normal);
+    _total = collection->getPhysical()->numberDocuments(trx);
     reset();  // initial seek
   }
 

diff --git a/arangod/Transaction/Methods.cpp b/arangod/Transaction/Methods.cpp
@@ -2885,7 +2885,7 @@ futures::Future<OperationResult> transaction::Methods::countCoordinatorHelper(
 
 /// @brief count the number of documents in a collection
 OperationResult transaction::Methods::countLocal(
-    std::string const& collectionName, transaction::CountType type,
+    std::string const& collectionName, transaction::CountType /*type*/,
     OperationOptions const& options) {
   DataSourceId cid =
       addCollectionAtRuntime(collectionName, AccessMode::Type::READ);
@@ -2900,7 +2900,7 @@ OperationResult transaction::Methods::countLocal(
 
   TRI_ASSERT(isLocked(collection.get(), AccessMode::Type::READ));
 
-  uint64_t num = collection->numberDocuments(this, type);
+  uint64_t num = collection->getPhysical()->numberDocuments(this);
 
   VPackBuilder resultBuilder;
   resultBuilder.add(VPackValue(num));

diff --git a/arangod/VocBase/LogicalCollection.cpp b/arangod/VocBase/LogicalCollection.cpp
@@ -429,28 +429,6 @@ std::unique_ptr<IndexIterator> LogicalCollection::getAnyIterator(
     transaction::Methods* trx) {
   return _physical->getAnyIterator(trx);
 }
-// @brief Return the number of documents in this collection
-uint64_t LogicalCollection::numberDocuments(transaction::Methods* trx,
-                                            transaction::CountType type) {
-  // detailed results should have been handled in the levels above us
-  TRI_ASSERT(type != transaction::CountType::Detailed);
-
-  uint64_t documents = transaction::CountCache::NotPopulated;
-  if (type == transaction::CountType::ForceCache) {
-    // always return from the cache, regardless what's in it
-    documents = _countCache.get();
-  } else if (type == transaction::CountType::TryCache) {
-    // get data from cache, but only if not expired
-    documents = _countCache.getWithTtl();
-  }
-  if (documents == transaction::CountCache::NotPopulated) {
-    // cache was not populated before or cache value has expired
-    documents = getPhysical()->numberDocuments(trx);
-    _countCache.store(documents);
-  }
-  TRI_ASSERT(documents != transaction::CountCache::NotPopulated);
-  return documents;
-}
 
 bool LogicalCollection::hasClusterWideUniqueRevs() const noexcept {
   return usesRevisionsAsDocumentIds() && isSmartChild();

diff --git a/arangod/VocBase/LogicalCollection.h b/arangod/VocBase/LogicalCollection.h
@@ -159,8 +159,6 @@ class LogicalCollection : public LogicalDataSource {
 
   void executeWhileStatusWriteLocked(std::function<void()> const& callback);
 
-  uint64_t numberDocuments(transaction::Methods*, transaction::CountType type);
-
   // SECTION: Properties
   RevisionId revision(transaction::Methods*) const;
   bool waitForSync() const noexcept { return _waitForSync; }

diff --git a/js/server/modules/@arangodb/testutils/aql-profiler-test-helper.js b/js/server/modules/@arangodb/testutils/aql-profiler-test-helper.js
@@ -491,10 +491,16 @@ function assertStatsMatchGenStats(profile, expectedStats) {
 ////////////////////////////////////////////////////////////////////////////////
 
 function assertNodesItemsAndCalls (expected, actual, details = {}) {
+  const normalize = (type) => { 
+    // SortLimitNode and SortNode are both SortNodes, so treat them as being
+    // identical here for the comparisons
+    return (type === 'SortLimitNode') ? 'SortNode' : type;
+  };
+
   // assert node types first
   assert.assertEqual(
-    expected.map(node => node.type),
-    actual.map(node => node.type),
+    expected.map(node => normalize(node.type)),
+    actual.map(node => normalize(node.type)),
     details
   );
 

diff --git a/tests/Aql/AqlSharedExecutionBlockImplTest.cpp b/tests/Aql/AqlSharedExecutionBlockImplTest.cpp
@@ -47,6 +47,8 @@
 #include "Basics/GlobalResourceMonitor.h"
 #include "Basics/ResourceUsage.h"
 #include "RestServer/TemporaryStorageFeature.h"
+#include "StorageEngine/PhysicalCollection.h"
+#include "VocBase/LogicalCollection.h"
 
 static_assert(GTEST_HAS_TYPED_TEST, "We need typed tests for the following:");
 
@@ -358,7 +360,7 @@ class AqlSharedExecutionBlockImplTest : public ::testing::Test {
     if constexpr (std::is_same_v<ExecutorType, InsertExecutor>) {
       std::shared_ptr<arangodb::LogicalCollection> col =
           server.getSystemDatabase().lookupCollection(collectionName);
-      auto docs = col->numberDocuments(nullptr, transaction::CountType::Normal);
+      auto docs = col->getPhysical()->numberDocuments(nullptr);
       EXPECT_EQ(docs, 3) << "Not all Documents have been properly inserted";
     }
   }
@@ -430,7 +432,7 @@ class AqlSharedExecutionBlockImplTest : public ::testing::Test {
     if constexpr (std::is_same_v<ExecutorType, InsertExecutor>) {
       std::shared_ptr<arangodb::LogicalCollection> col =
           server.getSystemDatabase().lookupCollection(collectionName);
-      auto docs = col->numberDocuments(nullptr, transaction::CountType::Normal);
+      auto docs = col->getPhysical()->numberDocuments(nullptr);
       EXPECT_EQ(docs, 3) << "Not all Documents have been properly inserted";
     }
   }

diff --git a/tests/Aql/SpliceSubqueryOptimizerRuleTest.cpp b/tests/Aql/SpliceSubqueryOptimizerRuleTest.cpp
@@ -36,6 +36,7 @@
 #include "Aql/WalkerWorker.h"
 #include "Logger/LogMacros.h"
 #include "RestServer/QueryRegistryFeature.h"
+#include "StorageEngine/PhysicalCollection.h"
 #include "Transaction/Methods.h"
 #include "Transaction/StandaloneContext.h"
 #include "VocBase/LogicalCollection.h"
@@ -499,8 +500,7 @@ TEST_F(SpliceSubqueryNodeOptimizerRuleTest, splice_subquery_with_upsert) {
   auto ctx = transaction::StandaloneContext::Create(server.getSystemDatabase());
   auto trx = std::make_unique<arangodb::transaction::Methods>(
       ctx, readCollection, noCollections, noCollections, opts);
-  ASSERT_EQ(1, collection->numberDocuments(trx.get(),
-                                           transaction::CountType::Normal));
+  ASSERT_EQ(1, collection->getPhysical()->numberDocuments(trx.get()));
   bool called = false;
   auto result = collection->getPhysical()->read(
       trx.get(), std::string_view{"myKey"},