Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support fragment result caching #15155

Merged
merged 5 commits into from
Oct 1, 2020
Merged

Support fragment result caching #15155

merged 5 commits into from
Oct 1, 2020

Conversation

shixuan-fan
Copy link
Contributor

@shixuan-fan shixuan-fan commented Sep 10, 2020 •
edited
Loading

Test plan - Multiple unit test for CanonicalPlanGenerator, FileFragmentResultCacheManager and Driver. Ran shadow test for internal queries. Ran verifier test for internal queries.

== RELEASE NOTES ==

General Changes
* Add support for fragment result caching. When enabled, if the same plan fragment and same connector split hit the same worker, engine would directly fetch result from cache and skip computation. Currently only partial aggregation is supported. Cache could be enabled by setting ``fragment-result-cache.enabled`` to ``true`` and tuned by other configs started with ``fragment-result-cache``. 
 Query could use fragment result cache by setting config ``experimental.fragment-result-caching-enabled`` or session property ``fragment_result_caching_enabled`` to ``true``.

SPI changes
* Add ``getSplitIdentifier`` to ``ConnectorSplit``. Split identifier is used in fragment result caching to identify if splits are identical.
* Add ``getIdentifier`` to ``ConnectorTableLayoutHandle``. Layout identifier is used in fragment result caching to construct canonical plan.

@shixuan-fan shixuan-fan marked this pull request as draft September 10, 2020 01:05
highker
highker reviewed Sep 11, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both Driver & LocalExecutionPlanner look clean

presto-main/src/main/java/com/facebook/presto/operator/Driver.java Outdated Show resolved Hide resolved
@shixuan-fan shixuan-fan changed the title [WIP] Support fragment result caching Support fragment result caching Sep 23, 2020
@shixuan-fan shixuan-fan marked this pull request as ready for review September 23, 2020 23:04
@shixuan-fan shixuan-fan requested a review from a team September 24, 2020 17:05
highker
highker reviewed Sep 27, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Introduce CanonicalPlanGenerator": one comment

presto-hive/src/main/java/com/facebook/presto/hive/HiveTableLayoutHandle.java Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/CanonicalPartitioningScheme.java Show resolved Hide resolved
highker
highker reviewed Sep 27, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Introduce split identifier" LGTM

highker
highker reviewed Sep 27, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Introduce fragment result cache manager": minor comments

presto-main/src/main/java/com/facebook/presto/operator/CacheStats.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/FileFragmentResultCacheManager.java Outdated Show resolved Hide resolved
highker
highker reviewed Sep 27, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Use static import and lambda in TestDriver": LGTM

highker
highker reviewed Sep 27, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Support fragment result caching": overall logic looks good to me. Question on missing aggregation support

presto-main/src/main/java/com/facebook/presto/execution/FragmentResultCacheContext.java Outdated Show resolved Hide resolved
highker
highker approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@shixuan-fan shixuan-fan merged commit 9eaa655 into prestodb:master Oct 1, 2020
@shixuan-fan shixuan-fan deleted the frc branch October 1, 2020 16:41
This was referenced Oct 6, 2020
Merged
Closed
Closed
wenleix
wenleix reviewed Nov 6, 2020
View reviewed changes
presto-main/src/main/java/com/facebook/presto/operator/FileFragmentResultCacheManager.java
}
}

private static <T> Iterator<T> closeWhenExhausted(Iterator<T> iterator, Closeable resource)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part of code looks so similar to FileSingleStreamSpiller lol.

@tooptoop4 tooptoop4 mentioned this pull request Jun 20, 2021
Closed
@zhengxingmao
Copy link
Contributor

Will this feature have the same performance improvement when it is migrated to iceberg connector?
Are there plans to support iceberg connector?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgwang7w fgwang7w fgwang7w left review comments

@wenleix wenleix wenleix left review comments

@highker highker highker approved these changes

@sachdevs sachdevs Awaiting requested review from sachdevs

@jainxrohit jainxrohit Awaiting requested review from jainxrohit

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shixuan-fan @zhengxingmao @wenleix @highker @fgwang7w