Skip to content

[Iceberg] Cache invalidation procedure for Iceberg coordinator cache#24831

Merged
yingsu00 merged 2 commits into
prestodb:masterfrom
agrawalreetika:procedure_add
Apr 3, 2025
Merged

[Iceberg] Cache invalidation procedure for Iceberg coordinator cache#24831
yingsu00 merged 2 commits into
prestodb:masterfrom
agrawalreetika:procedure_add

Conversation

@agrawalreetika

@agrawalreetika agrawalreetika commented Mar 31, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Cache invalidation procedure for Iceberg coordinator cache

Motivation and Context

Cache invalidation procedure for Iceberg coordinator cache.

The rationale for enabling this procedure is:

  • To provide a mechanism for on-demand cache invalidation, allowing memory to be freed up on the coordinator.

  • To facilitate detailed query execution analysis when there are no plan changes. This is particularly useful for single-query cold/warm analysis, where:
    Cold: The state where caches are not populated.
    Warm: The state where caches are already populated, and the query primarily runs in memory.

  • To accurately measure query performance, we need a way to clear all caches before executing the next query. Since there is currently no built-in method to invalidate the coordinator cache, this PR introduces a procedure to enable cache cleanup.

Impact

Enable new procedure -

presto> CALL <catalog-name>.system.invalidate_statistics_file_cache();

presto> CALL <catalog-name>.system.invalidate_manifest_file_cache();

Test Plan

Test added

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • 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 ==

Iceberg Connector Changes
* Add support for the procedure <catalog-name>.system.invalidate_statistics_file_cache() for StatisticsFile cache invalidation in Iceberg.
* Add support for the procedure <catalog-name>.system.invalidate_manifest_file_cache() for ManifestFile cache invalidation in Iceberg.

@agrawalreetika agrawalreetika requested a review from yingsu00 March 31, 2025 07:38
@agrawalreetika agrawalreetika self-assigned this Mar 31, 2025
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Mar 31, 2025
@prestodb-ci prestodb-ci requested review from a team, infvg and sh-shamsan and removed request for a team March 31, 2025 07:38
steveburnett
steveburnett previously approved these changes Mar 31, 2025
View reviewed changes

@steveburnett steveburnett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local doc build. Thanks for the doc!

@steveburnett

steveburnett commented Mar 31, 2025

Copy link
Copy Markdown
Contributor

Thanks for the release note! Nit of phrasing suggested to follow the Order of changes in the Release Notes Guidelines:

== RELEASE NOTES ==

Iceberg Connector Changes
* Add support for procedure for StatisticsFile cache invalidation in Iceberg.
* Add support for procedure for ManifestFile cache invalidation in Iceberg.

ZacBlanco
ZacBlanco requested changes Apr 1, 2025
View reviewed changes

@ZacBlanco ZacBlanco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

mostly nits. Approach seems reasonable

Comment thread presto-docs/src/main/sphinx/connector/iceberg.rst Outdated
Comment thread presto-docs/src/main/sphinx/connector/iceberg.rst Outdated
Comment thread presto-docs/src/main/sphinx/connector/iceberg.rst Outdated
Comment thread presto-docs/src/main/sphinx/connector/iceberg.rst Outdated
Comment thread presto-iceberg/src/main/java/com/facebook/presto/iceberg/ManifestFileCache.java Outdated
Comment thread ...a/com/facebook/presto/iceberg/procedure/IcebergStatisticsFileCacheInvalidationProcedure.java Outdated
Comment thread presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java Outdated
Comment thread presto-iceberg/src/main/java/com/facebook/presto/iceberg/statistics/StatisticsFileCache.java Outdated
Comment thread ...a/com/facebook/presto/iceberg/procedure/IcebergStatisticsFileCacheInvalidationProcedure.java Outdated
Comment thread ...ava/com/facebook/presto/iceberg/procedure/IcebergManifestFileCacheInvalidationProcedure.java Outdated
hantangwangd
hantangwangd requested changes Apr 1, 2025
View reviewed changes

@hantangwangd hantangwangd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change looks good to me. Only some little nits.

Comment thread presto-docs/src/main/sphinx/connector/iceberg.rst Outdated
Statistics file cache invalidation procedure
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

* Flush Statistics file cache ::

@hantangwangd hantangwangd Apr 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* Flush Statistics file cache ::
* Invalidate Statistics file cache ::

nit: miss modifying this?

Comment thread ...ava/com/facebook/presto/iceberg/procedure/IcebergManifestFileCacheInvalidationProcedure.java Outdated
import static com.facebook.presto.common.block.MethodHandleUtil.methodHandle;
import static java.util.Objects.requireNonNull;

public class IcebergManifestFileCacheInvalidationProcedure

@hantangwangd hantangwangd Apr 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Do you think it makes sense to remove Iceberg from the class name? Refer to other procedures which are implemented and registered in Iceberg as well.

Comment thread ...ava/com/facebook/presto/iceberg/procedure/IcebergManifestFileCacheInvalidationProcedure.java Outdated
CACHE_DATA_INVALIDATION.bindTo(this));
}

public void icebergManifestFileCacheInvalidation()

@hantangwangd hantangwangd Apr 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Should we remove iceberg from the method name?

Comment thread ...a/com/facebook/presto/iceberg/procedure/IcebergStatisticsFileCacheInvalidationProcedure.java Outdated
import static com.facebook.presto.common.block.MethodHandleUtil.methodHandle;
import static java.util.Objects.requireNonNull;

public class IcebergStatisticsFileCacheInvalidationProcedure

@hantangwangd hantangwangd Apr 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The same as above.

Comment thread ...a/com/facebook/presto/iceberg/procedure/IcebergStatisticsFileCacheInvalidationProcedure.java Outdated
CACHE_DATA_INVALIDATION.bindTo(this));
}

public void icebergStatisticsFileCacheInvalidation()

@hantangwangd hantangwangd Apr 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The same as above.

@agrawalreetika agrawalreetika force-pushed the procedure_add branch 4 times, most recently from a1dfa83 to c5bc14f Compare April 1, 2025 18:15
@agrawalreetika agrawalreetika added the iceberg Apache Iceberg related label Apr 1, 2025
@prestodb-ci prestodb-ci requested a review from a team April 1, 2025 18:30
@github-project-automation github-project-automation Bot moved this to 🆕 Unprioritized in Iceberg Support Apr 1, 2025
@agrawalreetika

agrawalreetika commented Apr 1, 2025

Copy link
Copy Markdown
Member Author

@ZacBlanco @hantangwangd Thanks for your review, addressed your comments. Please take a look when you get a chance.

@github-project-automation github-project-automation Bot moved this from 🆕 Unprioritized to ✅ Done in Iceberg Support Apr 2, 2025
yingsu00
yingsu00 reviewed Apr 2, 2025
View reviewed changes

@yingsu00 yingsu00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you Reetika! Would you please include the procedure names in the release notes?

@agrawalreetika

agrawalreetika commented Apr 2, 2025

Copy link
Copy Markdown
Member Author

Thank you Reetika! Would you please include the procedure names in the release notes?

@yingsu00 Done

@yingsu00 yingsu00 merged commit d16d9d7 into prestodb:master Apr 3, 2025
@agrawalreetika agrawalreetika deleted the procedure_add branch April 3, 2025 05:11
@ZacBlanco ZacBlanco mentioned this pull request May 29, 2025
Merged
21 tasks
This was referenced May 30, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZacBlanco ZacBlanco ZacBlanco approved these changes

@hantangwangd hantangwangd hantangwangd approved these changes

@yingsu00 yingsu00 yingsu00 approved these changes

@steveburnett steveburnett steveburnett left review comments

@elharo elharo Awaiting requested review from elharo elharo is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@infvg infvg Awaiting requested review from infvg infvg was automatically assigned from prestodb/ibm-reviewers

@sh-shamsan sh-shamsan Awaiting requested review from sh-shamsan sh-shamsan was automatically assigned from prestodb/ibm-reviewers

Assignees

@agrawalreetika agrawalreetika

Labels

from:IBM PR from IBM iceberg Apache Iceberg related

Projects

Iceberg Support
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@agrawalreetika @steveburnett @ZacBlanco @hantangwangd @yingsu00 @prestodb-ci