Fix order-by's scope of columns.

src/common/datatypes/DataSet.h

@@ -216,6 +216,17 @@ struct DataSet {
   bool operator==(const DataSet& rhs) const {
     return colNames == rhs.colNames && rows == rhs.rows;
   }
+
+  void removeColumn(std::vector<size_t>& indices) {
+    for (auto& row : rows) {
+      for (auto& idx : indices) {
+        row.values.erase(row.values.begin() + idx);
+      }
+    }
+    for (auto& idx : indices) {
+      colNames.erase(colNames.begin() + idx);
+    }
+  }
 };
 
 inline std::ostream& operator<<(std::ostream& os, const DataSet& d) {

src/common/datatypes/Value.cpp

@@ -678,6 +678,11 @@ const DataSet& Value::getDataSet() const {
   return *(value_.gVal);
 }
 
+DataSet& Value::getMutableDataSet() {
+  CHECK_EQ(type_, Type::DATASET);
+  return *(value_.gVal);
+}
+
 const DataSet* Value::getDataSetPtr() const {
   CHECK_EQ(type_, Type::DATASET);
   return value_.gVal.get();

src/common/datatypes/Value.h

@@ -309,6 +309,7 @@ struct Value {
   const Set& getSet() const;
   const Set* getSetPtr() const;
   const DataSet& getDataSet() const;
+  DataSet& getMutableDataSet();
   const DataSet* getDataSetPtr() const;
   const Geography& getGeography() const;
   const Geography* getGeographyPtr() const;

src/graph/context/Symbols.h

@@ -49,6 +49,7 @@ struct Variable {
 
   // the count of use the variable
   std::atomic<uint64_t> userCount{0};
+  std::vector<size_t> invisibleColIndicies;
 };
 
 class SymbolTable final {

src/graph/executor/query/SortExecutor.cpp

@@ -44,6 +44,8 @@ folly::Future<Status> SortExecutor::execute() {
 
   auto seqIter = static_cast<SequentialIter *>(iter);
   std::sort(seqIter->begin(), seqIter->end(), comparator);
+  result.valuePtr()->getMutableDataSet().removeColumn(sort->inputVars()[0]->invisibleColIndicies);
+
   return finish(ResultBuilder().value(result.valuePtr()).iter(std::move(result).iter()).build());
 }
 

src/graph/executor/query/TopNExecutor.cpp

@@ -54,13 +54,15 @@ folly::Future<Status> TopNExecutor::execute() {
     maxCount_ = size - offset_;
     heapSize_ = size;
   }
+
   if (heapSize_ == 0) {
     iter->clear();
-    return finish(ResultBuilder().value(result.valuePtr()).iter(std::move(result).iter()).build());
+  } else {
+    executeTopN<SequentialIter>(iter);
+    iter->eraseRange(maxCount_, size);
   }
+  result.valuePtr()->getMutableDataSet().removeColumn(topn->inputVars()[0]->invisibleColIndicies);
 
-  executeTopN<SequentialIter>(iter);
-  iter->eraseRange(maxCount_, size);
   return finish(ResultBuilder().value(result.valuePtr()).iter(std::move(result).iter()).build());
 }
 

src/graph/planner/match/ReturnClausePlanner.cpp

@@ -38,6 +38,13 @@ Status ReturnClausePlanner::buildReturn(ReturnClauseContext* rctx, SubPlan& subP
     NG_RETURN_IF_ERROR(orderPlan);
     auto plan = std::move(orderPlan).value();
     subPlan = SegmentsConnector::addInput(plan, subPlan, true);
+    size_t idx = 0;
+    for (auto& yiledCol : rctx->yield->yieldColumns->columns()) {
+      if (!yiledCol->isVisible()) {
+        subPlan.root->mutableInputVars()[0]->invisibleColIndicies.push_back(idx);
+      }
+      idx++;
+    }
   }
 
   if (rctx->pagination != nullptr &&

src/graph/planner/plan/PlanNode.h

@@ -278,6 +278,10 @@ class PlanNode {
     return inputVars_;
   }
 
+  std::vector<Variable*>& mutableInputVars() {
+    return inputVars_;
+  }
+
   void releaseSymbols();
 
   void updateSymbols();

src/graph/validator/MatchValidator.cpp

@@ -437,7 +437,8 @@ Status MatchValidator::validateReturn(MatchReturn *ret,
     if (!status.ok()) {
       return status;
     }
-    if (columns->empty() && !ret->returnItems()->columns()) {
+    if (ret->returnItems()->allNamedAliases() && columns->empty() &&
+        !ret->returnItems()->columns()) {
       return Status::SemanticError("RETURN * is not allowed when there are no variables in scope");
     }
   }
@@ -462,6 +463,13 @@ Status MatchValidator::validateReturn(MatchReturn *ret,
     }
   }
   DCHECK(!columns->empty());
+  if (ret->orderFactors() != nullptr) {
+    auto orderByCtx = getContext<OrderByClauseContext>();
+    NG_RETURN_IF_ERROR(validateOrderBy(ret, retClauseCtx, columns, *orderByCtx, queryParts));
+    retClauseCtx.order = std::move(orderByCtx);
+  }
+
+  // finished producing columns for further validations
   retClauseCtx.yield->yieldColumns = columns;
 
   NG_RETURN_IF_ERROR(validateAliases(exprs, retClauseCtx.yield->aliasesAvailable));
@@ -473,13 +481,6 @@ Status MatchValidator::validateReturn(MatchReturn *ret,
   NG_RETURN_IF_ERROR(validatePagination(ret->skip(), ret->limit(), *paginationCtx));
   retClauseCtx.pagination = std::move(paginationCtx);
 
-  if (ret->orderFactors() != nullptr) {
-    auto orderByCtx = getContext<OrderByClauseContext>();
-    NG_RETURN_IF_ERROR(
-        validateOrderBy(ret->orderFactors(), retClauseCtx.yield->yieldColumns, *orderByCtx));
-    retClauseCtx.order = std::move(orderByCtx);
-  }
-
   return Status::OK();
 }
 
@@ -830,6 +831,79 @@ Status MatchValidator::validateOrderBy(const OrderFactors *factors,
   return Status::OK();
 }
 
+Status MatchValidator::validateOrderBy(MatchReturn *ret,
+                                       ReturnClauseContext &retClauseCtx,
+                                       YieldColumns *yieldColumns,
+                                       OrderByClauseContext &orderByCtx,
+                                       const std::vector<QueryPart> &queryParts) const {
+  const OrderFactors *factors = ret->orderFactors();
+  bool needsAndOkToExpandScope = !retClauseCtx.yield->hasAgg_ && !ret->isDistinct() &&
+                                 !ret->returnItems()->allNamedAliases() && !queryParts.empty();
+
+  // if the columns required are not already in the projection columns, they may be found
+  // in in the columns before the projection
+  std::vector<std::string> inputColList;
+  inputColList.reserve(yieldColumns->columns().size());
+  for (auto *col : yieldColumns->columns()) {
+    inputColList.emplace_back(col->name());
+  }
+  std::unordered_map<std::string, size_t> inputColIndices;
+  for (auto i = 0u; i < inputColList.size(); i++) {
+    if (!inputColIndices.emplace(inputColList[i], i).second) {
+      return Status::SemanticError("Duplicated columns not allowed: %s", inputColList[i].c_str());
+    }
+  }
+  YieldColumns *allQueryPartsColumns = nullptr;
+  for (auto &factor : factors->factors()) {
+    auto factorExpr = factor->expr();
+    if (ExpressionUtils::isEvaluableExpr(factorExpr, qctx_)) continue;
+    if (factorExpr->kind() != Expression::Kind::kLabel) {
+      return Status::SemanticError("Only column name can be used as sort item");
+    }
+    auto &name = static_cast<const LabelExpression *>(factor->expr())->name();
+    auto iter = inputColIndices.find(name);
+    if (iter == inputColIndices.end()) {
+      // some column is not found
+      if (!needsAndOkToExpandScope) {
+        return Status::SemanticError("Column `%s' not found", name.c_str());
+      } else {
+        // expand the scope to columns before the projection
+        // and see whether the required column could be found
+        bool found = false;
+        if (allQueryPartsColumns == nullptr) {
+          // expand
+          allQueryPartsColumns = retClauseCtx.qctx->objPool()->makeAndAdd<YieldColumns>();
+          auto status = buildColumnsForAllNamedAliases(queryParts, allQueryPartsColumns);
+          if (!status.ok()) {
+            return status;
+          }
+        }
+        for (auto &col : allQueryPartsColumns->columns()) {
+          // check column
+          if (col->name().compare(name) != 0) {
+            continue;
+          } else {
+            col->setVisible(false);
+            yieldColumns->addColumn(col->clone().release());
+            found = true;
+            orderByCtx.indexedOrderFactors.emplace_back(yieldColumns->size() - 1,
+                                                        factor->orderType());
+            break;
+          }
+        }
+        if (!found) {
+          return Status::SemanticError("Column `%s' not found", name.c_str());
+        }
+      }
+    } else {
+      // iter found
+      orderByCtx.indexedOrderFactors.emplace_back(iter->second, factor->orderType());
+    }
+  }
+
+  return Status::OK();
+}
+
 // Validate group by and fill group by context.
 Status MatchValidator::validateGroup(YieldClauseContext &yieldCtx) {
   auto cols = yieldCtx.yieldColumns->columns();

src/graph/validator/MatchValidator.h

@@ -54,6 +54,12 @@ class MatchValidator final : public Validator {
                          const YieldColumns *yieldColumns,
                          OrderByClauseContext &orderByCtx) const;
 
+  Status validateOrderBy(MatchReturn *ret,
+                         ReturnClauseContext &retClauseCtx,
+                         YieldColumns *yieldColumns,
+                         OrderByClauseContext &orderByCtx,
+                         const std::vector<QueryPart> &queryParts) const;
+
   Status validateGroup(YieldClauseContext &yieldCtx);
 
   Status validateYield(YieldClauseContext &yieldCtx);

src/parser/Clauses.h

@@ -254,13 +254,14 @@ class WhenClause : public WhereClause {
 
 class YieldColumn final {
  public:
-  explicit YieldColumn(Expression *expr, const std::string &alias = "") {
+  explicit YieldColumn(Expression *expr, const std::string &alias = "", bool visible = true) {
     expr_ = expr;
     alias_ = alias;
+    visible_ = visible;
   }
 
   std::unique_ptr<YieldColumn> clone() const {
-    return std::make_unique<YieldColumn>(expr_->clone(), alias_);
+    return std::make_unique<YieldColumn>(expr_->clone(), alias_, visible_);
   }
 
   void setExpr(Expression *expr) {
@@ -285,9 +286,21 @@ class YieldColumn final {
 
   std::string toString() const;
 
+  bool isVisible() const {
+    return visible_;
+  }
+
+  void setVisible(bool visible) {
+    visible_ = visible;
+  }
+
  private:
   Expression *expr_{nullptr};
   std::string alias_;
+  // If some columns are required by order-by and are not explicitly
+  // declared in the return caluse, it is an invisible column and not to be
+  // produced as results.
+  bool visible_{true};
 };
 
 bool operator==(const YieldColumn &l, const YieldColumn &r);