Refactor doOutput

arangod/Aql/ModificationExecutor.cpp

@@ -50,17 +50,23 @@ using namespace arangodb::basics;
 namespace arangodb {
 namespace aql {
 
-std::ostream& operator<<(std::ostream& ostream, ModifierIteratorMode mode) {
+ModifierOutput::ModifierOutput(InputAqlItemRow const inputRow, bool const error)
-  switch (mode) {
+    : _inputRow(inputRow), _error(error), _oldValue(nullptr), _newValue(nullptr) {}
-    case ModifierIteratorMode::Full:
+
-      ostream << "Full";
+ModifierOutput::ModifierOutput(InputAqlItemRow const inputRow, bool const error,
-      break;
+                               std::unique_ptr<AqlValue>&& oldValue,
-    case ModifierIteratorMode::OperationsOnly:
+                               std::unique_ptr<AqlValue>&& newValue)
-      ostream << "OperationsOnly";
+    : _inputRow(inputRow),
-      break;
+      _error(error),
-  }
+      _oldValue(std::move(oldValue)),
-  return ostream;
+      _newValue(std::move(newValue)) {}
-}
+
+InputAqlItemRow ModifierOutput::getInputRow() const { return _inputRow; }
+bool ModifierOutput::isError() const { return _error; }
+bool ModifierOutput::hasOldValue() const { return _oldValue != nullptr; }
+AqlValue&& ModifierOutput::getOldValue() const { return std::move(*_oldValue); }
+bool ModifierOutput::hasNewValue() const { return _newValue != nullptr; }
+AqlValue&& ModifierOutput::getNewValue() const { return std::move(*_newValue); }
 
 template <typename FetcherType, typename ModifierType>
 ModificationExecutor<FetcherType, ModifierType>::ModificationExecutor(Fetcher& fetcher,
@@ -115,105 +121,32 @@ ModificationExecutor<FetcherType, ModifierType>::doCollect(size_t const maxOutpu
 template <typename FetcherType, typename ModifierType>
 void ModificationExecutor<FetcherType, ModifierType>::doOutput(OutputAqlItemRow& output,
                                                                Stats& stats) {
-  // If we have made no modifications or are silent,
+  _modifier.setupIterator();
-  // we can just copy rows; this is an optimisation for silent
-  // queries
-  // if (_modifier.nrOfDocuments() == 0 || _infos._options.silent) {
-  //   ModOperationType modOp;
-  //   InputAqlItemRow row{CreateInvalidInputRowHint{}};
-
-  //   _modifier.setupIterator(ModifierIteratorMode::OperationsOnly);
-  //   while (!_modifier.isFinishedIterator()) {
-  //     std::tie(modOp, row, std::ignore) = _modifier.getOutput();
-
-  //     output.copyRow(row);
-
-  //     if (_infos._doCount) {
-  //       switch (modOp) {
-  //         case ModOperationType::APPLY_RETURN: {
-  //           stats.incrWritesExecuted();
-  //           break;
-  //         }
-  //         case ModOperationType::IGNORE_RETURN: {
-  //           stats.incrWritesIgnored();
-  //           break;
-  //         }
-  //         case ModOperationType::IGNORE_SKIP: {
-  //           stats.incrWritesIgnored();
-  //           break;
-  //         }
-  //         default: {
-  //           TRI_ASSERT(false);
-  //         }
-  //       }
-  //     }
-  //     _modifier.advanceIterator();
-  //     output.advanceRow();
-  //   }
-  // } else {
-  ModOperationType modOp;
-  InputAqlItemRow row{CreateInvalidInputRowHint{}};
-  VPackSlice elm;
-
-  _modifier.setupIterator(ModifierIteratorMode::Full);
   while (!_modifier.isFinishedIterator()) {
-    std::tie(modOp, row, elm) = _modifier.getOutput();
+    ModifierOutput modifierOutput{_modifier.getOutput()};
 
-    bool error = VelocyPackHelper::getBooleanValue(elm, StaticStrings::Error, false);
+    if (!modifierOutput.isError()) {
-    if (!error) {
+      if (_infos._options.returnOld) {
-      switch (modOp) {
+        output.cloneValueInto(_infos._outputOldRegisterId, modifierOutput.getInputRow(),
-        case ModOperationType::APPLY_RETURN: {
+                              modifierOutput.getOldValue());
-          if (_infos._options.returnNew) {
+      }
-            AqlValue value(elm.get(StaticStrings::New));
+      if (_infos._options.returnNew) {
-            AqlValueGuard guard(value, true);
+        output.cloneValueInto(_infos._outputNewRegisterId, modifierOutput.getInputRow(),
-            output.moveValueInto(_infos._outputNewRegisterId, row, guard);
+                              modifierOutput.getNewValue());
-          }
+      }
-          if (_infos._options.returnOld) {
+      if (!_infos._options.returnOld && !_infos._options.returnNew) {
-            AqlValue value(elm.get(StaticStrings::Old));
+        output.copyRow(modifierOutput.getInputRow());
-            AqlValueGuard guard(value, true);
-            output.moveValueInto(_infos._outputOldRegisterId, row, guard);
-          }
-          if (_infos._doCount) {
-            stats.incrWritesExecuted();
-          }
-          break;
-        }
-        case ModOperationType::IGNORE_RETURN: {
-          output.copyRow(row);
-          if (_infos._doCount) {
-            stats.incrWritesIgnored();
-          }
-          break;
-        }
-        case ModOperationType::IGNORE_SKIP: {
-          output.copyRow(row);
-          if (_infos._doCount) {
-            stats.incrWritesIgnored();
-          }
-          break;
-        }
-        case ModOperationType::APPLY_UPDATE:
-        case ModOperationType::APPLY_INSERT: {
-          // These values should not appear here anymore
-          // As we handle them in the UPSERT modifier and translate them
-          // into APPLY_RETURN
-          TRI_ASSERT(false);
-        }
-        default: {
-          TRI_ASSERT(false);
-          break;
-        }
       }
       // only advance row if we produced something
       output.advanceRow();
-    } else {
-      if (_infos._doCount) {
-        stats.incrWritesIgnored();
-      }
     }
     _modifier.advanceIterator();
   }
-  // }
+
+  if (_infos._doCount) {
+    stats.addWritesExecuted(_modifier.nrOfWritesExecuted());
+    stats.addWritesIgnored(_modifier.nrOfWritesIgnored());
+  }
 }
 
 template <typename FetcherType, typename ModifierType>
@@ -242,6 +175,13 @@ ModificationExecutor<FetcherType, ModifierType>::produceRows(OutputAqlItemRow& o
 
   _modifier.transact();
 
+  // If the query is silent, there is no way to relate
+  // the results slice contents and the submitted documents
+  // If the query is *not* silent, we should get one result
+  // for every document.
+  // Yes. Really.
+  TRI_ASSERT(_infos._options.silent || _modifier.nrOfDocuments() == _modifier.nrOfResults());
+
   doOutput(output, stats);
 
   return {_lastState, std::move(stats)};

arangod/Aql/ModificationExecutor.h

@@ -99,14 +99,35 @@ enum class ModOperationType : uint8_t {
 // The first component has to be the operation type, the second an
 // InputAqlItemRow, and the third is a VPackSlice containing the result of the
 // transaction for this row.
-using ModifierOutput = std::tuple<ModOperationType, InputAqlItemRow, VPackSlice>;
+// using ModifierOutput = std::tuple<ModOperationType, InputAqlItemRow, VPackSlice>;
 
-// We have to switch between "Full" and "OperationsOnly" iterator mode because
+class ModifierOutput {
-// if the transaction is silent we do not actually have a Velocypack to iterate
+ public:
-// over This can go away once iterating over results of the operation is done
+  ModifierOutput() = delete;
-// with a bespoke iterator class
+  ModifierOutput(InputAqlItemRow const inputRow, bool const error);
-enum class ModifierIteratorMode { OperationsOnly, Full };
+  ModifierOutput(InputAqlItemRow const inputRow, bool const error,
-std::ostream& operator<<(std::ostream& ostream, ModifierIteratorMode mode);
+                 std::unique_ptr<AqlValue>&& oldValue, std::unique_ptr<AqlValue>&& newValue);
+
+  ModifierOutput(ModifierOutput&& o);
+  ModifierOutput& operator=(ModifierOutput&& o);
+
+  InputAqlItemRow getInputRow() const;
+  bool isError() const;
+  bool hasOldValue() const;
+  AqlValue&& getOldValue() const;
+  bool hasNewValue() const;
+  AqlValue&& getNewValue() const;
+
+ private:
+  // No copying or copy assignment allowed of this class or any derived class
+  ModifierOutput(ModifierOutput const&);
+  ModifierOutput& operator=(ModifierOutput const&);
+
+  InputAqlItemRow const _inputRow;
+  bool const _error;
+  std::unique_ptr<AqlValue> _oldValue;
+  std::unique_ptr<AqlValue> _newValue;
+};
 
 template <typename FetcherType, typename ModifierType>
 class ModificationExecutor {

arangod/Aql/ModificationExecutorHelpers.cpp

@@ -35,6 +35,8 @@
 
 #include <string>
 
+#include "Logger/LogMacros.h"
+
 using namespace arangodb;
 using namespace arangodb::aql;
 using namespace arangodb::basics;
@@ -107,6 +109,11 @@ void ModificationExecutorHelpers::throwOperationResultException(
     ModificationExecutorInfos& infos, OperationResult const& result) {
   auto const& errorCounter = result.countErrorCodes;
 
+  // LOG_DEVEL << "ok, here's the shit";
+  // if (result.hasSlice()) {
+  //   LOG_DEVEL << result.slice().toJson();
+  // }
+
   // Early escape if we are ignoring errors.
   if (infos._ignoreErrors == true || errorCounter.empty()) {
     return;

arangod/Aql/SimpleModifier.cpp

@@ -29,6 +29,7 @@
 #include "Aql/ModificationExecutorHelpers.h"
 #include "Aql/OutputAqlItemRow.h"
 #include "Basics/Common.h"
+#include "Basics/VelocyPackHelper.h"
 #include "VocBase/LogicalCollection.h"
 
 #include <velocypack/Collection.h>
@@ -41,6 +42,7 @@ class CollectionNameResolver;
 using namespace arangodb;
 using namespace arangodb::aql;
 using namespace arangodb::aql::ModificationExecutorHelpers;
+using namespace arangodb::basics;
 
 template <class ModifierCompletion, typename Enable>
 SimpleModifier<ModifierCompletion, Enable>::SimpleModifier(ModificationExecutorInfos& infos)
@@ -86,14 +88,32 @@ size_t SimpleModifier<ModifierCompletion, Enable>::nrOfDocuments() const {
 }
 
 template <typename ModifierCompletion, typename Enable>
-Result SimpleModifier<ModifierCompletion, Enable>::setupIterator(ModifierIteratorMode const mode) {
+size_t SimpleModifier<ModifierCompletion, Enable>::nrOfResults() const {
-  _iteratorMode = mode;
+  if (_results.hasSlice() && _results.slice().isArray()) {
-  _operationsIterator = _operations.begin();
+    return _results.slice().length();
-  if (mode == ModifierIteratorMode::Full) {
+  }
-    TRI_ASSERT(_results.slice().isArray());
+  return 0;
-    _resultsIterator = VPackArrayIterator{_results.slice()};
+}
+
+template <typename ModifierCompletion, typename Enable>
+size_t SimpleModifier<ModifierCompletion, Enable>::nrOfWritesExecuted() const {
+  return 0;
+}
+
+template <typename ModifierCompletion, typename Enable>
+size_t SimpleModifier<ModifierCompletion, Enable>::nrOfWritesIgnored() const {
+  return 0;
+}
+
+template <typename ModifierCompletion, typename Enable>
+void SimpleModifier<ModifierCompletion, Enable>::setupIterator() {
+  _operationsIterator = _operations.begin();
+  if (resultAvailable()) {
+    TRI_ASSERT(_results.hasSlice() && _results.slice().isArray());
+    _resultsIterator = VPackArrayIterator{_results.slice()};
+  } else {
+    _resultsIterator = VPackArrayIterator{VPackSlice::emptyArraySlice()};
   }
-  return Result{};
 }
 
 template <typename ModifierCompletion, typename Enable>
@@ -121,22 +141,26 @@ size_t SimpleModifier<ModifierCompletion, Enable>::getBatchSize() const {
   return _batchSize;
 }
 
+template <typename ModifierCompletion, typename Enable>
+
+bool SimpleModifier<ModifierCompletion, Enable>::resultAvailable() const {
+  return (nrOfDocuments() > 0 && !_infos._options.silent);
+}
+
 template <typename ModifierCompletion, typename Enable>
 ModifierOutput SimpleModifier<ModifierCompletion, Enable>::getOutput() {
-  switch (_iteratorMode) {
+  if (_operationsIterator->first == ModOperationType::APPLY_RETURN && resultAvailable()) {
-    case ModifierIteratorMode::Full: {
+    VPackSlice elm = *_resultsIterator;
-      return ModifierOutput{_operationsIterator->first,
+
-                            _operationsIterator->second, *_resultsIterator};
+    bool error = VelocyPackHelper::getBooleanValue(elm, StaticStrings::Error, false);
-    }
+    return ModifierOutput{_operationsIterator->second, error,
-    case ModifierIteratorMode::OperationsOnly: {
+                          std::make_unique<AqlValue>(elm.get(StaticStrings::Old)),
-      return ModifierOutput{_operationsIterator->first,
+                          std::make_unique<AqlValue>(elm.get(StaticStrings::New))};
-                            _operationsIterator->second, VPackSlice::noneSlice()};
+  } else {
-    }
+    return ModifierOutput{_operationsIterator->second, false};
   }
   TRI_ASSERT(false);
-  return ModifierOutput{ModOperationType::IGNORE_SKIP,
+  return ModifierOutput{InputAqlItemRow{CreateInvalidInputRowHint{}}, true};
-                        InputAqlItemRow{CreateInvalidInputRowHint()},
-                        VPackSlice::noneSlice()};
 }
 
 template class ::arangodb::aql::SimpleModifier<InsertModifierCompletion>;

arangod/Aql/SimpleModifier.h

@@ -94,9 +94,14 @@ class SimpleModifier {
   size_t nrOfOperations() const;
   // The number of documents in the accumulator
   size_t nrOfDocuments() const;
+  // The number of entries in the results slice
+  size_t nrOfResults() const;
+
+  size_t nrOfWritesExecuted() const;
+  size_t nrOfWritesIgnored() const;
 
   // TODO: Make this a real iterator
-  Result setupIterator(ModifierIteratorMode const mode);
+  void setupIterator();
   bool isFinishedIterator();
   ModifierOutput getOutput();
   void advanceIterator();
@@ -105,6 +110,8 @@ class SimpleModifier {
   size_t getBatchSize() const;
 
  private:
+  bool resultAvailable() const;
+
   ModificationExecutorInfos& _infos;
   ModifierCompletion _completion;
 
@@ -115,7 +122,6 @@ class SimpleModifier {
 
   std::vector<ModOp>::const_iterator _operationsIterator;
   VPackArrayIterator _resultsIterator;
-  ModifierIteratorMode _iteratorMode;
 
   size_t const _batchSize;
 };

arangod/Aql/UpsertModifier.cpp

@@ -29,6 +29,7 @@
 #include "Aql/ModificationExecutorHelpers.h"
 #include "Aql/OutputAqlItemRow.h"
 #include "Basics/Common.h"
+#include "Basics/VelocyPackHelper.h"
 #include "Transaction/Methods.h"
 #include "VocBase/LogicalCollection.h"
 
@@ -42,6 +43,7 @@ class CollectionNameResolver;
 using namespace arangodb;
 using namespace arangodb::aql;
 using namespace arangodb::aql::ModificationExecutorHelpers;
+using namespace arangodb::basics;
 
 UpsertModifier::UpsertModifier(ModificationExecutorInfos& infos)
     : _infos(infos),
@@ -128,6 +130,10 @@ ModOperationType UpsertModifier::insertCase(ModificationExecutorAccumulator& acc
   }
 }
 
+bool UpsertModifier::resultAvailable() const {
+  return (nrOfDocuments() > 0 && !_infos._options.silent);
+}
+
 Result UpsertModifier::accumulate(InputAqlItemRow& row) {
   RegisterId const inDocReg = _infos._input1RegisterId;
   RegisterId const insertReg = _infos._input2RegisterId;
@@ -180,28 +186,39 @@ size_t UpsertModifier::nrOfDocuments() const {
 
 size_t UpsertModifier::nrOfOperations() const { return _operations.size(); }
 
-Result UpsertModifier::setupIterator(ModifierIteratorMode const mode) {
+size_t UpsertModifier::nrOfResults() const {
-  _iteratorMode = mode;
+  size_t n{0};
-  _operationsIterator = _operations.begin();
-  if (mode == ModifierIteratorMode::Full) {
-    if (!_insertResults.hasSlice() || _insertResults.slice().isNone()) {
-      _insertResultsIterator = VPackArrayIterator(VPackSlice::emptyArraySlice());
-    } else if (_insertResults.slice().isArray()) {
-      _insertResultsIterator = VPackArrayIterator(_insertResults.slice());
-    } else {
-      TRI_ASSERT(false);
-    }
 
-    if (!_updateResults.hasSlice() || _updateResults.slice().isNone()) {
+  if (_insertResults.hasSlice() && _insertResults.slice().isArray()) {
-      _updateResultsIterator = VPackArrayIterator(VPackSlice::emptyArraySlice());
+    n += _insertResults.slice().length();
-    } else if (_updateResults.slice().isArray()) {
+  }
-      _updateResultsIterator = VPackArrayIterator(_updateResults.slice());
+  if (_updateResults.hasSlice() && _updateResults.slice().isArray()) {
-    } else {
+    n += _updateResults.slice().length();
-      TRI_ASSERT(false);
+  }
-    }
+  return n;
+}
+
+size_t UpsertModifier::nrOfWritesExecuted() const { return 0; }
+size_t UpsertModifier::nrOfWritesIgnored() const { return 0; }
+
+void UpsertModifier::setupIterator() {
+  _operationsIterator = _operations.begin();
+
+  if (!_insertResults.hasSlice() || _insertResults.slice().isNone()) {
+    _insertResultsIterator = VPackArrayIterator(VPackSlice::emptyArraySlice());
+  } else if (_insertResults.slice().isArray()) {
+    _insertResultsIterator = VPackArrayIterator(_insertResults.slice());
+  } else {
+    TRI_ASSERT(false);
   }
 
-  return Result{};
+  if (!_updateResults.hasSlice() || _updateResults.slice().isNone()) {
+    _updateResultsIterator = VPackArrayIterator(VPackSlice::emptyArraySlice());
+  } else if (_updateResults.slice().isArray()) {
+    _updateResultsIterator = VPackArrayIterator(_updateResults.slice());
+  } else {
+    TRI_ASSERT(false);
+  }
 }
 
 bool UpsertModifier::isFinishedIterator() {
@@ -209,14 +226,10 @@ bool UpsertModifier::isFinishedIterator() {
 }
 
 void UpsertModifier::advanceIterator() {
-  if (_iteratorMode == ModifierIteratorMode::Full) {
+  if (_operationsIterator->first == ModOperationType::APPLY_UPDATE) {
-    if (_operationsIterator->first == ModOperationType::APPLY_UPDATE) {
+    _updateResultsIterator++;
-      _updateResultsIterator++;
+  } else if (_operationsIterator->first == ModOperationType::APPLY_INSERT) {
-    } else if (_operationsIterator->first == ModOperationType::APPLY_INSERT) {
+    _insertResultsIterator++;
-      _insertResultsIterator++;
-    }
-    // If IGNORE_SKIP or IGNORE_RETURN the transaction results will
-    // not have an entry for this, so do not move any iterator.
   }
   _operationsIterator++;
 }
@@ -225,36 +238,31 @@ void UpsertModifier::advanceIterator() {
 // operation in question was APPLY_UPDATE or APPLY_INSERT to determine which
 // of the results slices (UpdateReplace or Insert) we have to look in and
 // increment.
-UpsertModifier::ModifierOutput UpsertModifier::getOutput() {
+ModifierOutput UpsertModifier::getOutput() {
-  switch (_iteratorMode) {
+  if (resultAvailable()) {
-    case ModifierIteratorMode::Full: {
+    VPackSlice elm;
-      if (_operationsIterator->first == ModOperationType::APPLY_UPDATE) {
+
-        return ModifierOutput{ModOperationType::APPLY_RETURN,
+    switch (_operationsIterator->first) {
-                              _operationsIterator->second, *_updateResultsIterator};
+      case ModOperationType::APPLY_UPDATE:
-      } else if (_operationsIterator->first == ModOperationType::APPLY_INSERT) {
+        elm = *_updateResultsIterator;
-        return ModifierOutput{ModOperationType::APPLY_RETURN,
+        break;
-                              _operationsIterator->second, *_insertResultsIterator};
+      case ModOperationType::APPLY_INSERT:
-      } else {
+        elm = *_insertResultsIterator;
-        return ModifierOutput{_operationsIterator->first,
+        break;
-                              _operationsIterator->second, VPackSlice::noneSlice()};
+      default:
-      }
+        TRI_ASSERT(false);
-    }
-    case ModifierIteratorMode::OperationsOnly: {
-      if (_operationsIterator->first == ModOperationType::APPLY_UPDATE ||
-          _operationsIterator->first == ModOperationType::APPLY_INSERT) {
-        return ModifierOutput{ModOperationType::APPLY_RETURN,
-                              _operationsIterator->second, VPackSlice::noneSlice()};
-      } else {
-        return ModifierOutput{_operationsIterator->first,
-                              _operationsIterator->second, VPackSlice::noneSlice()};
-      }
     }
+    bool error = VelocyPackHelper::getBooleanValue(elm, StaticStrings::Error, false);
+    return ModifierOutput{_operationsIterator->second, error,
+                          std::make_unique<AqlValue>(elm.get(StaticStrings::Old)),
+                          std::make_unique<AqlValue>(elm.get(StaticStrings::New))};
+  } else {
+    return ModifierOutput{_operationsIterator->second, false};
   }
+
   // shut up compiler
   TRI_ASSERT(false);
-  return ModifierOutput{ModOperationType::IGNORE_SKIP,
+  return ModifierOutput{InputAqlItemRow{CreateInvalidInputRowHint{}}, true};
-                        InputAqlItemRow{CreateInvalidInputRowHint()},
-                        VPackSlice::noneSlice()};
 }
 
 size_t UpsertModifier::getBatchSize() const { return _batchSize; }

arangod/Aql/UpsertModifier.h

@@ -37,7 +37,6 @@ struct ModificationExecutorInfos;
 
 class UpsertModifier {
  public:
-  using ModifierOutput = std::tuple<ModOperationType, InputAqlItemRow, VPackSlice>;
   using ModOp = std::pair<ModOperationType, InputAqlItemRow>;
 
  public:
@@ -51,9 +50,12 @@ class UpsertModifier {
 
   size_t nrOfOperations() const;
   size_t nrOfDocuments() const;
+  size_t nrOfResults() const;
+  size_t nrOfWritesExecuted() const;
+  size_t nrOfWritesIgnored() const;
 
   // TODO: Make this a real iterator
-  Result setupIterator(ModifierIteratorMode mode);
+  void setupIterator();
   bool isFinishedIterator();
   ModifierOutput getOutput();
   void advanceIterator();
@@ -61,6 +63,8 @@ class UpsertModifier {
   size_t getBatchSize() const;
 
  private:
+  bool resultAvailable() const;
+
   ModOperationType updateReplaceCase(ModificationExecutorAccumulator& accu,
                                      AqlValue const& inDoc, AqlValue const& updateDoc);
   ModOperationType insertCase(ModificationExecutorAccumulator& accu, AqlValue const& insertDoc);
@@ -78,7 +82,6 @@ class UpsertModifier {
   VPackArrayIterator _updateResultsIterator;
   VPackArrayIterator _insertResultsIterator;
 
-  ModifierIteratorMode _iteratorMode;
   size_t const _batchSize;
 };