[native] Disable Nested Loop Join in Prestissimo. by kgpai · Pull Request #23341 · prestodb/presto

kgpai · 2024-07-31T16:53:24Z

Description

This PR disables NestedLoopJoins when converting plans to Velox. This is gated by the property : fail_on_nested_loop_join .

Motivation and Context

Currently there is a bug with Streaming aggregations when the source is a Nested loop join. (#22585)

Impact

Test Plan

Adding test cases.

aditi-pandit · 2024-07-31T17:21:20Z

@kgpai @amitkdutta : We have encountered this issue already (#22585). This drastic fix is not needed.

You can correct the grouping behavior to avoid streaming aggregation using #22998
The more elaborate fix is to avoid generating such plans in the optimizer #23315

If there is urgency then #22998 is better for you.

amitkdutta · 2024-07-31T17:33:14Z

@kgpai @amitkdutta : We have encountered this issue already (#22585). This drastic fix is not needed.

You can correct the grouping behavior to avoid streaming aggregation using #22998 The more elaborate fix is to avoid generating such plans in the optimizer #23315

If there is urgency then #22998 is better for you.

CC: @pedroerp @mbasmanova

rschlussel · 2024-07-31T17:39:24Z

presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp

+  auto streamable = !node->preGroupedVariables.empty()
+      && node->groupingSets.groupingSetCount == 1
+      && node->groupingSets.globalGroupingSets.empty();
+  VELOX_CHECK(!streamable,


Is it possible to just use a regular aggregation instead of failing?

@rschlussel : Yes. You are correct.
There is a nuance though, HashAggregation also has preGrouping related optimizations which streamable should disable.

That is the fix in #22998.

aditi-pandit · 2024-07-31T17:49:15Z

@kgpai @amitkdutta : We have encountered this issue already (#22585). This drastic fix is not needed.
You can correct the grouping behavior to avoid streaming aggregation using #22998 The more elaborate fix is to avoid generating such plans in the optimizer #23315
If there is urgency then #22998 is better for you.

CC: @pedroerp @mbasmanova

@pedroerp @mbasmanova : For context we had tried fixing the NestedLoopJoin in Velox to maintain the grouping property on LHS as well, but there was lot of overhead to maintain RHS to give such behavior. ref facebookincubator/velox#10048

kgpai · 2024-08-01T18:15:32Z

@aditi-pandit Based on inputs from @rschlussel its probably safer to turn off NLJ till its fixed.

aditi-pandit · 2024-08-01T18:19:01Z

@aditi-pandit Based on inputs from @rschlussel its probably safer to turn off NLJ till its fixed.

@kgpai @rschlussel : Agree this is a more conservative fix. Please feel free to go ahead with it if you prefer.

@karteekmurthys

github-actions · 2024-08-01T18:21:02Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 27a3384...aa5761e.

No notifications.

facebook-github-bot · 2024-08-01T18:33:31Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-08-01T19:03:10Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

aditi-pandit

Thanks @kgpai

kgpai · 2024-08-01T19:24:05Z

I am seeing a couple of failures related to docker compose: https://github.com/prestodb/presto/actions/runs/10204170880/job/28232117475?pr=23341

++ docker-compose version
presto-hive-hadoop2/bin/common.sh: line 79: docker-compose: command not found

facebook-github-bot · 2024-08-01T22:11:29Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kgpai · 2024-08-01T23:14:25Z

@rschlussel @amitkdutta @aditi-pandit Please have a look again, I have gated this under a configuration.

facebook-github-bot · 2024-08-01T23:22:22Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

karteekmurthys · 2024-08-01T23:24:36Z

@kgpai Would you please also disable streaming agg when nestedloopjoin is enabled?

cc: @aditi-pandit

facebook-github-bot · 2024-08-01T23:38:26Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-08-02T00:21:33Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

aditi-pandit · 2024-08-02T03:03:12Z

@kgpai Would you please also disable streaming agg when nestedloopjoin is enabled?

cc: @aditi-pandit

@karteekmurthys : Let Krishna proceed with this change to keep it minimal. You can add the correction (disable pre-grouping (not disable streaming)) when !disableNestedLoopJoin in a subsequent PR since you are more familiar with those code-paths.

@kgpai , @amitkdutta

kgpai · 2024-08-02T18:13:39Z

Unfortunately there are a lot more tests than I anticipated that depend on NLJ.

pedroerp · 2024-08-02T20:04:31Z

This PR should address the NLJ issue in Velox, so hopefully this won't be needed: facebookincubator/velox#10651

kgpai requested a review from a team as a code owner July 31, 2024 16:53

kgpai force-pushed the disable_sorted_aggregations branch from ac84520 to 0005412 Compare July 31, 2024 17:20

rschlussel reviewed Jul 31, 2024

View reviewed changes

kgpai force-pushed the disable_sorted_aggregations branch from 0005412 to fe2f054 Compare August 1, 2024 18:06

kgpai changed the title ~~[WIP] [Native] Disable Streaming aggregations in Prestissimo.~~ [WIP] [Native] Disable Nested Loop Join in Prestissimo. Aug 1, 2024

kgpai force-pushed the disable_sorted_aggregations branch from fe2f054 to ff877fc Compare August 1, 2024 18:08

kgpai mentioned this pull request Aug 1, 2024

[native] Error out on StreamingAggregation #22998

Closed

6 tasks

kgpai force-pushed the disable_sorted_aggregations branch from ff877fc to 28a6819 Compare August 1, 2024 19:00

rschlussel previously approved these changes Aug 1, 2024

View reviewed changes

aditi-pandit previously approved these changes Aug 1, 2024

View reviewed changes

amitkdutta changed the title ~~[WIP] [Native] Disable Nested Loop Join in Prestissimo.~~ [native] Disable Nested Loop Join in Prestissimo. Aug 1, 2024

kgpai dismissed stale reviews from aditi-pandit and rschlussel via d84549a August 1, 2024 22:02

kgpai force-pushed the disable_sorted_aggregations branch from 28a6819 to d84549a Compare August 1, 2024 22:02

kgpai force-pushed the disable_sorted_aggregations branch 2 times, most recently from 1173acf to c6b6eb3 Compare August 1, 2024 23:13

kgpai force-pushed the disable_sorted_aggregations branch from c6b6eb3 to 092dca3 Compare August 1, 2024 23:21

kgpai force-pushed the disable_sorted_aggregations branch from 092dca3 to 2d2f62a Compare August 1, 2024 23:32

kgpai force-pushed the disable_sorted_aggregations branch from 2d2f62a to 67c4a39 Compare August 2, 2024 00:10

kgpai force-pushed the disable_sorted_aggregations branch 6 times, most recently from c028258 to 61cdbb8 Compare August 2, 2024 18:12

kgpai force-pushed the disable_sorted_aggregations branch from 61cdbb8 to 076b32d Compare August 2, 2024 19:31

Disable nested loop join.

aa5761e

kgpai force-pushed the disable_sorted_aggregations branch from 076b32d to aa5761e Compare August 2, 2024 20:44

amitkdutta mentioned this pull request Aug 4, 2024

Ensure Nested Loop Join output follows probe order facebookincubator/velox#10651

Closed

amitkdutta mentioned this pull request Aug 12, 2024

[native] Add hook for Velox plan validation. #23423

Merged

Conversation

kgpai commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Uh oh!

aditi-pandit commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitkdutta commented Jul 31, 2024

Uh oh!

rschlussel Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

aditi-pandit Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

aditi-pandit commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kgpai commented Aug 1, 2024

Uh oh!

aditi-pandit commented Aug 1, 2024

Uh oh!

github-actions bot commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Aug 1, 2024

Uh oh!

facebook-github-bot commented Aug 1, 2024

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

kgpai commented Aug 1, 2024

Uh oh!

facebook-github-bot commented Aug 1, 2024

Uh oh!

kgpai commented Aug 1, 2024

Uh oh!

facebook-github-bot commented Aug 1, 2024

Uh oh!

karteekmurthys commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Aug 1, 2024

Uh oh!

facebook-github-bot commented Aug 2, 2024

Uh oh!

aditi-pandit commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kgpai commented Aug 2, 2024

Uh oh!

pedroerp commented Aug 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kgpai commented Jul 31, 2024 •

edited

Loading

aditi-pandit commented Jul 31, 2024 •

edited

Loading

aditi-pandit commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Aug 1, 2024 •

edited

Loading

karteekmurthys commented Aug 1, 2024 •

edited

Loading

aditi-pandit commented Aug 2, 2024 •

edited

Loading