[WIP] feature: Make filesystem cache table aware #27277

RatulDawar · 2025-11-11T19:30:48Z

Related Issue : #27276

cla-bot · 2025-11-11T19:30:51Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-11-11T19:47:44Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-11-11T19:54:04Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-11-11T20:13:51Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

RotRotAl

Hi please fix your code (5 test shave failed), and submit signed cla as the bot commented.

RotRotAl · 2025-11-12T09:32:58Z

lib/trino-filesystem-manager/src/main/java/io/trino/filesystem/manager/FileSystemConfig.java

+    }
+
+    @Config("fs.cache.include-tables")
+    @ConfigDescription("List of tables to include in file system cache (schema.table format, supports wildcards like schema.* or *)")


The wording is a bit off,

Suggested change

@ConfigDescription("List of tables to include in file system cache (schema.table format, supports wildcards like schema.* or *)")

@ConfigDescription("List of tables to include in file system cache,

use * to cache listings for all tables in all schemas

RotRotAl · 2025-11-12T09:33:41Z

...rino-filesystem-manager/src/main/java/io/trino/filesystem/manager/TableCachingPredicate.java

+
+/**
+ * Predicate to determine if a table should be cached based on configured include list.
+ * Supports wildcards: schema.table, schema.*, or *


use * to cache listings for all tables in all schemas

lib/trino-filesystem/src/main/java/io/trino/filesystem/cache/CacheFileSystem.java

RotRotAl · 2025-11-12T10:02:20Z

@RatulDawar have you tried adding this feature to hive connector as well? it will be very helpful

ratuldawar11 · 2025-11-12T11:29:21Z

Hi please fix your code (5 test shave failed), and submit signed cla as the bot commented.

Yes have already submitted cla 3-4 days back. Also still working on this PR. It's a WIP.

ratuldawar11 · 2025-11-12T11:29:41Z

@RatulDawar have you tried adding this feature to hive connector as well? it will be very helpful

Sure will add that too in the same PR.

RotRotAl · 2025-11-26T16:17:33Z

Hi is there something new?

RatulDawar · 2025-11-26T17:58:21Z

Hi is there something new?

Hey resolving the comments in fews hours, was waiting for CLA to be signed, it just got signed today.

lib/trino-filesystem-manager/src/main/java/io/trino/filesystem/manager/FileSystemConfig.java

findepi · 2025-11-27T11:06:00Z

...rino-filesystem-manager/src/main/java/io/trino/filesystem/manager/TableCachingPredicate.java

+        if (includeTables.isEmpty()) {
+            this.predicate = _ -> true;


findepi · 2025-11-27T11:07:24Z

...rino-filesystem-manager/src/main/java/io/trino/filesystem/manager/TableCachingPredicate.java

+        return predicate.test(table);
+    }
+
+    private static Predicate<SchemaTableName> matches(List<String> tables)


matches -> buildPredicate

findepi · 2025-11-27T11:09:26Z

...rino-filesystem-manager/src/main/java/io/trino/filesystem/manager/TableCachingPredicate.java

+                .map(prefix -> (Predicate<SchemaTableName>) prefix::matches)
+                .reduce(Predicate::or)


Looks fancy, but it would be better to just collect parsed table names on a field
and implement predicate with explicit for loop over them.

also, if any entry is *, then other elements still should be parsed (validated), but don't need to be remembered

findepi · 2025-11-27T11:18:23Z

lib/trino-filesystem/src/main/java/io/trino/filesystem/TrinoFileSystemFactory.java

+    default TrinoFileSystem create(ConnectorSession session, boolean cachingEnabled)
+    {
+        return create(session);
+    }
+
+    default TrinoFileSystem create(ConnectorIdentity identity, boolean cachingEnabled)
+    {
+        return create(identity);
+    }


i generally like the idea that the FS calling code can knowingly bypass caching layer. Today this is handled by connector's CacheKeyProvider having knowledge of other components internals, eg

trino/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/cache/DeltaLakeCacheKeyProvider.java

Lines 52 to 54 in 99b456a

if (path.endsWith(".trinoSchema") || path.contains("/.trinoPermissions/")) {

// Needed to avoid caching files from FileHiveMetastore on coordinator during tests

return Optional.empty();

trino/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/cache/IcebergCacheKeyProvider.java

Lines 28 to 31 in 2cb497d

if (path.endsWith(".trinoSchema") || path.contains("/.trinoPermissions/")) {

// Needed to avoid caching files from FileHiveMetastore on coordinator during tests

return Optional.empty();

}

I am not sure these new functions are the best way to do it.
at least for FileMetastore i would try to simply inject the uncached underlying FS via Guice, or maybe have it configurable for FileMetastore separately (eg read-only use-cases on static data sets are OK to cache)

cc @electrum @raunaqmorarka

if we want to go this direction with new methods in TFSF, please open a separate PR adding those methods.
We can add usage of this methods in FileMetastore (controlled by configuration). This will be a simpler prep change & will ensure we have agreement on the API.

@findepi I think these API's make this bypassing more robust both the Guice binding and api's with caching flag seems better approches than the current cache key provider with context internal file paths.
I would prefer API changes given that it is more flexible for possible future requirements. Open to discussion on this... !

findepi · 2025-11-27T11:19:08Z

.../src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogFileSystemFactory.java

+        if (vendedCredentialsEnabled &&
+                fileIoProperties.containsKey(VENDED_S3_ACCESS_KEY) &&
+                fileIoProperties.containsKey(VENDED_S3_SECRET_KEY) &&
+                fileIoProperties.containsKey(VENDED_S3_SESSION_TOKEN)) {
+            // Do not include original credentials as they should not be used in vended mode
+            ConnectorIdentity identityWithExtraCredentials = ConnectorIdentity.forUser(identity.getUser())
+                    .withGroups(identity.getGroups())
+                    .withPrincipal(identity.getPrincipal())
+                    .withEnabledSystemRoles(identity.getEnabledSystemRoles())
+                    .withConnectorRole(identity.getConnectorRole())
+                    .withExtraCredentials(ImmutableMap.<String, String>builder()
+                            .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, fileIoProperties.get(VENDED_S3_ACCESS_KEY))
+                            .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, fileIoProperties.get(VENDED_S3_SECRET_KEY))
+                            .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, fileIoProperties.get(VENDED_S3_SESSION_TOKEN))
+                            .buildOrThrow())
+                    .build();


That looks like a copy of the method above. Avoid copying (duplicating) this logic.

findepi · 2025-11-27T11:20:32Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileSystemFactory.java

+     * @param cachingEnabled whether file system caching should be enabled
+     * @return a TrinoFileSystem instance
+     */
+    default TrinoFileSystem create(ConnectorSession session, Map<String, String> fileIoProperties, boolean cachingEnabled)


the problem is that cachingEnabled = true does not imply there will be any caching
it depends what the backing TrinoFileSystemFactory is. i.e. caching needs to be allowed on the FS layer as well

Seems fair, I think then it's make it virtually impossible to use apis with caching flags.
Should we go guice dependencies ? That seems fine over handling this in cachekeyprovider.

findepi · 2025-11-27T11:23:13Z

...rino-filesystem-manager/src/main/java/io/trino/filesystem/manager/TableCachingPredicate.java

+    private final Predicate<SchemaTableName> predicate;
+
+    @Inject
+    public TableCachingPredicate(FileSystemConfig config)


This looks like part of trino-filesystem-manager caching layer, but it's used only in Iceberg.

For FS caching it would be more natural to cache based on locations, but i can see how user-unfriendly this might be and that cache control by table names is reasonable.

This currently is a connector-specific cache control and if implemented as such, it should be configured as such. see delta.fs.cache.disable-transaction-log-caching for example of pre-existing connector-specific cache control.

@findepi I think we can extended it for all importers of trino-file-system manager as this doesn't require any connector specific handling. Adding this specific to connectos seems to add more complexity than implementing it connector agonistic.

Let me confirm if this is possible or not

@notempty

- Change default value from empty list to ImmutableList.of('*') to maintain backward compatibility - Add @notempty validation to reject empty list configuration - Add test to verify empty list validation fails - Add jakarta.validation-api and io.airlift:testing dependencies to pom.xml

…itialization and improve readability

github-actions · 2025-12-19T17:04:16Z

This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack.

github-actions · 2026-01-09T17:06:04Z

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

RoeyoOgen · 2026-01-11T09:46:45Z

@RatulDawar any update?

RatulDawar · 2026-01-15T19:17:08Z

@RatulDawar any update?

I have addressed all the comments need to start a discussion on how the API's should be formed here, will add my pointers and do that in this week itself, will again start working on this actively

github-actions bot added the iceberg Iceberg connector label Nov 11, 2025

RatulDawar changed the title ~~feature: Make filesystem cache table aware~~ [WIP] feature: Make filesystem cache table aware Nov 11, 2025

RotRotAl suggested changes Nov 12, 2025

View reviewed changes

cla-bot bot added the cla-signed label Nov 26, 2025

RatulDawar force-pushed the feature/explicit-table-caching branch from 1ee85e3 to 6bb2852 Compare November 26, 2025 18:07

RatulDawar force-pushed the feature/explicit-table-caching branch from 6bb2852 to 5482baf Compare November 26, 2025 18:11

RatulDawar added 5 commits November 27, 2025 00:08

Make filesystem cache table aware

eb2cd16

Add dependency injection for FileSystemConfig in TableCachingPredicate

d4b9053

Add missing overrides

5d16e99

Address review comments: Update ConfigDescription and javadoc wording

dabe5d5

Update TestFileSystemConfig to include cacheIncludeTables property

feda670

RatulDawar force-pushed the feature/explicit-table-caching branch from 5482baf to feda670 Compare November 26, 2025 18:39

findepi reviewed Nov 27, 2025

View reviewed changes

RatulDawar added 2 commits November 27, 2025 23:37

Refactor TableCachingPredicate constructor to streamline predicate in…

5f1b7cd

…itialization and improve readability

github-actions bot added the stale label Dec 19, 2025

github-actions bot closed this Jan 9, 2026

RoeyoOgen reopened this Jan 11, 2026

github-actions bot removed the stale label Jan 12, 2026

	@ConfigDescription("List of tables to include in file system cache (schema.table format, supports wildcards like schema.* or *)")
	@ConfigDescription("List of tables to include in file system cache,
	use * to cache listings for all tables in all schemas

		.map(prefix -> (Predicate<SchemaTableName>) prefix::matches)
		.reduce(Predicate::or)

	if (path.endsWith(".trinoSchema") \|\| path.contains("/.trinoPermissions/")) {
	// Needed to avoid caching files from FileHiveMetastore on coordinator during tests
	return Optional.empty();

[WIP] feature: Make filesystem cache table aware #27277

Are you sure you want to change the base?

[WIP] feature: Make filesystem cache table aware #27277

Uh oh!

Conversation

RatulDawar commented Nov 11, 2025

Uh oh!

cla-bot bot commented Nov 11, 2025

Uh oh!

cla-bot bot commented Nov 11, 2025

Uh oh!

cla-bot bot commented Nov 11, 2025

Uh oh!

cla-bot bot commented Nov 11, 2025

Uh oh!

RotRotAl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RotRotAl commented Nov 12, 2025

Uh oh!

ratuldawar11 commented Nov 12, 2025

Uh oh!

ratuldawar11 commented Nov 12, 2025

Uh oh!

RotRotAl commented Nov 26, 2025

Uh oh!

RatulDawar commented Nov 26, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 19, 2025

Uh oh!

github-actions bot commented Jan 9, 2026

Uh oh!

RoeyoOgen commented Jan 11, 2026

Uh oh!

RatulDawar commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants