Implement caching of iceberg metadata on coordinator by raunaqmorarka · Pull Request #22739 · trinodb/trino

raunaqmorarka · 2024-07-19T06:10:57Z

Description

This is intended to reduce filesystem accesses for metadata operations in iceberg without having to explicitly configure filesystem caching on local disks

Additional context and related issues

Pre-requisite to landing #22584 as that potentially increases filesystem accesses for planning queries with different columns projected from same table through multiple scans

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Iceberg
* Improve query planning performance. ({issue}`22739`)
  Iceberg metadata files are now cached on the coordinator, this can be disabled by setting the catalog configuration property `iceberg.metadata-cache.enabled` to `false`

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

jkylling

This will also cache data files if the coordinator does execution?

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

...rino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMemoryCacheFileOperations.java

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCache.java

...rino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMemoryCacheFileOperations.java

jkylling · 2024-07-22T15:54:05Z

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCacheConfig.java

nit: Default configuration for cache is not stable if Runtime.getRutime().maxMemory() < 2000 MB?

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCache.java

raunaqmorarka · 2024-07-23T08:08:13Z

iceberg parquet partitioned.pdf

iceberg parquet unpartitioned.pdf

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCacheConfig.java

lib/trino-filesystem/src/main/java/io/trino/filesystem/tracing/TracingInputFile.java

sopel39 · 2024-07-30T11:44:07Z

...lesystem-cache-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioFileSystemCache.java

How frequently cacheLength is called?

Seems like length could be important to cache. @jkylling could alluxio provide an implementation of this method?

In the context of iceberg metadata files, we need it only because SNAPSHOT file lengths are not known in advance and iceberg library uses file length to open it's avro reader.
Having it in the cache helps for subsequent queries.

From what I can tell the CacheManager interface we use from Alluxio does not have a way to get the file length.

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCacheModule.java

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

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

sopel39 · 2024-07-30T12:07:04Z

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

Maybe the javadoc should be changed noting that location could be a directory now. What's the semantics then?

There is no "semantics", it's a hint to the cache, it can do whatever with it.

I think the expectation is that it should evict cached data for all files in location, right?

it's not a strict requirement, but ideally files under directory should be deleted.
this was needed only because certain tests manipulate files and directories directly, it's not a problem for real workloads where direct manipulation is not allowed.

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

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCache.java

sopel39 · 2024-07-30T12:35:25Z

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCache.java

nit: prob should be default

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCache.java

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCacheConfig.java

sopel39 · 2024-07-30T15:23:09Z

#22739 (comment)

analysis time is not reported as planning time, so the gains are probably even better

Every implementation of TrinoInputFile caches lastModified. Therefore, calls to lastModified() should not be traced repeatedly.

lib/trino-filesystem/src/main/java/io/trino/filesystem/tracing/Tracing.java

electrum · 2024-07-31T05:15:08Z

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCache.java

It seems better to put these in trino-filesystem-manager module as that is where they are used.

This would require making MemoryInputStream and MemoryInput public and bring some dependencies into trino-filesystem-manager pom.xml
Doable but the current way seems simpler.
Let me know if you would still like to move these classes to trino-filesystem-manager module.

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

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCache.java

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

lib/trino-filesystem/src/test/java/io/trino/filesystem/memory/TestMemoryFileSystemCache.java

This is intended to reduce filesystem accesses for metadata operations in iceberg without having to explicitly configure filesystem caching on local disks

cla-bot bot added the cla-signed label Jul 19, 2024

raunaqmorarka mentioned this pull request Jul 19, 2024

Use projected columns in iceberg getTableStatistics #22584

Merged

raunaqmorarka requested review from ebyhr, electrum, findepi, findinpath, jkylling and martint and removed request for martint July 19, 2024 06:12

github-actions bot added iceberg Iceberg connector delta-lake Delta Lake connector labels Jul 19, 2024

raunaqmorarka force-pushed the small-files-cache branch from 04890a1 to c910b2d Compare July 19, 2024 07:23

github-actions bot added the hive Hive connector label Jul 19, 2024

raunaqmorarka added the performance label Jul 19, 2024

raunaqmorarka force-pushed the small-files-cache branch 2 times, most recently from f01974c to 3e7ebd4 Compare July 19, 2024 09:00

findinpath reviewed Jul 19, 2024

View reviewed changes

lib/trino-filesystem-manager/src/main/java/io/trino/filesystem/manager/FileSystemConfig.java Outdated Show resolved Hide resolved

jkylling reviewed Jul 19, 2024

View reviewed changes

raunaqmorarka force-pushed the small-files-cache branch from 3e7ebd4 to ff07326 Compare July 22, 2024 11:05

github-actions bot added the hudi Hudi connector label Jul 22, 2024

raunaqmorarka changed the title ~~Implement caching of small files on coordinator~~ Implement caching of iceberg metadata on coordinator Jul 22, 2024

raunaqmorarka force-pushed the small-files-cache branch from ff07326 to 9beb3e8 Compare July 22, 2024 11:07

raunaqmorarka requested review from findinpath and jkylling July 22, 2024 11:22

raunaqmorarka force-pushed the small-files-cache branch from 9beb3e8 to 445043b Compare July 22, 2024 11:23

jkylling reviewed Jul 22, 2024

View reviewed changes

raunaqmorarka mentioned this pull request Jul 23, 2024

Add manifest file caching support for Iceberg Hive Metastore catalog #22376

Closed

raunaqmorarka force-pushed the small-files-cache branch from 445043b to d8dcbfa Compare July 24, 2024 14:25

raunaqmorarka requested a review from jkylling July 24, 2024 14:27

wendigo reviewed Jul 30, 2024

View reviewed changes

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCacheConfig.java Outdated Show resolved Hide resolved

wendigo reviewed Jul 30, 2024

View reviewed changes

lib/trino-filesystem/src/main/java/io/trino/filesystem/memory/MemoryFileSystemCacheConfig.java Outdated Show resolved Hide resolved

wendigo approved these changes Jul 30, 2024

View reviewed changes

sopel39 reviewed Jul 30, 2024

View reviewed changes

lib/trino-filesystem/src/main/java/io/trino/filesystem/cache/CacheInputFile.java Outdated Show resolved Hide resolved

lib/trino-filesystem/src/main/java/io/trino/filesystem/cache/CacheInputFile.java Outdated Show resolved Hide resolved

sopel39 reviewed Jul 30, 2024

View reviewed changes

raunaqmorarka force-pushed the small-files-cache branch 2 times, most recently from 964eefc to 3712402 Compare July 30, 2024 14:53

Skip tracing when lastModified is cached

28dc176

Every implementation of TrinoInputFile caches lastModified. Therefore, calls to lastModified() should not be traced repeatedly.

raunaqmorarka force-pushed the small-files-cache branch from 3712402 to 891fe76 Compare July 31, 2024 03:06

electrum reviewed Jul 31, 2024

View reviewed changes

lib/trino-filesystem/src/main/java/io/trino/filesystem/tracing/Tracing.java Outdated Show resolved Hide resolved

electrum reviewed Jul 31, 2024

View reviewed changes

Make io.trino.filesystem.tracing.Tracing public for reuse

af680ab

raunaqmorarka force-pushed the small-files-cache branch from 891fe76 to 56bc045 Compare July 31, 2024 06:20

raunaqmorarka requested review from electrum and sopel39 July 31, 2024 06:24

sopel39 approved these changes Jul 31, 2024

View reviewed changes

jkylling approved these changes Jul 31, 2024

View reviewed changes

lib/trino-filesystem/src/test/java/io/trino/filesystem/memory/TestMemoryFileSystemCache.java Outdated Show resolved Hide resolved

raunaqmorarka force-pushed the small-files-cache branch from 56bc045 to 595c427 Compare July 31, 2024 18:16

Implement caching of iceberg metadata on coordinator

52291c6

This is intended to reduce filesystem accesses for metadata operations in iceberg without having to explicitly configure filesystem caching on local disks

raunaqmorarka force-pushed the small-files-cache branch from 595c427 to 52291c6 Compare August 1, 2024 06:33

raunaqmorarka merged commit 351920a into trinodb:master Aug 1, 2024

raunaqmorarka deleted the small-files-cache branch August 1, 2024 07:56

github-actions bot added this to the 454 milestone Aug 1, 2024

colebow mentioned this pull request Aug 7, 2024

Add Trino 454 release notes #22977

Merged

raunaqmorarka mentioned this pull request Sep 5, 2024

Iceberg Metadata Cache for faster query planning #13338

Closed

sopel39 mentioned this pull request Sep 20, 2024

Performance Issue with Retrieving Columns from Iceberg Catalog #23468

Closed

Conversation

raunaqmorarka commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

Uh oh!

jkylling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raunaqmorarka commented Jul 23, 2024

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sopel39 commented Jul 30, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raunaqmorarka Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

raunaqmorarka commented Jul 19, 2024 •

edited

Loading

raunaqmorarka Jul 31, 2024 •

edited

Loading