Bug fix/collect aggregate undefined (#8853)

* allow 3.4 behaviour regarding aggregate collect and undefined variable

* added test to check undefined var

* rm print

* remove duplicate doCopyRow

* wrongly removed due merge

* initialize _allowSourceRowUninitialized always as false as default

* try to fix another case

* added missing parenthesis

* optimized if, also added compiler hint

arangod/Aql/OutputAqlItemRow.cpp

@@ -48,7 +48,8 @@ OutputAqlItemRow::OutputAqlItemRow(
       _doNotCopyInputRow(copyRowBehaviour == CopyRowBehaviour::DoNotCopyInputRows),
       _outputRegisters(std::move(outputRegisters)),
       _registersToKeep(std::move(registersToKeep)),
-      _registersToClear(std::move(registersToClear))
+      _registersToClear(std::move(registersToClear)),
+      _allowSourceRowUninitialized(false)
 #ifdef ARANGODB_ENABLE_MAINTAINER_MODE
       ,
       _setBaseIndexNotUsed(true)

arangod/Aql/OutputAqlItemRow.h

@@ -229,6 +229,10 @@ class OutputAqlItemRow {
 #endif
     _baseIndex = index;
   }
+  // Use this function with caution! We need it only for the SortedCollectExecutor
+  void setAllowSourceRowUninitialized() {
+    _allowSourceRowUninitialized = true;
+  }
 
   // This function can be used to restore the row's invariant.
   // After setting this value numRowsWritten() rather returns
@@ -301,6 +305,8 @@ class OutputAqlItemRow {
 #ifdef ARANGODB_ENABLE_MAINTAINER_MODE
   bool _setBaseIndexNotUsed;
 #endif
+  // need this special bool for allowing an empty AqlValue inside the SortedCollectExecutor
+  bool _allowSourceRowUninitialized;
 
  private:
   size_t nextUnwrittenIndex() const noexcept { return numRowsWritten(); }
@@ -334,10 +340,15 @@ void OutputAqlItemRow::doCopyRow(InputAqlItemRow const& sourceRow, bool ignoreMi
 
   if (mustClone) {
     for (auto itemId : registersToKeep()) {
+#ifdef ARANGODB_ENABLE_MAINTAINER_MODE
+      if (!_allowSourceRowUninitialized) {
         TRI_ASSERT(sourceRow.isInitialized());
+      }
+#endif
       if (ignoreMissing && itemId >= sourceRow.getNrRegisters()) {
         continue;
       }
+      if (ADB_LIKELY(!_allowSourceRowUninitialized || sourceRow.isInitialized())) {
         auto const& value = sourceRow.getValue(itemId);
         if (!value.isEmpty()) {
           AqlValue clonedValue = value.clone();
@@ -354,6 +365,7 @@ void OutputAqlItemRow::doCopyRow(InputAqlItemRow const& sourceRow, bool ignoreMi
           guard.steal();
         }
       }
+    }
   } else {
     TRI_ASSERT(_baseIndex > 0);
     block().copyValuesFromRow(_baseIndex, registersToKeep(), _lastBaseIndex);

arangod/Aql/SortedCollectExecutor.cpp

@@ -236,9 +236,14 @@ void SortedCollectExecutor::CollectGroup::groupValuesToArray(VPackBuilder& build
   builder.close();
 }
 
-void SortedCollectExecutor::CollectGroup::writeToOutput(OutputAqlItemRow& output) {
+void SortedCollectExecutor::CollectGroup::writeToOutput(OutputAqlItemRow& output, InputAqlItemRow& input) {
   // Thanks to the edge case that we have to emit a row even if we have no
   // input We cannot assert here that the input row is valid ;(
+
+  if (!input.isInitialized()) {
+    output.setAllowSourceRowUninitialized();
+  }
+
   size_t i = 0;
   for (auto& it : infos.getGroupRegisters()) {
     AqlValue val = this->groupValues[i];
@@ -289,7 +294,7 @@ std::pair<ExecutionState, NoStats> SortedCollectExecutor::produceRows(OutputAqlI
   while (true) {
     if (_fetcherDone) {
       if (_currentGroup.isValid()) {
-        _currentGroup.writeToOutput(output);
+        _currentGroup.writeToOutput(output, input);
         InputAqlItemRow input{CreateInvalidInputRowHint{}};
         _currentGroup.reset(input);
         TRI_ASSERT(!_currentGroup.isValid());
@@ -321,7 +326,7 @@ std::pair<ExecutionState, NoStats> SortedCollectExecutor::produceRows(OutputAqlI
 
       if (state == ExecutionState::DONE) {
         TRI_ASSERT(!output.produced());
-        _currentGroup.writeToOutput(output);
+        _currentGroup.writeToOutput(output, input);
         // Invalidate group
         input = InputAqlItemRow{CreateInvalidInputRowHint{}};
         _currentGroup.reset(input);
@@ -331,7 +336,7 @@ std::pair<ExecutionState, NoStats> SortedCollectExecutor::produceRows(OutputAqlI
       if (_currentGroup.isValid()) {
         // Write the current group.
         // Start a new group from input
-        _currentGroup.writeToOutput(output);
+        _currentGroup.writeToOutput(output, input);
         TRI_ASSERT(output.produced());
         _currentGroup.reset(input);  // reset and recreate new group
         if (input.isInitialized()) {
@@ -344,7 +349,7 @@ std::pair<ExecutionState, NoStats> SortedCollectExecutor::produceRows(OutputAqlI
           if (_infos.getGroupRegisters().empty()) {
             // we got exactly 0 rows as input.
             // by definition we need to emit one collect row
-            _currentGroup.writeToOutput(output);
+            _currentGroup.writeToOutput(output, input);
             TRI_ASSERT(output.produced());
           }
           TRI_ASSERT(state == ExecutionState::DONE);

arangod/Aql/SortedCollectExecutor.h

@@ -157,7 +157,7 @@ class SortedCollectExecutor {
     void addLine(InputAqlItemRow& input);
     bool isSameGroup(InputAqlItemRow& input);
     void groupValuesToArray(VPackBuilder& builder);
-    void writeToOutput(OutputAqlItemRow& output);
+    void writeToOutput(OutputAqlItemRow& output, InputAqlItemRow& input);
   };
 
  public:

tests/js/server/aql/aql-optimizer-collect-count.js

@@ -379,6 +379,20 @@ function optimizerCountTestSuite () {
         var plan = AQL_EXPLAIN(query).plan;
         assertNotEqual(-1, plan.rules.indexOf("collect-in-cluster"));
       }
+    },
+
+    testCollectAggregateUndefined: function () {
+      var randomDocumentID = db["UnitTestsCollection"].any()._id;
+      var query = 'LET start = DOCUMENT("' + randomDocumentID + '")._key for i in [] collect aggregate count = count(i) return {count, start}';
+      var bindParams = {};
+      var options = {optimizer: {rules: ['-remove-unnecessary-calculations','-remove-unnecessary-calculations-2']}};
+
+      var results = db._query(query, bindParams, options).toArray();
+      // expectation is that we exactly get one result
+      // count will be 0, start will be undefined
+      assertEqual(1, results.length);
+      assertEqual(0, results[0].count);
+      assertEqual(undefined, results[0].start);
     }
 
   };