Add CTE Materialization documentation #22682

jaystarshot · 2024-05-07T01:15:52Z

Description

Fixes #22648

Motivation and Context

Impact

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
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

jaystarshot · 2024-05-07T01:16:40Z

cc: @feilong-liu @mlyublena @tdcmeehan @rschlussel for any suggestions

elharo · 2024-05-07T10:50:38Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+CTE Materialization
+=================================
+
+CTEs (Common Table Expressions) are defined as subqueries that appear in a WITH clause provided by the user.


Common Table Expressions (CTEs)

elharo · 2024-05-07T10:50:56Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+=================================
+
+CTEs (Common Table Expressions) are defined as subqueries that appear in a WITH clause provided by the user.
+Their repeated usage within a query can lead in unnecessary redundant computations, excessive data retrieval, and high resource consumption


lead to
period after consumption

elharo · 2024-05-07T10:51:36Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+CTEs (Common Table Expressions) are defined as subqueries that appear in a WITH clause provided by the user.
+Their repeated usage within a query can lead in unnecessary redundant computations, excessive data retrieval, and high resource consumption
+
+To address this, Presto supports CTE Materialization. This feature allows intermediate CTEs to be materialized and effectively stored


"materialized and effectively stored
and reused within" --> materialized, stored, and reused within

or maybe just "reused"

elharo · 2024-05-07T10:51:54Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+and reused within the scope of the same query.
+
+This improves performance and is especially beneficial when the same CTE is utilized multiple times in a query.
+These CTEs will be stored in temporary tables that are bucketed based on the first projection column


elharo · 2024-05-07T10:52:04Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+This improves performance and is especially beneficial when the same CTE is utilized multiple times in a query.
+These CTEs will be stored in temporary tables that are bucketed based on the first projection column
+
+To use this feature the connector used by the query must support the creation of temporary tables. Currently, only the :doc:`/connector/hive` connector offers this capability.


comma after feature

elharo · 2024-05-07T10:52:55Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+How to use CTE Materialization
+--------------
+
+The following configurations and session properties are provided to enable CTE materialization and fine-tune its settings.


delete "are provided to"

elharo · 2024-05-07T10:53:09Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+    * **Allowed values:** ``ALL``, ``NONE``, ``HEURISTIC`, ``HEURISTIC_COMPLEX_QUERIES_ONLY``
+    * **Default value:** ``NONE``
+
+    Specifies the strategy to use for materializing Common Table Expressions (CTEs) in queries.


delete "to use"

elharo · 2024-05-07T10:53:32Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+    * **Default value:** ``NONE``
+
+    Specifies the strategy to use for materializing Common Table Expressions (CTEs) in queries.
+    ``NONE`` indicates that no CTEs will be materialized.


delete "indicates that"

elharo · 2024-05-07T10:53:36Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+
+    Specifies the strategy to use for materializing Common Table Expressions (CTEs) in queries.
+    ``NONE`` indicates that no CTEs will be materialized.
+    ``ALL`` indicates that all CTEs in the query will be materialized.


delete "indicates that"

elharo · 2024-05-07T10:54:00Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+    * **Minimum value:** ``0``
+    * **Default value:** ``4``
+
+    Used with CTE Materialization Strategy is set to ``HEURISTIC` or ``HEURISTIC_COMPLEX_QUERIES_ONLY``. CTES are only materialized if they are used greater than or equal to this number


I don't understand this sentence. Please rewrite

Maybe something like:

Suggested change

Used with CTE Materialization Strategy is set to ``HEURISTIC` or ``HEURISTIC_COMPLEX_QUERIES_ONLY``. CTES are only materialized if they are used greater than or equal to this number

When ``cte-materialization-strategy`` is set to ``HEURISTIC`` or ``HEURISTIC_COMPLEX_QUERIES_ONLY``, then CTEs will be materialized if they are appear in a query at least``cte-heuristic-replication-threshold`` number of times

rschlussel · 2024-05-07T13:50:10Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+    * **Minimum value:** ``0``
+    * **Default value:** ``4``
+
+    Used with CTE Materialization Strategy is set to ``HEURISTIC` or ``HEURISTIC_COMPLEX_QUERIES_ONLY``. CTES are only materialized if they are used greater than or equal to this number


Maybe something like:

Suggested change

Used with CTE Materialization Strategy is set to ``HEURISTIC` or ``HEURISTIC_COMPLEX_QUERIES_ONLY``. CTES are only materialized if they are used greater than or equal to this number

When ``cte-materialization-strategy`` is set to ``HEURISTIC`` or ``HEURISTIC_COMPLEX_QUERIES_ONLY``, then CTEs will be materialized if they are appear in a query at least``cte-heuristic-replication-threshold`` number of times

rschlussel · 2024-05-07T16:41:44Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+To address this, Presto supports CTE Materialization. This feature allows intermediate CTEs to be materialized and effectively stored
+and reused within the scope of the same query.
+
+This improves performance and is especially beneficial when the same CTE is utilized multiple times in a query.


Suggested change

This improves performance and is especially beneficial when the same CTE is utilized multiple times in a query.

Materializing CTEs can improve performance when the same CTE is used multiple times in a query by reducing recomputation of the CTE. However, there is also a cost to writing to and reading from disk, so the optimization may not be beneficial for very simple CTEs or CTEs that are not used many times in a query.

rschlussel · 2024-05-07T16:46:38Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+and reused within the scope of the same query.
+
+This improves performance and is especially beneficial when the same CTE is utilized multiple times in a query.
+These CTEs will be stored in temporary tables that are bucketed based on the first projection column


Suggested change

These CTEs will be stored in temporary tables that are bucketed based on the first projection column

Materialized CTEs are stored in temporary tables that are bucketed based on the first projection column.

steveburnett

Nice work, thanks for the doc! I made a lot of suggestions that are mostly repetitive. I had a few comments that I ask for your thoughts.

Also, I'm planning to reformat the Properties Reference page to remove the indents for content under each header.
Instead of cluttering this review with even more comments, I'll ask you here to remove the indents for the content of the property entries under the headings.

Here's a screenshot of my local build where I removed the indents for cte-materialization-strategy, showing the format change in the build - removing the gray vertical line - from that entry, but leaving the entry for cte-heuristic-replication-threshold to show what I'm asking you to do. Let me know if you have any questions.

presto-docs/src/main/sphinx/admin/cte-materialization.rst

aditi-pandit

Thanks @jaystarshot for this doc. Have some comments.

aditi-pandit · 2024-05-08T19:12:34Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+    Specifies the strategy to use for materializing Common Table Expressions (CTEs) in queries.
+    ``NONE`` indicates that no CTEs will be materialized.
+    ``ALL`` indicates that all CTEs in the query will be materialized.
+    ``HEURISTIC` greedily materializes the earliest parent CTE which is repeated >= cte_heuristic_replication_threshold times


@jaystarshot : Do you intend to have a blog article or some doc giving more examples (queries and plans) demonstrating the behavior of these config ?

Yes that is a good idea, i can add it as a blog or https://github.com/prestodb/presto/wiki

aditi-pandit · 2024-05-08T19:14:28Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+
+    The number of partitions to be used for materializing Common Table Expressions (CTEs) in queries.
+    This setting determines how many buckets or writers should be used when materializing the CTEs, potentially affecting the performance of queries involving CTE materialization.
+    A higher number of partitions might improve parallelism but also increases overhead in terms of memory and network communication.


Its strange that a property called partitioning actually affects the buckets. But please emphasize in the explanation that these are buckets and not partitions.

This actually comes from exchange materialization where we use the hash partition count to count the number of buckets

Thanks for explaining! I thought this was strange, but the explanation makes sense.

aditi-pandit · 2024-05-08T19:15:22Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+    This setting specifies which catalog should be used for CTE materialization and for determining how to partition the materialization of CTEs in queries.
+    This can also be specified on a per-query basis using the ``cte_partitioning_provider_catalog`` session property.
+
+``cte-filter-and-projection-pushdown-enabled``


Would be great to have an example of this as well. Do you have something in a design doc ?

#21638 There is a good example on this ticket

aditi-pandit · 2024-05-08T19:16:05Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+    * **Allowed values:** ``HIVE_COMPATIBLE``, ``PRESTO_NATIVE``
+    * **Default value:** ``PRESTO_NATIVE``
+
+    This setting specifies the Hash function type for cte materialization


What do each of these settings mean ? Can you elaborate ?

cb69e89
Sample commit which added them.

Its used

presto/presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

Line 2968 in 785ece6

switch (bucketFunctionType) {

I think it determines which hashing functions to use when inserting records into the buckets.

feilong-liu · 2024-05-08T21:12:56Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+    ``NONE`` indicates that no CTEs will be materialized.
+    ``ALL`` indicates that all CTEs in the query will be materialized.
+    ``HEURISTIC` greedily materializes the earliest parent CTE which is repeated >= cte_heuristic_replication_threshold times
+    ``HEURISTIC_COMPLEX_QUERIES_ONLY` greedily materializes the earliest parent CTE which meets the heuristic criteria and has a join or aggregate


We may consider adding other operations, for example window operator which is also expensive for this strategy. But of course, this will be a separate PR from this one.

feilong-liu · 2024-05-08T21:15:03Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+
+    * **Type:** ``string``
+    * **Allowed values:** ``PAGEFILE``, ``ORC``, ``DWRF``, ``ALPHA``, ``PARQUET``, ``AVRO``, ``RCBINARY``, ``RCTEXT``, ``SEQUENCEFILE``, ``JSON``, ``TEXTFILE``, ``CSV``
+    * **Default value:** ``ORC``


Wondering if we want to change the default value if the recommendation is another one (again on a separate PR)

Yeah we can I will do it in a follow up PR. This is also used by exchange materialization and I did not want to break any default flow incase anyone was using the default value. Also prestissimo doesn't support PAGEFILE for now

steveburnett

Nice! A couple of nits for formatting issues, but from a docs standpoint it looks good. Happy to take another look when you're done.

presto-docs/src/main/sphinx/admin/cte-materialization.rst

jaystarshot · 2024-05-13T18:24:10Z

Thanks for the review

steveburnett

Thanks for the quick fix! In addition to the one comment about the value descriptions in cte-materialization-strategy, I have a request that would be a repetitive set of changes and is simpler to make here.

Suggest removing the indent throughout to remove the vertical stripe on the left. I'm trying to get to this throughout the Presto doc and it's taking time, but your new doc following this behavior would help.

Screenshots of the first one and a half entries (before, and after - the after also includes my suggested formatting for the first item's values) from my local environment to show what I mean.

(The second entry is unchanged in after, only cte-materialization-strategy acts as an example.)

before

after

presto-docs/src/main/sphinx/admin/cte-materialization.rst

steveburnett

Nice work! Only a few nits.

presto-docs/src/main/sphinx/admin/cte-materialization.rst

steveburnett · 2024-05-15T20:41:42Z

presto-docs/src/main/sphinx/admin/cte-materialization.rst

+
+    The number of partitions to be used for materializing Common Table Expressions (CTEs) in queries.
+    This setting determines how many buckets or writers should be used when materializing the CTEs, potentially affecting the performance of queries involving CTE materialization.
+    A higher number of partitions might improve parallelism but also increases overhead in terms of memory and network communication.


Thanks for explaining! I thought this was strange, but the explanation makes sense.

presto-docs/src/main/sphinx/admin/cte-materialization.rst

steveburnett

LGTM! (docs)

Pulled updated branch, new local build of docs to review, everything looks good. Thanks!

jaystarshot marked this pull request as ready for review May 7, 2024 01:16

jaystarshot requested review from steveburnett and a team as code owners May 7, 2024 01:16

jaystarshot requested a review from presto-oss May 7, 2024 01:16

github-actions bot added the docs label May 7, 2024

jaystarshot force-pushed the cte-docs branch 2 times, most recently from 5d43a16 to ce3f455 Compare May 7, 2024 01:40

elharo reviewed May 7, 2024

View reviewed changes

rschlussel reviewed May 7, 2024

View reviewed changes

steveburnett requested changes May 7, 2024

View reviewed changes

aditi-pandit reviewed May 8, 2024

View reviewed changes

feilong-liu previously approved these changes May 8, 2024

View reviewed changes

jaystarshot dismissed feilong-liu’s stale review via b5c5f2d May 11, 2024 01:11

jaystarshot force-pushed the cte-docs branch 4 times, most recently from b284122 to 55ed042 Compare May 11, 2024 01:14

jaystarshot requested a review from steveburnett May 11, 2024 01:14

jaystarshot force-pushed the cte-docs branch from 55ed042 to 8240364 Compare May 11, 2024 04:06

steveburnett requested changes May 13, 2024

View reviewed changes

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

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

jaystarshot force-pushed the cte-docs branch from 8240364 to d38723e Compare May 13, 2024 18:23

jaystarshot requested a review from steveburnett May 13, 2024 18:35

steveburnett requested changes May 13, 2024

View reviewed changes

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

jaystarshot force-pushed the cte-docs branch from d38723e to 3560fcf Compare May 15, 2024 19:01

jaystarshot requested a review from steveburnett May 15, 2024 19:01

jaystarshot force-pushed the cte-docs branch 2 times, most recently from 6ecdd80 to 37daaaa Compare May 15, 2024 19:39

steveburnett requested changes May 15, 2024

View reviewed changes

jaystarshot force-pushed the cte-docs branch 2 times, most recently from 45d061c to e04ddb4 Compare May 15, 2024 20:56

Add CTE Materialization documentation

b64604e

jaystarshot force-pushed the cte-docs branch from e04ddb4 to b64604e Compare May 15, 2024 20:57

steveburnett approved these changes May 15, 2024

View reviewed changes

jaystarshot merged commit 2aa4d42 into prestodb:master May 15, 2024
57 checks passed

	Used with CTE Materialization Strategy is set to ``HEURISTIC` or ``HEURISTIC_COMPLEX_QUERIES_ONLY``. CTES are only materialized if they are used greater than or equal to this number
	When ``cte-materialization-strategy`` is set to ``HEURISTIC`` or ``HEURISTIC_COMPLEX_QUERIES_ONLY``, then CTEs will be materialized if they are appear in a query at least``cte-heuristic-replication-threshold`` number of times

	This improves performance and is especially beneficial when the same CTE is utilized multiple times in a query.
	Materializing CTEs can improve performance when the same CTE is used multiple times in a query by reducing recomputation of the CTE. However, there is also a cost to writing to and reading from disk, so the optimization may not be beneficial for very simple CTEs or CTEs that are not used many times in a query.

	These CTEs will be stored in temporary tables that are bucketed based on the first projection column
	Materialized CTEs are stored in temporary tables that are bucketed based on the first projection column.

Add CTE Materialization documentation #22682

Add CTE Materialization documentation #22682

Conversation

jaystarshot commented May 7, 2024

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

jaystarshot commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

jaystarshot commented May 13, 2024

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment