[Iceberg] Add Histogram Statistic Support #22365

ZacBlanco · 2024-03-29T04:33:28Z

Description

This changes adds support for the histogram statistic type to Iceberg. There are 3 related commits

Adds support for collecting a superset of supported statistics in the HMS and Iceberg. This is required to allow histograms to be supported while the HMS is in use.
Adds the histogram implementation for Iceberg.
Adds documentation for histograms statistics.

Motivation and Context

Histograms result in better accuracy for output row counts from filter nodes when calculating statistics. This should result in closer estimates in the CBO, resulting in better query plans.

Impact

ANALYZE for Iceberg and Hive will now store statistics in the table's Puffin files.
Writes new puffin blob type containing the histogram.

Test Plan

New tests to ensure histograms are collected for supported types
Tests that ensure the histograms result in more accurate plan stat estimates for non-uniform data

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
* Add support for the histogram statistic type. :pr:`22365`

steveburnett · 2024-04-17T14:00:12Z

Consider this suggested change for the release note entry:

== RELEASE NOTES ==

Iceberg Connector Changes
* Add support for the histogram statistic type. :pr:`22365`

steveburnett

Thanks for the docs! A few nits suggested for clarity and style. Let me know what you think.

presto-docs/src/main/sphinx/admin/properties.rst

presto-docs/src/main/sphinx/optimizer/statistics.rst

github-actions · 2024-04-23T17:14:45Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff f9f884d...ee5e3fd.

No notifications.

steveburnett

LGTM! (docs)

Pull updated branch, new local docs build, everything looks good. Thanks!

hantangwangd

Took a rough glance, and bring some questions for discussion. Will take a detailed look in the next one or two days.

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java

presto-iceberg/pom.xml

steveburnett

Thanks for the updated doc! Two nits about formatting, no issues with the content in any of the three doc files.

presto-docs/src/main/sphinx/admin/properties.rst

presto-docs/src/main/sphinx/optimizer/statistics.rst

steveburnett

LGTM! (docs)

Thanks for the quick fix! Pull updated branch, new local doc build, looks good.

hantangwangd

Thanks for the fix. The changes look good to me, only some nits.

presto-iceberg/src/main/java/com/facebook/presto/iceberg/statistics/KllHistogram.java

...o-spi/src/test/java/com/facebook/presto/spi/statistics/TestDisjointRangeDomainHistogram.java

presto-iceberg/src/main/java/com/facebook/presto/iceberg/statistics/KllHistogram.java

steveburnett

LGTM! (docs)
Pull updated branch, new local docs build, everything looks good. Thanks!

steveburnett

LGTM! (docs)
Pull updated branch, new local docs build, everything looks good. Thanks!

The set of ColumnStatisticsMetadata defined by the Hive and non-Hive connectors are not equivalent. However, it is possible to collect the superset of the relevant metadata and use it for ANALYZE. The returned statistics just need to be filtered out to contain only the relevant column statistics. This may include duplicate calculations for some statistics. For example, with distinct values Iceberg puffin files can store the result of sketch_theta for distinct values, but the code path for storing the statistic in the HMS requires a direct value from approx_distinct. Thus, ANALYZE may compute a value twice.

aaneja

Did a short first pass

aaneja · 2024-10-04T11:58:15Z

presto-docs/src/main/sphinx/optimizer/statistics.rst


 The set of statistics available for a particular query depends on the connector
 being used and can also vary by table or even by table layout. For example, the
 Hive connector does not currently provide statistics on data size.

-Table statistics can be displayed via the Presto SQL interface using the
+Table statistics can be displayed using a SQL statement with the


nit: can be fetched using the below SQL query sounds better to me

presto-docs/src/main/sphinx/optimizer/statistics.rst

presto-docs/src/main/sphinx/sql/show-stats.rst

presto-spi/src/main/java/com/facebook/presto/spi/statistics/HistogramCalculator.java

aaneja · 2024-10-04T12:35:28Z

presto-common/src/main/java/com/facebook/presto/common/Utils.java

+        }
+    }
+
+    public static ToStringHelper toStringHelper(Object self)


Let's attribute this to Guava ?

shouldn't we just use Guava for all of this? We already depend on it. If there is some reason we want to fork this code, then full tests are needed. But it should be much easier just to call Guava

The guava dependency is not included in presto-common as the presto-common module is a dependency of the presto-spi module which we avoid putting additional dependencies so that when creating connectors or other plugins there it is less likely for downstream users of presto to encounter issues or conflicts

OK, that's reasonable. However if we're forking this in, we also MUST include the tests as well, and be prepared to support this for the foreseeable future. I'm not sure it's worth the effort. I'd be inclined to just manually build the strings in the toString method like we've been doing since 1995. :-)

elharo · 2024-10-04T12:53:12Z

presto-common/src/main/java/com/facebook/presto/common/Utils.java

+        }
+    }
+
+    public static ToStringHelper toStringHelper(Object self)


shouldn't we just use Guava for all of this? We already depend on it. If there is some reason we want to fork this code, then full tests are needed. But it should be much easier just to call Guava

presto-common/src/main/java/com/facebook/presto/common/Utils.java

presto-common/src/main/java/com/facebook/presto/common/predicate/Marker.java

elharo · 2024-10-05T18:59:01Z

presto-common/src/main/java/com/facebook/presto/common/Utils.java

+        }
+    }
+
+    public static ToStringHelper toStringHelper(Object self)


OK, that's reasonable. However if we're forking this in, we also MUST include the tests as well, and be prepared to support this for the foreseeable future. I'm not sure it's worth the effort. I'd be inclined to just manually build the strings in the toString method like we've been doing since 1995. :-)

Utilizes the sketch_kll function to generate histograms and store them into the Iceberg table's puffin files for table-level statistic storage. Histograms are always collected by ANALYZE, but they are not used by the cost calculator unless enabled via optimizer.use-histograms

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch from 58cfe30 to e1a2caa Compare April 17, 2024 13:37

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch 6 times, most recently from bac6b3f to 0e3a87c Compare April 23, 2024 15:26

steveburnett requested changes Apr 23, 2024

View reviewed changes

ZacBlanco marked this pull request as ready for review April 23, 2024 17:13

ZacBlanco requested review from a team, hantangwangd and jaystarshot as code owners April 23, 2024 17:13

ZacBlanco requested a review from presto-oss April 23, 2024 17:13

ZacBlanco requested review from aaneja and ClarenceThreepwood April 23, 2024 17:15

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch 2 times, most recently from 3d551e9 to 65b6fe8 Compare April 26, 2024 03:02

steveburnett previously approved these changes Apr 26, 2024

View reviewed changes

ZacBlanco dismissed steveburnett’s stale review via 0d6389a June 4, 2024 17:46

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch from 65b6fe8 to 0d6389a Compare June 4, 2024 17:46

ZacBlanco requested a review from feilong-liu as a code owner June 4, 2024 17:46

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch from 0d6389a to 24398a5 Compare June 4, 2024 21:55

hantangwangd reviewed Jun 13, 2024

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java Outdated Show resolved Hide resolved

presto-iceberg/pom.xml Outdated Show resolved Hide resolved

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch from 24398a5 to 56b79b0 Compare June 20, 2024 17:51

steveburnett requested changes Jun 20, 2024

View reviewed changes

presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/optimizer/statistics.rst Outdated Show resolved Hide resolved

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch from 56b79b0 to 492ab5f Compare June 20, 2024 19:31

steveburnett previously approved these changes Jun 20, 2024

View reviewed changes

hantangwangd reviewed Jun 24, 2024

View reviewed changes

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch from 492ab5f to 3fccf39 Compare July 2, 2024 15:57

steveburnett previously approved these changes Jul 2, 2024

View reviewed changes

tdcmeehan self-assigned this Jul 2, 2024

ZacBlanco dismissed steveburnett’s stale review via 6e20e34 July 9, 2024 21:30

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch 2 times, most recently from 6e20e34 to 0dceef8 Compare July 9, 2024 23:10

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch from 0dceef8 to 3476da9 Compare September 4, 2024 00:15

ZacBlanco requested a review from elharo as a code owner September 4, 2024 00:15

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch 5 times, most recently from 720d06f to e16dfe6 Compare September 6, 2024 19:51

steveburnett previously approved these changes Sep 6, 2024

View reviewed changes

ZacBlanco dismissed steveburnett’s stale review via 56b02fd September 23, 2024 16:26

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch 2 times, most recently from 56b02fd to d041947 Compare September 23, 2024 17:48

aaneja reviewed Oct 4, 2024

View reviewed changes

elharo requested changes Oct 4, 2024

View reviewed changes

presto-common/src/main/java/com/facebook/presto/common/Utils.java Show resolved Hide resolved

presto-common/src/main/java/com/facebook/presto/common/Utils.java Show resolved Hide resolved

presto-common/src/main/java/com/facebook/presto/common/predicate/Marker.java Show resolved Hide resolved

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch from d041947 to eae161e Compare October 4, 2024 21:12

elharo requested changes Oct 5, 2024

View reviewed changes

ZacBlanco added 2 commits October 8, 2024 16:27

Add histogram statistic documentation

ee5e3fd

ZacBlanco force-pushed the upstream-iceberg-histogram-storage branch from eae161e to ee5e3fd Compare October 8, 2024 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Iceberg] Add Histogram Statistic Support #22365

[Iceberg] Add Histogram Statistic Support #22365

ZacBlanco commented Mar 29, 2024 •

edited

Loading

steveburnett commented Apr 17, 2024

steveburnett left a comment

github-actions bot commented Apr 23, 2024 •

edited

Loading

steveburnett left a comment

hantangwangd left a comment

steveburnett left a comment

steveburnett left a comment

hantangwangd left a comment

steveburnett left a comment

steveburnett left a comment

aaneja left a comment

aaneja Oct 4, 2024

aaneja Oct 4, 2024

elharo Oct 4, 2024

ZacBlanco Oct 4, 2024

elharo Oct 5, 2024 •

edited

Loading

elharo Oct 4, 2024

elharo Oct 5, 2024 •

edited

Loading

[Iceberg] Add Histogram Statistic Support #22365

Are you sure you want to change the base?

[Iceberg] Add Histogram Statistic Support #22365

Conversation

ZacBlanco commented Mar 29, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

steveburnett commented Apr 17, 2024

steveburnett left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 23, 2024 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

hantangwangd left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

hantangwangd left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

aaneja left a comment

Choose a reason for hiding this comment

aaneja Oct 4, 2024

Choose a reason for hiding this comment

aaneja Oct 4, 2024

Choose a reason for hiding this comment

elharo Oct 4, 2024

Choose a reason for hiding this comment

ZacBlanco Oct 4, 2024

Choose a reason for hiding this comment

elharo Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

elharo Oct 4, 2024

Choose a reason for hiding this comment

elharo Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

ZacBlanco commented Mar 29, 2024 •

edited

Loading

github-actions bot commented Apr 23, 2024 •

edited

Loading

elharo Oct 5, 2024 •

edited

Loading

elharo Oct 5, 2024 •

edited

Loading