Fix iceberg $files metadata table not show delete files #16232

0xffmeta · 2023-02-23T11:26:01Z

Description

This PR is aimed to fix $files table not showing delete files for iceberg v2 format. #16233

Additional context and related issues

Release notes

( ) This is not user-visible or 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:

# Section
* Fix `$files` table not showing delete files for iceberg v2 format. ({issue}`[16233]`)

cla-bot · 2023-02-23T11:26: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 · 2023-02-24T02:08:09Z

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 · 2023-02-24T08:13:24Z

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 · 2023-02-24T08:24:26Z

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 · 2023-02-25T05:35:38Z

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

0xffmeta · 2023-02-27T06:46:03Z

fyi - I have already signed CLA, and I think it might be pending on the process at this moment.

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

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java

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

cla-bot · 2023-02-27T13:37:36Z

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

0xffmeta · 2023-02-27T13:42:02Z

Thanks @ebyhr for the review. I have updated the PR according to the comment.
Please take a look when you have some time.

krvikash · 2023-03-01T07:23:47Z

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

@@ -255,6 +271,32 @@ private Object toIntegerVarcharMapBlock(Map<Integer, String> values)
 blockBuilder.closeEntry();
 return integerToVarcharMapType.getObject(blockBuilder, 0);
 }
+
+ private Object getIntegerArrayBlock(List<Integer> values)


Since the type is changed in 1st commit Change to use TypeManager to get ArrayType signature. then type-related changes should be part of 1st commit.

Make sense. Let me put those changes into the 1st commit.

krvikash · 2023-03-01T07:25:53Z

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

@@ -140,6 +145,8 @@ public PlanFilesIterable(CloseableIterable<FileScanTask> planFiles, Map<Integer,
 this.types = ImmutableList.copyOf(requireNonNull(types, "types is null"));
 this.integerToBigintMapType = typeManager.getType(mapType(INTEGER.getTypeSignature(), BIGINT.getTypeSignature()));
 this.integerToVarcharMapType = typeManager.getType(mapType(INTEGER.getTypeSignature(), VARCHAR.getTypeSignature()));
+ this.integerArrayType = typeManager.getType(arrayType(INTEGER.getTypeSignature()));
+ this.bigintArrayType = typeManager.getType(arrayType(INTEGER.getTypeSignature()));


nit: I'd have declared and assigned in order (same as order of tableMetadata) of bigintArrayType and then integerArrayType.

Nice catch. Let me update the PR.

cla-bot · 2023-03-02T06:14:08Z

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

krvikash

Overall LGTM.

krvikash · 2023-03-02T12:22:12Z

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

+ return null;
+ }
+
+ BlockBuilder blockBuilder = bigintArrayType.createBlockBuilder(null, values.size());


expected entries are 1 block of List type.

Suggested change

BlockBuilder blockBuilder = bigintArrayType.createBlockBuilder(null, values.size());

BlockBuilder blockBuilder = bigintArrayType.createBlockBuilder(null, 1);

Thanks for the review. Let me update this accordingly.

krvikash · 2023-03-02T12:22:31Z

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

+ return null;
+ }
+
+ BlockBuilder blockBuilder = integerArrayType.createBlockBuilder(null, values.size());


expected entries are 1 block of List type.

Suggested change

BlockBuilder blockBuilder = integerArrayType.createBlockBuilder(null, values.size());

BlockBuilder blockBuilder = integerArrayType.createBlockBuilder(null, 1);

krvikash · 2023-03-02T12:25:12Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java

+ .filter(path -> !path.contains("regionkey=1"))
+ .toArray(String[]::new));


is this change needed? If yes then it should be in a separate commit.

Oh...this might be related with my code format. It's not needed...let me revert this in the new commit.

cla-bot · 2023-03-03T09:25:57Z

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

0xffmeta · 2023-03-03T09:30:09Z

@krvikash Thanks for the review. I just updated the PR accordingly. Please take a look when you have some time.

findinpath · 2023-03-07T12:19:17Z

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

@@ -25,7 +25,6 @@
 import io.trino.spi.connector.SchemaTableName;


Fix split_offsets and equality_ids to use ArrayBlock for size calculation -> Wrap collection values in array blocks

Let me update the commit message.

findinpath · 2023-03-07T12:20:20Z

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

@@ -171,16 +174,25 @@ public CloseableIterator<List<Object>> iterator()
 addCloseable(planFilesIterator);

 return new CloseableIterator<>() {
+ private Iterator<DeleteFile> deleteFileIterator = emptyIterator();


nit: deleteFileIterator -> deleteFilesIterator

findinpath · 2023-03-07T13:03:50Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java

+ @Test
+ public void testFilesTableWithDelete()
+ {
+ assertQuery("SELECT count(*) FROM test_schema.\"test_table_with_delete$files\" WHERE content = 0", "VALUES 4");


Use FileContent .id() method instead of magic constants here.

findinpath · 2023-03-07T13:28:52Z

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

 }

 @Override
 public List<Object> next()
 {
- return getRecord(planFilesIterator.next().file());
+ if (deleteFileIterator.hasNext()) {


Use createMetadataTableInstance(icebergTable, FILES) in io.trino.plugin.iceberg.FilesTable#cursor to be able to get delete files from planFiles().

TableScan tableScan = createMetadataTableInstance(icebergTable, FILES) .newScan() .includeColumnStats();

While your solution works as well, I find it retrieves a bit too much data in order to provide the functionality required by $files table.

I've done some time ago an experiment related to #11206 which you may find useful here:

master...findinpath:trino:findinpath/add-all-files

Thanks for the detailed info. Let me use the createMetadataTableInstance for the table scan.

alexjo2144 · 2023-03-07T15:23:09Z

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

+ }
+ else {
+ FileScanTask planFileTask = planFilesIterator.next();
+ deleteFileIterator = planFileTask.deletes().iterator();


This has the potential to include duplicates. Equality deletes can belong to several files, so those will show up multiple times here.

Good point. Let me find a way to deduplicate the delete files and add a test case for this.

I think by using the metadata table scan suggested by @findinpath, it can avoid duplicated equality files in the final results. I have added one test in TestIcebergV2.

@0xffmeta try the following in a product test environment

testing/bin/ptl env up --environment 'singlenode-spark-iceberg' --config 'config-default'

via spark-sql

create table default.test1 (x int) using iceberg tblproperties ('format-version' = '2','write.delete.mode' = 'merge-on-read', 'write.update.mode'='merge-on-read', 'write.merge.mode'='merge-on-read');

via trino

insert into t1 values 1,2,3,4,5,6; insert into t1 values 1,2,3,4,5,6;

via spark-sql

delete from t1 where x=2; -- position deletes delete from t1 where x > 2; -- position deletes

check via Trino whether $files returns duplicated equality delete files.

via flink can be written equality deletes

delete from t1 where x=2;

The equality deletes may be duplicated (in case they apply to several data files) in the content of $files - in such a case we'll need to figure out a way to deduplicate such entries...

Great. Thanks for the detailed steps. Let me try to check this in the test env.

check via Trino whether $files returns duplicated equality delete files.

The trino result:

via flink can be written equality deletes

Not sure which environment should be used to create a flink-sql container... @findinpath Do you know how to run flink sql in product test env?

ebyhr · 2023-03-07T22:57:13Z

@cla-bot check

hashhar · 2024-03-01T09:53:19Z

@alexjo2144 @krvikash This is important to fix.

One thing I've observed people trying to do is to build some way to identify whether their table needs to be optimized to remove delete files for example.

A simple logic is to see if the size of delete files is above some threshold or the count of records in delete files is above some threshold. Unfortunately Trino cannot be used for this today because $files doesn't show delete files.

github-actions · 2024-03-22T17:14:37Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-03-22T18:00:22Z

Maybe @electrum @findepi or @findinpath can help out here.

Also @0xffmeta could you rebase?

github-actions · 2024-04-15T17:03:37Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

bitsondatadev · 2024-04-16T01:11:35Z

@amoghmargoor would you mind giving this review a pass please?

@0xffmeta let me know if you're willing to pick this up. I do apologize it's taken this long. If we don't hear from you after a couple weeks we'll pick up the work. I definitely think this is important to track.

0xffmeta · 2024-04-16T02:53:19Z

As @hashhar mentioned, we also have the similar usecase to view the status of deletion file, and we turned out to use spark to view those files. But it's not efficienct to switch between trino sql and spark sql. With more and more usecase onboared to v2 format, I think this will be usefull to allow the user to check the deletion file status.

@bitsondatadev Sure, let me pick this up and will rebase this PR for the review.

bitsondatadev · 2024-04-16T03:11:22Z

As @hashhar mentioned, we also have the similar usecase to view the status of deletion file, and we turned out to use spark to view those files. But it's not efficienct to switch between trino sql and spark sql. With more and more usecase onboared to v2 format, I think this will be usefull to allow the user to check the deletion file status.

@bitsondatadev Sure, let me pick this up and will rebase this PR for the review.

Awesome, I'll do what I can to keep the momentum going. Don't hesitate to ping me.

0xffmeta · 2024-04-23T10:07:04Z

Hi @bitsondatadev, this PR is ready for the review. :)

hashhar · 2024-04-25T08:51:08Z

@findinpath / @alexjo2144 Can you please help with a review here?

ebyhr · 2024-06-24T02:53:15Z

Rebased on master to resolve conflicts and fixed a wrong style.

ebyhr · 2024-06-24T03:41:33Z

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

+ // the StructWithReadableMetrics is a private class in iceberg-core
+ // so we need to use reflection to access it
+ try {
+ Class<?> clazz = dataTaskFile.getClass();
+ Field field = clazz.getDeclaredField("struct");
+ field.setAccessible(true);
+ innerContentFile = (StructLike) field.get(dataTaskFile);
+ }


Is it possible to avoid reflection if we manage a map (name -> position) like SnapshotsTable?

0xffmeta mentioned this pull request Feb 23, 2023

Iceberg $files table not show delete files #16233

Open

0xffmeta force-pushed the fix_files_system_table branch from 1de3c51 to 669cec6 Compare February 24, 2023 08:13

0xffmeta force-pushed the fix_files_system_table branch from 669cec6 to 64c4803 Compare February 24, 2023 08:24

krvikash self-requested a review February 24, 2023 09:00

0xffmeta force-pushed the fix_files_system_table branch from 64c4803 to d10a1bf Compare February 25, 2023 05:35

ebyhr reviewed Feb 27, 2023

View reviewed changes

0xffmeta force-pushed the fix_files_system_table branch from d10a1bf to 62254c7 Compare February 27, 2023 13:37

ebyhr self-requested a review February 28, 2023 07:22

krvikash reviewed Mar 1, 2023

View reviewed changes

0xffmeta force-pushed the fix_files_system_table branch from 62254c7 to 7fdec9e Compare March 2, 2023 06:14

github-actions bot added the iceberg Iceberg connector label Mar 2, 2023

krvikash reviewed Mar 2, 2023

View reviewed changes

0xffmeta force-pushed the fix_files_system_table branch from 7fdec9e to e133a14 Compare March 3, 2023 09:25

findepi requested review from findinpath and alexjo2144 March 3, 2023 12:14

krvikash mentioned this pull request Mar 6, 2023

Add tests for eqaulity deletes in iceberg connector #16389

Draft

findinpath reviewed Mar 7, 2023

View reviewed changes

alexjo2144 reviewed Mar 7, 2023

View reviewed changes

github-actions bot removed the stale label Feb 12, 2024

github-actions bot added the stale label Mar 22, 2024

github-actions bot removed the stale label Mar 25, 2024

github-actions bot added the stale label Apr 15, 2024

bitsondatadev added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Apr 16, 2024

0xffmeta force-pushed the fix_files_system_table branch 9 times, most recently from c5c856d to 06bb2b2 Compare April 19, 2024 07:23

0xffmeta force-pushed the fix_files_system_table branch 2 times, most recently from 9638685 to c31c13d Compare April 23, 2024 08:22

Show delete content in $files metadata table

aec59f5

ebyhr force-pushed the fix_files_system_table branch from c31c13d to aec59f5 Compare June 24, 2024 02:52

ebyhr reviewed Jun 24, 2024

View reviewed changes

	BlockBuilder blockBuilder = bigintArrayType.createBlockBuilder(null, values.size());
	BlockBuilder blockBuilder = bigintArrayType.createBlockBuilder(null, 1);

	BlockBuilder blockBuilder = integerArrayType.createBlockBuilder(null, values.size());
	BlockBuilder blockBuilder = integerArrayType.createBlockBuilder(null, 1);

		.filter(path -> !path.contains("regionkey=1"))
		.toArray(String[]::new));

		@@ -25,7 +25,6 @@
		import io.trino.spi.connector.SchemaTableName;

Fix iceberg $files metadata table not show delete files #16232

Are you sure you want to change the base?

Fix iceberg $files metadata table not show delete files #16232

Conversation

0xffmeta commented Feb 23, 2023 • edited Loading

Description

Additional context and related issues

Release notes

cla-bot bot commented Feb 23, 2023

cla-bot bot commented Feb 24, 2023

cla-bot bot commented Feb 24, 2023

cla-bot bot commented Feb 24, 2023

cla-bot bot commented Feb 25, 2023

0xffmeta commented Feb 27, 2023

cla-bot bot commented Feb 27, 2023

0xffmeta commented Feb 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented Mar 2, 2023

krvikash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented Mar 3, 2023

0xffmeta commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xffmeta Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Mar 7, 2023

hashhar commented Mar 1, 2024

github-actions bot commented Mar 22, 2024

mosabua commented Mar 22, 2024

github-actions bot commented Apr 15, 2024

bitsondatadev commented Apr 16, 2024

0xffmeta commented Apr 16, 2024

bitsondatadev commented Apr 16, 2024

0xffmeta commented Apr 23, 2024

hashhar commented Apr 25, 2024

ebyhr commented Jun 24, 2024

ebyhr Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

0xffmeta commented Feb 23, 2023 •

edited

Loading

0xffmeta Mar 15, 2023 •

edited

Loading

ebyhr Jun 24, 2024 •

edited

Loading