Avoid materializing all results in Iceberg `$files` system table at once #16112

krvikash · 2023-02-14T15:38:10Z

Description

The RR is an alternative approach to fix #15991. another approach #16063 to fix #15991

For iceberg.iceberg_small_files_tpcds_sf1000_parquet.store_sales$files earlier it used to take 826MB of memory (~32K records loaded) and after the change, it takes 32.3 MB (~72K records loaded). It also reduces the response to console time by 50%.

This example is when the table has ~227K files.

Before fix:

After fix:

Release notes

(X) Release notes are required, with the following suggested text:

# Iceberg
* Reduce memory usage when reading `$files` system tables. ({issue}`16112`)

krvikash · 2023-02-14T15:39:09Z

CC: @alexjo2144 | @findinpath

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

alexjo2144

This is definitely a better direction that the PageBuilder way

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

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

krvikash · 2023-02-14T19:42:38Z

Thanks @alexjo2144 | @findinpath for reviewing the PR.

This is definitely a better direction that the PageBuilder way

I hope we are going forward with this (RecordCursor) approach. I will make this as primary PR.

krvikash · 2023-02-15T11:19:10Z

Currently, we materialize all columns from the $files table irrespective of what column is asked in the query. Is it worth looking for if we can only materialize the column which is asked?

krvikash · 2023-02-15T13:51:36Z

Currently, we materialize all columns from the $files table irrespective of what column is asked in the query. Is it worth looking for if we can only materialize the column which is asked?

Like, If we query SELECT file_path from "store_sales$files"; then we collect all columns (My question is here why not just collect the column which is queried for, file_path in this case) from $files table and pass it to the caller. and the caller will display according to what field we have in the SELECT query.

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

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

krvikash · 2023-02-15T18:08:51Z

Addressed comments and rebased with master.

core/trino-spi/src/main/java/io/trino/spi/connector/InMemoryRecordSet.java

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

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

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

krvikash · 2023-02-16T04:30:12Z

Hi @ebyhr, When you get time, could you please take a look at this PR?

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

ebyhr

Just skimmed.

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

krvikash · 2023-02-16T06:38:00Z

Thanks, @ebyhr | @findinpath | @homar for the reviews. I have addressed the comments.

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

findepi · 2023-02-28T08:18:01Z

core/trino-spi/src/main/java/io/trino/spi/connector/InMemoryRecordSet.java

@@ -68,15 +68,15 @@ public RecordCursor cursor()
        return new InMemoryRecordCursor(types, records.iterator());
    }

-    private static class InMemoryRecordCursor
+    public static class InMemoryRecordCursor<T extends Iterator<? extends List<?>>>


The class became generic, but so there are now warnings wherever it's used as a raw class (eg 3 lines above).

BTW it seems the generics are not leveraged, since T shows up only in the constructor declaration.
#16295

cla-bot bot added the cla-signed label Feb 14, 2023

krvikash requested review from findinpath and alexjo2144 February 14, 2023 15:38

krvikash self-assigned this Feb 14, 2023

findinpath reviewed Feb 14, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java Show resolved Hide resolved

krvikash force-pushed the avoid-materializing-all-results-in-file-table-alternative branch from e81e50e to f1658ac Compare February 14, 2023 16:17

krvikash changed the title ~~[Alternative]: Avoid materializing all results in Iceberg system table at once~~ [Alternative]: Avoid materializing all results in Iceberg $files system table at once Feb 14, 2023

findinpath reviewed Feb 14, 2023

View reviewed changes

alexjo2144 reviewed Feb 14, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IncrementalInMemoryRecordCursor.java Outdated Show resolved Hide resolved

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java Outdated Show resolved Hide resolved

krvikash force-pushed the avoid-materializing-all-results-in-file-table-alternative branch from f1658ac to 441c747 Compare February 14, 2023 19:37

krvikash changed the title ~~[Alternative]: Avoid materializing all results in Iceberg $files system table at once~~ Avoid materializing all results in Iceberg $files system table at once Feb 14, 2023

krvikash marked this pull request as ready for review February 14, 2023 19:45

krvikash requested review from ebyhr and homar February 14, 2023 19:45

krvikash added the performance label Feb 14, 2023

krvikash mentioned this pull request Feb 15, 2023

[Do Not Review] : Avoid materializing all results in $files Iceberg system table at once #16063

Closed

findinpath reviewed Feb 15, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java Show resolved Hide resolved

findinpath reviewed Feb 15, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IncrementalInMemoryRecordCursor.java Outdated Show resolved Hide resolved

krvikash force-pushed the avoid-materializing-all-results-in-file-table-alternative branch from 441c747 to 7e02045 Compare February 15, 2023 18:04

homar approved these changes Feb 15, 2023

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/connector/InMemoryRecordSet.java Outdated Show resolved Hide resolved

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java Outdated Show resolved Hide resolved

findinpath reviewed Feb 15, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/CloseableInMemoryRecordCursor.java Outdated Show resolved Hide resolved

findinpath reviewed Feb 15, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/CloseableInMemoryRecordCursor.java Outdated Show resolved Hide resolved

findinpath reviewed Feb 15, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/CloseableInMemoryRecordCursor.java Outdated Show resolved Hide resolved

findinpath reviewed Feb 15, 2023

View reviewed changes

krvikash commented Feb 16, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java Outdated Show resolved Hide resolved

ebyhr reviewed Feb 16, 2023

View reviewed changes

krvikash force-pushed the avoid-materializing-all-results-in-file-table-alternative branch from 7e02045 to 7e1c6b1 Compare February 16, 2023 06:35

Make InMemoryRecordSet.InMemoryRecordCursor generic and public for reuse

950ad40

krvikash force-pushed the avoid-materializing-all-results-in-file-table-alternative branch from 7e1c6b1 to 71f693a Compare February 16, 2023 06:58

ebyhr reviewed Feb 16, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java Outdated Show resolved Hide resolved

findinpath reviewed Feb 16, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java Outdated Show resolved Hide resolved

Avoid materializing all results in Iceberg $files system table at once

b4b5d36

krvikash force-pushed the avoid-materializing-all-results-in-file-table-alternative branch from 71f693a to b4b5d36 Compare February 16, 2023 09:01

findinpath approved these changes Feb 16, 2023

View reviewed changes

ebyhr approved these changes Feb 17, 2023

View reviewed changes

ebyhr merged commit b8cbac2 into trinodb:master Feb 17, 2023

ebyhr mentioned this pull request Feb 17, 2023

Release notes for 408 #16147

Closed

krvikash deleted the avoid-materializing-all-results-in-file-table-alternative branch February 17, 2023 08:20

github-actions bot added this to the 408 milestone Feb 17, 2023

This was referenced Feb 20, 2023

Add a metadata table showing commits in Hudi connector #16181

Merged

Push down version filter in the Delta $history table #16192

Closed

colebow mentioned this pull request Feb 21, 2023

Add Trino 408 release notes #16209

Merged

findepi reviewed Feb 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid materializing all results in Iceberg `$files` system table at once #16112

Avoid materializing all results in Iceberg `$files` system table at once #16112

krvikash commented Feb 14, 2023 •

edited

Loading

krvikash commented Feb 14, 2023

alexjo2144 left a comment

krvikash commented Feb 14, 2023

krvikash commented Feb 15, 2023

krvikash commented Feb 15, 2023

krvikash commented Feb 15, 2023

krvikash commented Feb 16, 2023

ebyhr left a comment

krvikash commented Feb 16, 2023

findepi Feb 28, 2023

Avoid materializing all results in Iceberg $files system table at once #16112

Avoid materializing all results in Iceberg $files system table at once #16112

Conversation

krvikash commented Feb 14, 2023 • edited Loading

Description

Release notes

krvikash commented Feb 14, 2023

alexjo2144 left a comment

Choose a reason for hiding this comment

krvikash commented Feb 14, 2023

krvikash commented Feb 15, 2023

krvikash commented Feb 15, 2023

krvikash commented Feb 15, 2023

krvikash commented Feb 16, 2023

ebyhr left a comment

Choose a reason for hiding this comment

krvikash commented Feb 16, 2023

findepi Feb 28, 2023

Choose a reason for hiding this comment

Avoid materializing all results in Iceberg `$files` system table at once #16112

Avoid materializing all results in Iceberg `$files` system table at once #16112

krvikash commented Feb 14, 2023 •

edited

Loading