Skip to content

[native] Fix crash in window queries without ORDER BY clause #18895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mbasmanova merged 1 commit into from
Jan 12, 2023
+39 −21
Merged

[native] Fix crash in window queries without ORDER BY clause #18895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1855,12 +1855,14 @@ VeloxQueryPlanConverter::toVeloxQueryPlan(

std::vector<core::FieldAccessTypedExprPtr> sortFields;
std::vector<core::SortOrder> sortOrders;
auto nodeSpecOrdering = node->specification.orderingScheme->orderBy;
sortFields.reserve(nodeSpecOrdering.size());
sortOrders.reserve(nodeSpecOrdering.size());
for (const auto& spec : nodeSpecOrdering) {
sortFields.emplace_back(exprConverter_.toVeloxExpr(spec.variable));
sortOrders.emplace_back(toVeloxSortOrder(spec.sortOrder));
if (node->specification.orderingScheme) {
Comment thread
wuxueyang96 marked this conversation as resolved.
Outdated
auto nodeSpecOrdering = node->specification.orderingScheme->orderBy;
sortFields.reserve(nodeSpecOrdering.size());
sortOrders.reserve(nodeSpecOrdering.size());
for (const auto& spec : nodeSpecOrdering) {
sortFields.emplace_back(exprConverter_.toVeloxExpr(spec.variable));
sortOrders.emplace_back(toVeloxSortOrder(spec.sortOrder));
}
}

std::vector<std::string> windowNames;
Expand Down
46 changes: 31 additions & 15 deletions ...ative-execution/src/test/java/com/facebook/presto/nativeworker/TestHiveWindowQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,33 @@
public class TestHiveWindowQueries
extends AbstractTestHiveQueries
{
private static final List<String> OVER_CLAUSES_WITH_ORDER_BY = Arrays.asList(
"PARTITION BY orderkey ORDER BY totalprice",
"PARTITION BY custkey, orderkey ORDER BY totalprice",
"PARTITION BY orderdate ORDER BY orderkey asc, totalprice desc",
"PARTITION BY orderkey, custkey ORDER BY orderdate asc nulls first, totalprice asc, shippriority desc",
"PARTITION BY custkey, orderkey, shippriority ORDER BY orderdate, totalprice asc nulls first",
"PARTITION BY orderkey, orderdate ORDER BY totalprice asc nulls first",
"ORDER BY orderdate desc, totalprice asc, shippriority desc nulls first");

private static final List<String> OVER_CLAUSES_WITHOUT_ORDER_BY = Arrays.asList(
"PARTITION BY orderkey",
"PARTITION BY custkey, orderkey",
"PARTITION BY orderdate",
"PARTITION BY orderkey, orderdate",
"PARTITION BY custkey, orderkey, shippriority",
"PARTITION BY orderkey, custkey");

public TestHiveWindowQueries()
{
super(true);
}

protected List<String> getRankingQueries(String rankingFunction)
protected List<String> getRankingQueries(String rankingFunction, boolean orderBy)
{
ImmutableList.Builder<String> queries = ImmutableList.builder();
List<String> columnProjections = Arrays.asList("orderkey, orderdate, totalprice");
List<String> overClauses = Arrays.asList("PARTITION BY orderkey ORDER BY totalprice",
"PARTITION BY custkey, orderkey ORDER BY totalprice",
"PARTITION BY orderdate ORDER BY orderkey asc, totalprice desc",
"PARTITION BY orderkey, orderdate ORDER BY totalprice asc nulls first",
"PARTITION BY orderkey, custkey ORDER BY orderdate asc nulls first, totalprice asc, shippriority desc",
"PARTITION BY custkey, orderkey, shippriority ORDER BY orderdate, totalprice asc nulls first",
"ORDER BY orderdate desc, totalprice asc, shippriority desc nulls first");
List<String> overClauses = orderBy ? OVER_CLAUSES_WITH_ORDER_BY : OVER_CLAUSES_WITHOUT_ORDER_BY;

for (String columnProjection : columnProjections) {
for (String overClause : overClauses) {
Expand All @@ -47,9 +58,9 @@ protected List<String> getRankingQueries(String rankingFunction)
return queries.build();
}

protected void testRankingFunction(String functionName)
protected void testRankingFunction(String functionName, boolean orderBy)
{
List<String> queries = getRankingQueries(functionName);
List<String> queries = getRankingQueries(functionName, orderBy);
for (String query : queries) {
assertQuery(query);
}
Expand All @@ -58,30 +69,35 @@ protected void testRankingFunction(String functionName)
@Test
public void testCumeDist()
{
testRankingFunction("cume_dist()");
testRankingFunction("cume_dist()", true);
testRankingFunction("cume_dist()", false);
}

@Test
public void testDenseRank()
{
testRankingFunction("dense_rank()");
testRankingFunction("dense_rank()", true);
testRankingFunction("dense_rank()", false);
}

@Test
public void testPercentRank()
{
testRankingFunction("percent_rank()");
testRankingFunction("percent_rank()", true);
testRankingFunction("percent_rank()", false);
}

@Test
public void testRank()
{
testRankingFunction("rank()");
testRankingFunction("rank()", true);
testRankingFunction("rank()", false);
}

@Test
public void testRowNumber()
{
testRankingFunction("row_number()");
// `row_number() over (partition by key1)` will use `RowNumberNode` which hasn't been implemented yet.
testRankingFunction("row_number()", true);
}
}