Broadcast join if build estimation is small and from HBO #22681

feilong-liu · 2024-05-07T00:14:15Z

Description

Previously I added this session property to do broadcast join when build side is small and probe side is unknown. However, in production we found that it will cause problem when the build side estimation is wrong. However, HBO is a confident source and we should be able to enable it when the estimation is from HBO.

Motivation and Context

To improve performance for queries with unknown probe and small build.

Impact

To improve performance for queries with unknown probe and small build.

Test Plan

It's an easy change, and also tested locally end to end.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Improve session property ``property-use_broadcast_when_buildsize_small_probeside_unknown`` to do broadcast join when probe side size is unknown and build side estimation from HBO is small

feilong-liu · 2024-05-07T00:14:42Z

...n/java/com/facebook/presto/sql/planner/iterative/rule/DetermineSemiJoinDistributionType.java

@@ -124,6 +128,12 @@ private PlanNode getCostBasedDistributionType(SemiJoinNode node, Context context
        possibleJoinNodes.add(getSemiJoinNodeWithCost(node.withDistributionType(PARTITIONED), context));

        if (possibleJoinNodes.stream().anyMatch(result -> result.getCost().hasUnknownComponents())) {
+            if (isUseBroadcastJoinWhenBuildSizeSmallProbeSizeUnknownEnabled(context.getSession()) && possibleJoinNodes.stream().anyMatch(result -> ((SemiJoinNode) result.getPlanNode()).getDistributionType().get().equals(REPLICATED))) {


Also enable it for semi join

kaikalur · 2024-05-07T00:20:13Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/DetermineJoinDistributionType.java

@@ -126,7 +127,10 @@ private PlanNode getCostBasedJoin(JoinNode joinNode, Context context)
        if (possibleJoinNodes.stream().anyMatch(result -> result.getCost().hasUnknownComponents()) || possibleJoinNodes.isEmpty()) {
            // TODO: currently this session parameter is added so as to roll out the plan change gradually, after proved to be a better choice, make it default and get rid of the session parameter here.
            if (isUseBroadcastJoinWhenBuildSizeSmallProbeSizeUnknownEnabled(context.getSession()) && possibleJoinNodes.stream().anyMatch(result -> ((JoinNode) result.getPlanNode()).getDistributionType().get().equals(REPLICATED))) {
-                return getOnlyElement(possibleJoinNodes.stream().filter(result -> ((JoinNode) result.getPlanNode()).getDistributionType().get().equals(REPLICATED)).map(x -> x.getPlanNode()).collect(toImmutableList()));
+                JoinNode broadcastJoin = (JoinNode) getOnlyElement(possibleJoinNodes.stream().filter(result -> ((JoinNode) result.getPlanNode()).getDistributionType().get().equals(REPLICATED)).map(x -> x.getPlanNode()).collect(toImmutableList()));


We should do it always - we trust history and if HBO says build side is small, we just broadcast it.

This depends on whether the other side of the join is small or not, i.e. if both sides have estimations showing data size is small, whether we should do partitioned join or broadcast join. I think we can open a separate PR for this.

jaystarshot · 2024-05-07T00:34:33Z

This might be a good time to fix and use the correct confidence level in planNodeStatsEstimate I had a PR #21280 since we were facing a similar issue

jaystarshot · 2024-05-07T01:35:45Z

But that might involve additional testing and could be an overkill for this case so will leave you to decide

steveburnett · 2024-05-07T17:52:15Z

Suggest revising the release note entry to follow the Order of changes in the Release Notes Guidelines. Maybe something like this? Edit this as needed, please.

== RELEASE NOTES ==

General Changes
* Improve session property ``property-name`` (to?) broadcast join when probe side size is unknown and build side estimation from HBO is small

ClarenceThreepwood

I have a couple of related questions

It is well known that incorrect cost estimates can negatively influence optimizer planning. With this change, if the cost were correct, then we lose the benefit of the optimization - do we not?
I agree that the HBO is a better source of truth. I wonder if we should introduce a config parameter (or a set of configs) that govern whether some optimizations should apply based on stats sources? Maintenance becomes hard if we pepper the code-base with hard-coded special cases for HBO optimizations.

ClarenceThreepwood · 2024-05-08T21:19:40Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/DetermineJoinDistributionType.java

@@ -126,7 +127,10 @@ private PlanNode getCostBasedJoin(JoinNode joinNode, Context context)
        if (possibleJoinNodes.stream().anyMatch(result -> result.getCost().hasUnknownComponents()) || possibleJoinNodes.isEmpty()) {
            // TODO: currently this session parameter is added so as to roll out the plan change gradually, after proved to be a better choice, make it default and get rid of the session parameter here.
            if (isUseBroadcastJoinWhenBuildSizeSmallProbeSizeUnknownEnabled(context.getSession()) && possibleJoinNodes.stream().anyMatch(result -> ((JoinNode) result.getPlanNode()).getDistributionType().get().equals(REPLICATED))) {
-                return getOnlyElement(possibleJoinNodes.stream().filter(result -> ((JoinNode) result.getPlanNode()).getDistributionType().get().equals(REPLICATED)).map(x -> x.getPlanNode()).collect(toImmutableList()));
+                JoinNode broadcastJoin = (JoinNode) getOnlyElement(possibleJoinNodes.stream().filter(result -> ((JoinNode) result.getPlanNode()).getDistributionType().get().equals(REPLICATED)).map(x -> x.getPlanNode()).collect(toImmutableList()));


nit: limit lines to 180 chars

feilong-liu · 2024-05-08T23:03:45Z

But that might involve additional testing and could be an overkill for this case so will leave you to decide

Agree that we need to have different confidence level for estimations. I will consider this PR as a simple fix for existing features and not including other major changes.

feilong-liu · 2024-05-08T23:08:18Z

I have a couple of related questions

It is well known that incorrect cost estimates can negatively influence optimizer planning. With this change, if the cost were correct, then we lose the benefit of the optimization - do we not?

Yes, we will. But fail queries which ran successfully is a larger pain than failing to optimize queries already running successful/failing. And this is the reason why we still cannot turn on this feature internally.

I agree that the HBO is a better source of truth. I wonder if we should introduce a config parameter (or a set of configs) that govern whether some optimizations should apply based on stats sources? Maintenance becomes hard if we pepper the code-base with hard-coded special cases for HBO optimizations.

I think this is a good idea and not only applicable to this optimization, but also other optimizations. But it also involves larger changes. I think we can open an issue for this.

jaystarshot · 2024-05-09T05:44:53Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/DetermineJoinDistributionType.java

-                return getOnlyElement(possibleJoinNodes.stream().filter(result -> ((JoinNode) result.getPlanNode()).getDistributionType().get().equals(REPLICATED)).map(x -> x.getPlanNode()).collect(toImmutableList()));
+                JoinNode broadcastJoin = (JoinNode) getOnlyElement(possibleJoinNodes.stream().filter(result -> ((JoinNode) result.getPlanNode()).getDistributionType().get().equals(REPLICATED)).map(x -> x.getPlanNode()).collect(toImmutableList()));
+                if (context.getStatsProvider().getStats(broadcastJoin.getBuild()).getSourceInfo() instanceof HistoryBasedSourceInfo) {
+                    return broadcastJoin;


I think, in future whether the previous join was broadcast/hash and if it was efficient/or if it failed etc could also be stored using HBO

Broadcast join if build estimation is small and from HBO

ac04f21

feilong-liu requested review from jaystarshot and a team as code owners May 7, 2024 00:14

feilong-liu requested a review from presto-oss May 7, 2024 00:14

feilong-liu commented May 7, 2024

View reviewed changes

feilong-liu requested review from kaikalur and rschlussel May 7, 2024 00:14

kaikalur reviewed May 7, 2024

View reviewed changes

tdcmeehan requested a review from ClarenceThreepwood May 7, 2024 13:28

ClarenceThreepwood reviewed May 8, 2024

View reviewed changes

jaystarshot reviewed May 9, 2024

View reviewed changes

jaystarshot approved these changes May 9, 2024

View reviewed changes

feilong-liu merged commit 9c82b1e into prestodb:master May 10, 2024
56 checks passed

feilong-liu deleted the bcast_join branch May 10, 2024 22:26

wanglinsong mentioned this pull request Jun 25, 2024

Add release notes for 0.288 #23079

Merged

36 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broadcast join if build estimation is small and from HBO #22681

Broadcast join if build estimation is small and from HBO #22681

feilong-liu commented May 7, 2024 •

edited

Loading

feilong-liu May 7, 2024

kaikalur May 7, 2024

feilong-liu May 8, 2024

jaystarshot commented May 7, 2024 •

edited

Loading

jaystarshot commented May 7, 2024

steveburnett commented May 7, 2024

ClarenceThreepwood left a comment

ClarenceThreepwood May 8, 2024

feilong-liu commented May 8, 2024

feilong-liu commented May 8, 2024

jaystarshot May 9, 2024

Broadcast join if build estimation is small and from HBO #22681

Broadcast join if build estimation is small and from HBO #22681

Conversation

feilong-liu commented May 7, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

feilong-liu May 7, 2024

Choose a reason for hiding this comment

kaikalur May 7, 2024

Choose a reason for hiding this comment

feilong-liu May 8, 2024

Choose a reason for hiding this comment

jaystarshot commented May 7, 2024 • edited Loading

jaystarshot commented May 7, 2024

steveburnett commented May 7, 2024

ClarenceThreepwood left a comment

Choose a reason for hiding this comment

ClarenceThreepwood May 8, 2024

Choose a reason for hiding this comment

feilong-liu commented May 8, 2024

feilong-liu commented May 8, 2024

jaystarshot May 9, 2024

Choose a reason for hiding this comment

feilong-liu commented May 7, 2024 •

edited

Loading

jaystarshot commented May 7, 2024 •

edited

Loading