Skip to content

[native] Error out on StreamingAggregation #22998

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

Closed
karteekmurthys wants to merge 1 commit into from
Closed

[native] Error out on StreamingAggregation #22998

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
6 changes: 5 additions & 1 deletion 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 @@ -989,7 +989,11 @@ VeloxQueryPlanConverterBase::toVeloxQueryPlan(
bool streamable = !node->preGroupedVariables.empty() &&
node->groupingSets.groupingSetCount == 1 &&
node->groupingSets.globalGroupingSets.empty();

// TODO karteekmurthys@ Re-enable after
// fixing:https://github.com/prestodb/presto/issues/22585
if (streamable) {
Copy link
Copy Markdown
Contributor

@rschlussel rschlussel Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for all the back and forth. instead of failing in plan conversion for aggregation as you are doing here, can we fail in plan conversion for join if it's a nested loop join? That would be a more robust mitigation because it's the root cause of the issue, and there are other optimizations aside from streaming aggregation that might be assuming nested loop join is preserving order when it's not e.g. we might remove an order by because we think the input is already ordered or plan a merge join because we think the input is ordered.

Copy link
Copy Markdown
Contributor

@rschlussel rschlussel Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh looks like https://github.com/prestodb/presto/pull/23341/files is already doing this.

Copy link
Copy Markdown
Contributor

@kgpai kgpai Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I am turning off NLJ here : #23341 .

VELOX_UNSUPPORTED("StreamingAggregation is not supported");
}
// groupIdField and globalGroupingSets are required for producing default
// output rows for global grouping sets when there are no input rows.
// Global grouping sets can be present without groupIdField in Final
Expand Down