Skip to content

Commit

Permalink
Add duplicated sorting keys validation in TopNNode
Browse files Browse the repository at this point in the history
  • Loading branch information
JkSelf committed Nov 8, 2023
1 parent bee2998 commit 6a27cf3
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 6 deletions.
12 changes: 6 additions & 6 deletions cpp/velox/substrait/SubstraitToVeloxPlan.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,12 @@ class SubstraitToVeloxPlanConverter {
/// Get aggregation step from AggregateRel.
core::AggregationNode::Step toAggregationStep(const ::substrait::AggregateRel& sAgg);

/// Helper Function to convert Substrait sortField to Velox sortingKeys and
/// sortingOrders.
std::pair<std::vector<core::FieldAccessTypedExprPtr>, std::vector<core::SortOrder>> processSortField(
const ::google::protobuf::RepeatedPtrField<::substrait::SortField>& sortField,
const RowTypePtr& inputType);

private:
/// Integrate Substrait emit feature. Here a given 'substrait::RelCommon'
/// is passed and check if emit is defined for this relation. Basically a
Expand Down Expand Up @@ -328,12 +334,6 @@ class SubstraitToVeloxPlanConverter {
std::vector<variant> values_;
};

/// Helper Function to convert Substrait sortField to Velox sortingKeys and
/// sortingOrders.
std::pair<std::vector<core::FieldAccessTypedExprPtr>, std::vector<core::SortOrder>> processSortField(
const ::google::protobuf::RepeatedPtrField<::substrait::SortField>& sortField,
const RowTypePtr& inputType);

/// Returns unique ID to use for plan node. Produces sequential numbers
/// starting from zero.
std::string nextPlanNodeId();
Expand Down
28 changes: 28 additions & 0 deletions cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,34 @@ bool SubstraitToVeloxPlanValidator::validate(const ::substrait::FetchRel& fetchR
logValidateMsg("native validation failed due to: Offset and count should be valid in FetchRel.");
return false;
}

core::PlanNodePtr childNode;
// Check the input of fetchRel, if it's sortRel, we need to check whether the sorting key is duplicated.
::substrait::SortRel sortRel;
bool topNFlag;
if (fetchRel.has_input()) {
topNFlag = fetchRel.input().has_sort();
if (topNFlag) {
sortRel = fetchRel.input().sort();
childNode = planConverter_.toVeloxPlan(sortRel.input());
} else {
childNode = planConverter_.toVeloxPlan(fetchRel.input());
}
}

if (topNFlag) {
auto [sortingKeys, sortingOrders] = planConverter_.processSortField(sortRel.sorts(), childNode->outputType());

folly::F14FastSet<std::string> sortingKeyNames;
for (const auto& sortingKey : sortingKeys) {
auto result = sortingKeyNames.insert(sortingKey->name());
if (!result.second) {
logValidateMsg(
"native validation failed due to: if the input of fetchRel is a SortRel, we will convert it to a TopNNode. In Velox, it is important to ensure unique sorting keys. However, duplicate keys were found in this case.");
return false;
}
}
}
return true;
}

Expand Down

0 comments on commit 6a27cf3

Please sign in to comment.