RFC-0005. Implementation Scalar function stats propagation, Phase 1 #23545

ScrapCodes · 2024-08-29T09:39:00Z

Description

Support for annotating functions with both constant stats and propagating source stats.
Added tests for the same.
Added Scalar stats calculation based on annotation and tests for the same.
Introduced the feature flag and session flag.

Not added SQLInvokedScalarFunctions.
Not annotated builtin functions, as that is covered in next implementation phase. Not added C++ changes as this phase only covers Java side of changes.

Motivation and Context

https://github.com/prestodb/rfcs/blob/main/RFC-0005-functions-stats.md

Impact

None unless the user chooses to enable the feature via setting the session/feature flag.
A new session flag, scalar_function_stats_propagation_enabled and a new feature config will be introduced i.e. optimizer.scalar-function-stats-propagation-enabled, by setting this session flag or feature flag, this feature can be turned on or off.

Test Plan

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 configuration property ``optimizer.scalar-function-stats-propagation-enabled`` and session property ``scalar_function_stats_propagation_enabled`` to enable stats propagation by annotation, supporting `RFC-5 <https://github.com/prestodb/rfcs/blob/main/RFC-0005-functions-stats.md>`_. :pr: `23545`

steveburnett · 2024-08-29T14:50:12Z

Suggest including documentation for the new configuration property and session property, following the Review and Commit Guidelines.
Suggest revision to the release note entry following the Release Notes Guidelines:

== RELEASE NOTES ==

General Changes
* Add configuration property ``optimizer.scalar-function-stats-propagation-enabled`` and session property ``scalar_function_stats_propagation_enabled`` to enable stats propagation by annotation, supporting `RFC-5 <https://github.com/prestodb/rfcs/blob/main/RFC-0005-functions-stats.md>`_. :pr: `23545`

ScrapCodes · 2024-08-30T07:28:01Z

Thank you for the comment @steveburnett , do you have a suggestion on documenting this feature in full detail e.g. developer docs.

ScrapCodes · 2024-09-11T09:35:21Z

Hi @ZacBlanco , can you please take a look!

steveburnett · 2024-09-11T14:56:46Z

Thank you for the comment @steveburnett , do you have a suggestion on documenting this feature in full detail e.g. developer docs.

Yes, thanks! If these properties are relevant to Presto, please add entries for them to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/admin/properties.rst.

If these are Presto C++ (Prestissimo), please add the entries to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/presto_cpp/features.rst#session-properties and https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/presto_cpp/properties.rst as appropriate.

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

presto-main/src/main/java/com/facebook/presto/operator/scalar/ScalarHeader.java

...c/main/java/com/facebook/presto/operator/scalar/annotations/ScalarFromAnnotationsParser.java

presto-spi/src/main/java/com/facebook/presto/spi/function/Signature.java

steveburnett

Thank you for adding the documentation! A few nits of formatting, and a couple of revisions for consistency with the existing documentation.

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

steveburnett

Thanks for the quick response! Two final nits that I should have caught in my first pass, and I think this should be everything.

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

steveburnett

LGTM! (docs)

Pull updated branch, reviewed new local doc build, looks good. Thanks!

steveburnett

One nit of formatting.

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

steveburnett

LGTM! (docs)

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

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

ZacBlanco

Just a few minor things, otherwise lgtm

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

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

...c/main/java/com/facebook/presto/operator/scalar/annotations/ScalarFromAnnotationsParser.java

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

ScrapCodes · 2024-10-16T06:39:03Z

Hi @elharo is it good to go?

elharo

I'm still not sold on using doubles for row counts and the like. Doubles are not just a bigger long and have round off and representation problems integers don't. If these things are really going to exceed the size of a long (which I'm not sure they do) then a BigInteger would be preferred.

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

ScrapCodes · 2024-10-17T05:03:41Z

I'm still not sold on using doubles for row counts and the like. Doubles are not just a bigger long and have round off and representation problems integers don't. If these things are really going to exceed the size of a long (which I'm not sure they do) then a BigInteger would be preferred.

BigInteger does not exist in java natively. May be this is a decision that presto project has made much before this PR came in. Will it be a good idea, if you can start a mailing group thread with broader audience?

elharo · 2024-10-17T10:54:02Z

BigInteger isn't a primitive type, but java.math.BigInteger is available if you need it. Again, I'm not convinced you do. long should work here.

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

presto-main/src/test/java/com/facebook/presto/cost/TestScalarStatsCalculator.java

ScrapCodes · 2024-11-20T07:08:39Z

Rebased with master

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

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

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

presto-spi/src/main/java/com/facebook/presto/spi/function/ScalarStatsHeader.java

presto-spi/src/main/java/com/facebook/presto/spi/function/ScalarFunctionConstantStats.java

...-main/src/main/java/com/facebook/presto/metadata/BuiltInTypeAndFunctionNamespaceManager.java

1. Support for annotating functions with both constant stats and propagating source stats. 2. Added tests for the same. 3. Added Scalar stats calculation based on annotation and tests for the same. Not added SQLInvokedScalarFunctions. Not annotated builtin functions, as that is covered in next implementation phase. Not added C++ changes as this phase only covers Java side of changes. Added documentation for the new properties and ... 1. Previously, if any of the source stats were missing, we would still compute the max/min/sum of argument stats etc.. now we propagate NaNs if any one of the arguments' stats are missing. 2. For distinct values count, upper bounding it to row count is as good as unknown. Therefore, the approach here is, when distinctValuesCount is greater than row count and is provided via annotation we set it to unknown. A function developer has full control here, for example developer can choose to upper bound or not by selecting the appropriate StatsPropagationBehavior value. 3. For average row size, a) If average row size is provided via ScalarFunctionConstantStats annotation, then we allow even if the size is greater than functions return type width. b) If average row size is provided via one of the StatsPropagationBehavior values, then we upper bound it to functions return type width - if available. If both (a) and (b) is unknown, then we default it to functions return type width if available. This way the function developer has greater control. Added new behaviour SUM_ARGUMENTS_UPPER_BOUND_ROW_COUNT which would upper bound the values to row count, so that summing distinct values count not exceed row counts.

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch from 1504ced to c358d95 Compare August 29, 2024 12:09

ScrapCodes marked this pull request as ready for review August 29, 2024 13:11

ScrapCodes requested review from elharo, a team, jaystarshot, feilong-liu and ClarenceThreepwood as code owners August 29, 2024 13:11

ScrapCodes requested a review from presto-oss August 29, 2024 13:11

aaneja self-requested a review August 29, 2024 16:03

ScrapCodes changed the title ~~Phase 1. Implementation for RFC-0005: Scalar function stats propagation.~~ RFC-0005. Implementation Scalar function stats propagation, Phase 1 Aug 30, 2024

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch 2 times, most recently from de5982e to 49a20e0 Compare August 30, 2024 08:59

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch from 49a20e0 to 495734f Compare September 6, 2024 11:14

ZacBlanco self-requested a review September 11, 2024 20:06

ZacBlanco reviewed Sep 11, 2024

View reviewed changes

ScrapCodes requested a review from ZacBlanco September 18, 2024 06:27

ScrapCodes requested a review from steveburnett as a code owner September 18, 2024 13:04

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch from babde3b to 2ee5cd4 Compare September 18, 2024 13:22

steveburnett requested changes Sep 18, 2024

View reviewed changes

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

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

steveburnett previously approved these changes Sep 19, 2024

View reviewed changes

ScrapCodes marked this pull request as draft September 20, 2024 02:15

ScrapCodes dismissed steveburnett’s stale review via f5d3275 September 23, 2024 11:13

ScrapCodes marked this pull request as ready for review September 23, 2024 11:15

steveburnett requested changes Sep 23, 2024

View reviewed changes

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

steveburnett previously approved these changes Sep 23, 2024

View reviewed changes

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch 2 times, most recently from 9e1a403 to ad6e77d Compare October 3, 2024 10:12

elharo reviewed Oct 3, 2024

View reviewed changes

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

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

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch from ad6e77d to a9f04a8 Compare October 4, 2024 10:07

elharo requested changes Oct 4, 2024

View reviewed changes

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

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch from a9f04a8 to 99302d3 Compare October 10, 2024 08:34

ZacBlanco reviewed Oct 14, 2024

View reviewed changes

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch from 99302d3 to 9d77026 Compare October 15, 2024 10:03

ScrapCodes requested review from elharo and ZacBlanco October 15, 2024 10:15

ZacBlanco reviewed Oct 15, 2024

View reviewed changes

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

ScrapCodes requested a review from ZacBlanco October 16, 2024 00:23

elharo requested changes Oct 16, 2024

View reviewed changes

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

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

ZacBlanco previously approved these changes Oct 16, 2024

View reviewed changes

ScrapCodes requested a review from elharo October 17, 2024 05:05

ScrapCodes dismissed ZacBlanco’s stale review via e248c01 October 22, 2024 10:11

aaneja requested a review from rschlussel November 5, 2024 10:50

ZacBlanco previously approved these changes Nov 8, 2024

View reviewed changes

aaneja previously approved these changes Nov 11, 2024

View reviewed changes

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

presto-main/src/test/java/com/facebook/presto/cost/TestScalarStatsCalculator.java Show resolved Hide resolved

ScrapCodes dismissed stale reviews from aaneja and ZacBlanco via 40b6f47 November 20, 2024 07:07

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch from e248c01 to 40b6f47 Compare November 20, 2024 07:07

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch from 9551c16 to 7693879 Compare December 13, 2024 09:09

aaneja requested changes Dec 16, 2024

View reviewed changes

ScrapCodes force-pushed the RFC_5_UDF_STATS_PHASE_1 branch from c334fdb to 07d2d0c Compare December 28, 2024 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC-0005. Implementation Scalar function stats propagation, Phase 1 #23545

RFC-0005. Implementation Scalar function stats propagation, Phase 1 #23545

ScrapCodes commented Aug 29, 2024 •

edited

Loading

steveburnett commented Aug 29, 2024

ScrapCodes commented Aug 30, 2024

ScrapCodes commented Sep 11, 2024

steveburnett commented Sep 11, 2024

steveburnett left a comment

steveburnett left a comment

steveburnett left a comment

steveburnett left a comment

steveburnett left a comment

ZacBlanco left a comment

ScrapCodes commented Oct 16, 2024

elharo left a comment

ScrapCodes commented Oct 17, 2024

elharo commented Oct 17, 2024

ScrapCodes commented Nov 20, 2024

RFC-0005. Implementation Scalar function stats propagation, Phase 1 #23545

Are you sure you want to change the base?

RFC-0005. Implementation Scalar function stats propagation, Phase 1 #23545

Conversation

ScrapCodes commented Aug 29, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

steveburnett commented Aug 29, 2024

ScrapCodes commented Aug 30, 2024

ScrapCodes commented Sep 11, 2024

steveburnett commented Sep 11, 2024

steveburnett 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

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

ZacBlanco left a comment

Choose a reason for hiding this comment

ScrapCodes commented Oct 16, 2024

elharo left a comment

Choose a reason for hiding this comment

ScrapCodes commented Oct 17, 2024

elharo commented Oct 17, 2024

ScrapCodes commented Nov 20, 2024

ScrapCodes commented Aug 29, 2024 •

edited

Loading