Fix optimizer performance regression for large IN by ZacBlanco · Pull Request #22664 · prestodb/presto

ZacBlanco · 2024-05-03T19:37:33Z

Description

Fixes the bug found in #22661

This bug occurs due to large IN lists. When computing
statistics for queries with this clause, we generate an instance
of VariableStatisticEstimate for each expression in the list.

For example, a query with

SELECT ... IN (1, 2,  ... 10_000);

generates 10k VariableStatisticEstimate instances. Then, for
each instance, we sum the statistics and distinct values
to get a final result to determine the probability of a value
occurring. Creating these VariableStatisticEstimate instances
were not the root cause of the issue.

The main problem is that when the
DIsjointRangeDomainHistogram was introduced it was designed as
immutable. When folding all the instances together through
the reduce() call at FilterStatsCalculator#L755, it re-creates
the internal TreeRangeSet. The set of disjoint ranges is the size
of the IN list, so we were re-creating the set 10k times. The first
with range a set size of 1, then 2, ... etc up to 10k. So the
running time of this ended up being polynomial.

The following change updates the DisjointRangeDomain histogram
to use a lazily initialized RangeSet. This prevents incurring the
high cost of re-creating the range set for every new addition to
the IN list.

Additionally, the StatisticRange class now serializes to a byte
encoded format to decrease the amount of bytes required to
serialize plans with many filters. This is mainly useful for
when a query has thousands of filters in a complex plan and the
filters are applied to the histogram.

Impact

Re-adds the original changes from the histogram PR
Plans with a significant number of filters can more quickly hit the limit imposed by the configuration: experimental.internal-communication.max-task-update-size due to filter ranges also being stored on top of the histogram. In local testing I've been able to hit about 17k filter values before reaching the plan serialization limit. To combat this, I've disabled histograms from being serialized in the JSON structure of the statistics because it shouldn't be used outside of the coordinator.
Prevents performing any calculations on histograms when optimizer_use_histograms is set to false.

Test Plan

Added a test to ensure that large filter lists don't degrade the optimizer performance

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

== NO RELEASE NOTE ==

elharo · 2024-05-04T11:06:07Z

"The main problem is that when the DIsjointRangeDomainHistogram was introduced it was designed as immutable. When folding all of the instances together via the reduce() call at FilterStatsCalculator#L755 it re-creates the internal TreeRangeSet. The set of disjoint ranges is the size of the IN list, so we were re-creating the set 10k times. The first with range a set size of 1, then 2, ... etc up to 10k."

Tangential note: this is exactly the pattern that led to the handshoe DoS attack on protobuf. Immutable data structures are not as risk-free as they seem.

jaystarshot · 2024-05-06T02:32:37Z

I missed some context, but why is this a problem when the property (use_histogram) is turned off?

ZacBlanco · 2024-05-06T02:36:28Z

This happened because the histograms were still computed in PlanNodeStatsEstimateMath here which was not gated by the property:

https://github.com/ZacBlanco/prestodb/blob/b377b41532c4607dd79ff83fa56d9da374995a76/presto-main/src/main/java/com/facebook/presto/cost/PlanNodeStatsEstimateMath.java#L289

Before the histogram PR, this line was not there. For a large IN list we end up calling addColumnStats 1 time for each constant in the IN list. The reason the performance regressed is due to the cost of creating a new instance of the TreeRangeSet with many constant values

feilong-liu · 2024-05-06T07:06:40Z

Thanks for identifying the root cause so fast!

I missed some context, but why is this a problem when the property (use_histogram) is turned off?

Second @jaystarshot on this. I think we need to have this histogram computation disabled when this property is turned off.

Ideally, we should have the histogram related changes to be completely turned off when the property is set to false. I see that this was discussed in the original PR #21236 (comment), and understand that this may need additional code change and having additional boolean flag passed around. But I think we need to find a sweet balance point, and try to gate as much change as possible.

presto-main/src/main/java/com/facebook/presto/util/Lazy.java

presto-main/src/main/java/com/facebook/presto/cost/VariableStatsEstimate.java

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

feilong-liu · 2024-05-14T23:13:13Z

presto-main/src/main/java/com/facebook/presto/cost/ComparisonStatsCalculator.java

            OptionalDouble literalValue,
-            ComparisonExpression.Operator operator)
+            ComparisonExpression.Operator operator,
+            Optional<Session> session)


If only histogram field in the session is used, can we pass a boolean instead of the whole session around?

Do you see a specific downside to passing the session? If we intend to make changes in the future which reference more feature flags, I feel it would be better to pass the session rather than adding a new argument for each potentially new flag

I was thinking about keeping it consistent as I see in some other cases it's passing a boolean flag. But also fine if you think passing session is better.

...java/com/facebook/presto/operator/scalar/queryplan/TestJsonPrestoQueryPlanFunctionUtils.java

ZacBlanco · 2024-05-23T22:39:24Z

presto-main/src/main/java/com/facebook/presto/cost/VariableStatsEstimate.java

+    // We ignore the histogram during serialization because histograms can be
+    // quite large. Histograms are not used outside the coordinator, so there
+    // isn't a need to serialize them
+    @JsonIgnore


@rschlussel This is the change to prevent serializing histograms. An associated test is added in TestVariableStatsEstimate

rschlussel

just fix the json property and should be good.

presto-main/src/main/java/com/facebook/presto/cost/VariableStatsEstimate.java

rschlussel · 2024-05-24T20:00:53Z

presto-main/src/main/java/com/facebook/presto/cost/VariableStatsEstimate.java

+            @JsonProperty("highValue") double highValue,
+            @JsonProperty("nullsFraction") double nullsFraction,
+            @JsonProperty("averageRowSize") double averageRowSize,
+            @JsonProperty("distinctValuesCount") double distinctValuesCount)


ah sorry I thought you should be able to remove the json property from the original constructor, but I can't find similar examples, so maybe not. Anyway, before was fine and this is also fine.

hantangwangd

The description in the first commit has a little inaccuracy,ConnectorHistogram have 2 methods, not 4 :-).

hantangwangd

Thanks for the great work, overall LGTM except some little things.

presto-main/src/main/java/com/facebook/presto/cost/DisjointRangeDomainHistogram.java

presto-main/src/main/java/com/facebook/presto/cost/HistogramCalculator.java

This commits provides two critical changes: 1. Adds a new enum value to ColumnStatisticType: Histogram. 2. Utilizes the new histograms in optimizer's cost calculations Implementation details below: With this change, a new column statistic type for histograms is introduced. In addition, a new SPI class `ConnectorHistogram` is also introduced. This interface is designed to be able to be implemented by either the connectors or in the main presto codebase. This should allow connectors to return utilize histogram statistics in any format regardless of the source. The API is straightforward and includes 2 methods. - cumulativeProbability(double value, boolean inclusive): -> CDF function - inverseCumulativeProbability(double probability) -> inverse CDF function A reference implementation is provided inside of UniformDistributionHistogram. This implementation results in the same logic and same plans as the previous cost-based calculations. The math ends up being the same, but just utilizing the histogram API. Additionally, to propagate histogram information further up into a plan, another implementation of histograms is provided inside of DisjointRangeDomainHistogram. This class is used to bound a source histogram with a given domain as additional filters may be applied further up in the plan. Previously all cost calculations were performed inside of the ComparisonStatsCalculator using logic from the StatisticRange class to calculate the filter factors of overlapping and intersecting ranges. This change introduces cost calculation using the new histogram model and API. The core of the filter proportion calculation logic exists in the new HistogramCalculator utility class. If the underlying Histogram implementation is swapped from the UniformDistributionHistogram, then the stats calculator will calculate the costs using the histogram information.

hantangwangd

LGTM. Thanks for the fix and the great work!

presto-main/src/main/java/com/facebook/presto/cost/PlanNodeStatsEstimateMath.java

presto-main/src/main/java/com/facebook/presto/cost/StatisticRange.java

presto-main/src/main/java/com/facebook/presto/cost/ConnectorFilterStatsCalculatorService.java

This bug occurs due to large IN lists. When computing statistics for queries with this clause, we generate an instance of VariableStatisticEstimate for each expression in the list. For example, a query with SELECT ... IN (1, 2, ... 10_000); generates 10k VariableStatisticEstimate instances. Then, for each instance, we sum the statistics and distinct values to get a final result to determine the probability of a value occurring. Creating these VariableStatisticEstimate instances were not the root cause of the issue. The main problem is that when the DIsjointRangeDomainHistogram was introduced it was designed as immutable. When folding all the instances together through the `reduce()` call at FilterStatsCalculator#L755, it re-creates the internal TreeRangeSet. The set of disjoint ranges is the size of the IN list, so we were re-creating the set 10k times. The first with range a set size of 1, then 2, ... etc up to 10k. So the running time of this ended up being polynomial. The following change updates the DisjointRangeDomain histogram to use a lazily initialized RangeSet. This prevents incurring the high cost of re-creating the range set for every new addition to the IN list. Additionally, the StatisticRange class now serializes to a byte encoded format to decrease the amount of bytes required to serialize plans with many filters. This is mainly useful for when a query has thousands of filters in a complex plan and the filters are applied to the histogram.

tdcmeehan

Thanks @ZacBlanco

rschlussel · 2024-06-06T19:01:21Z

I saw a test failure today from testLargeInWithHistograms test #22944

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from 37237da to 298bb28 Compare May 3, 2024 19:42

elharo previously approved these changes May 4, 2024

View reviewed changes

ZacBlanco mentioned this pull request May 6, 2024

Re-commit iceberg statistics and table handle revert #22666

Merged

6 tasks

ZacBlanco dismissed elharo’s stale review via 4b73697 May 6, 2024 16:10

ZacBlanco force-pushed the upstream-fix-slow-large-in branch 9 times, most recently from 115f618 to d74daa7 Compare May 10, 2024 21:39

ZacBlanco marked this pull request as ready for review May 13, 2024 17:48

ZacBlanco requested review from a team, feilong-liu, hantangwangd and jaystarshot as code owners May 13, 2024 17:48

ZacBlanco requested a review from presto-oss May 13, 2024 17:48

tdcmeehan reviewed May 13, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/util/Lazy.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/util/Lazy.java Outdated Show resolved Hide resolved

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from d74daa7 to ffdf74b Compare May 14, 2024 21:52

feilong-liu reviewed May 14, 2024

View reviewed changes

feilong-liu requested review from kaikalur and rschlussel May 14, 2024 23:13

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from ffdf74b to 9390dcf Compare May 16, 2024 23:05

jaystarshot reviewed May 21, 2024

View reviewed changes

...java/com/facebook/presto/operator/scalar/queryplan/TestJsonPrestoQueryPlanFunctionUtils.java Outdated Show resolved Hide resolved

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from 10d4345 to 60c5f06 Compare May 21, 2024 16:51

ZacBlanco mentioned this pull request May 21, 2024

Remove fields from PlanFragment for task serialization #22771

Merged

6 tasks

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from 60c5f06 to 43ef3e7 Compare May 23, 2024 22:37

ZacBlanco commented May 23, 2024

View reviewed changes

rschlussel previously approved these changes May 24, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/cost/VariableStatsEstimate.java Outdated Show resolved Hide resolved

ZacBlanco dismissed rschlussel’s stale review via bf57810 May 24, 2024 16:26

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from 43ef3e7 to bf57810 Compare May 24, 2024 16:26

rschlussel previously approved these changes May 24, 2024

View reviewed changes

aaneja previously approved these changes May 28, 2024

View reviewed changes

hantangwangd reviewed May 29, 2024

View reviewed changes

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from bf57810 to 9d6b7fb Compare May 29, 2024 18:19

hantangwangd reviewed May 30, 2024

View reviewed changes

ZacBlanco dismissed stale reviews from rschlussel and aaneja via 6b04326 May 30, 2024 23:02

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from 9d6b7fb to 6b04326 Compare May 30, 2024 23:02

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from 6b04326 to e7061d8 Compare May 31, 2024 15:32

hantangwangd previously approved these changes Jun 3, 2024

View reviewed changes

feilong-liu previously approved these changes Jun 3, 2024

View reviewed changes

tdcmeehan reviewed Jun 3, 2024

View reviewed changes

ZacBlanco added 2 commits June 3, 2024 17:57

Gate histogram calcs by session property

75c1c07

ZacBlanco dismissed stale reviews from feilong-liu and hantangwangd via 75c1c07 June 4, 2024 01:04

ZacBlanco force-pushed the upstream-fix-slow-large-in branch from e7061d8 to 75c1c07 Compare June 4, 2024 01:04

jaystarshot approved these changes Jun 4, 2024

View reviewed changes

tdcmeehan approved these changes Jun 4, 2024

View reviewed changes

ZacBlanco merged commit 39e11a5 into prestodb:master Jun 4, 2024

Conversation

ZacBlanco commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

elharo commented May 4, 2024

Uh oh!

jaystarshot commented May 6, 2024

Uh oh!

ZacBlanco commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feilong-liu commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feilong-liu May 14, 2024

Choose a reason for hiding this comment

Uh oh!

ZacBlanco May 15, 2024

Choose a reason for hiding this comment

Uh oh!

feilong-liu May 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZacBlanco May 23, 2024

Choose a reason for hiding this comment

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rschlussel May 24, 2024

Choose a reason for hiding this comment

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

rschlussel commented Jun 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ZacBlanco commented May 3, 2024 •

edited

Loading

ZacBlanco commented May 6, 2024 •

edited

Loading

feilong-liu commented May 6, 2024 •

edited

Loading