Support position deletes for Iceberg TableEngine

[divanik]

Changelog category

• New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Support position deletes for Iceberg TableEngine

Documentation entry for user-facing changes

• Documentation is written (mandatory for new features)

This PR is a second attempt of merging Positional deletes to CH. Comparing to this PR several issues are going to be addressed:

• Docs should be written (the box is above)

• Adress this comment ([Iceberg] Support position deletes for Iceberg TableEngine #80237 (comment))
• Figure out what happened with the flaky test here

[clickhouse-gh]

Workflow [PR], commit [7251b06]

Summary: ✅

[alexey-milovidov]

Do not merge until all tests for Iceberg:
https://play.clickhouse.com/play?user=play#U0VMRUNUIGNoZWNrX3N0YXJ0X3RpbWUsIGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcmVwb3J0X3VybApGUk9NIGNoZWNrcwpXSEVSRSBjaGVja19zdGFydF90aW1lID49IG5vdygpIC0gSU5URVJWQUwgMSBNT05USAogICAgQU5EIChoZWFkX3JlZiA9ICdtYXN0ZXInIEFORCBzdGFydHNXaXRoKGhlYWRfcmVwbywgJ0NsaWNrSG91c2UvJykpCiAgICBBTkQgdGVzdF9zdGF0dXMgIT0gJ1NLSVBQRUQnCiAgICBBTkQgKHRlc3Rfc3RhdHVzIExJS0UgJ0YlJyBPUiB0ZXN0X3N0YXR1cyBMSUtFICdFJScpIAogICAgQU5EIGNoZWNrX3N0YXR1cyAhPSAnc3VjY2VzcycKICAgIEFORCBwb3NpdGlvbih0ZXN0X25hbWUsICdpY2ViZXJnJykgPiAwCk9SREVSIEJZIGNoZWNrX3N0YXJ0X3RpbWU=

for Delta https://play.clickhouse.com/play?user=play#U0VMRUNUIGNoZWNrX3N0YXJ0X3RpbWUsIGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcmVwb3J0X3VybApGUk9NIGNoZWNrcwpXSEVSRSBjaGVja19zdGFydF90aW1lID49IG5vdygpIC0gSU5URVJWQUwgMSBNT05USAogICAgQU5EIChoZWFkX3JlZiA9ICdtYXN0ZXInIEFORCBzdGFydHNXaXRoKGhlYWRfcmVwbywgJ0NsaWNrSG91c2UvJykpCiAgICBBTkQgdGVzdF9zdGF0dXMgIT0gJ1NLSVBQRUQnCiAgICBBTkQgKHRlc3Rfc3RhdHVzIExJS0UgJ0YlJyBPUiB0ZXN0X3N0YXR1cyBMSUtFICdFJScpIAogICAgQU5EIGNoZWNrX3N0YXR1cyAhPSAnc3VjY2VzcycKICAgIEFORCBwb3NpdGlvbih0ZXN0X25hbWUsICdkZWx0YScpID4gMApPUkRFUiBCWSBjaGVja19zdGFydF90aW1l

for Hive Metastore https://play.clickhouse.com/play?user=play#U0VMRUNUIGNoZWNrX3N0YXJ0X3RpbWUsIGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcmVwb3J0X3VybApGUk9NIGNoZWNrcwpXSEVSRSBjaGVja19zdGFydF90aW1lID49IG5vdygpIC0gSU5URVJWQUwgMSBNT05USAogICAgQU5EIChoZWFkX3JlZiA9ICdtYXN0ZXInIEFORCBzdGFydHNXaXRoKGhlYWRfcmVwbywgJ0NsaWNrSG91c2UvJykpCiAgICBBTkQgdGVzdF9zdGF0dXMgIT0gJ1NLSVBQRUQnCiAgICBBTkQgKHRlc3Rfc3RhdHVzIExJS0UgJ0YlJyBPUiB0ZXN0X3N0YXR1cyBMSUtFICdFJScpIAogICAgQU5EIGNoZWNrX3N0YXR1cyAhPSAnc3VjY2VzcycKICAgIEFORCBwb3NpdGlvbih0ZXN0X25hbWUsICdobXMnKSA+IDAKT1JERVIgQlkgY2hlY2tfc3RhcnRfdGltZQ==

for Glue https://play.clickhouse.com/play?user=play#U0VMRUNUIGNoZWNrX3N0YXJ0X3RpbWUsIGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcmVwb3J0X3VybApGUk9NIGNoZWNrcwpXSEVSRSBjaGVja19zdGFydF90aW1lID49IG5vdygpIC0gSU5URVJWQUwgMSBNT05USAogICAgQU5EIChoZWFkX3JlZiA9ICdtYXN0ZXInIEFORCBzdGFydHNXaXRoKGhlYWRfcmVwbywgJ0NsaWNrSG91c2UvJykpCiAgICBBTkQgdGVzdF9zdGF0dXMgIT0gJ1NLSVBQRUQnCiAgICBBTkQgKHRlc3Rfc3RhdHVzIExJS0UgJ0YlJyBPUiB0ZXN0X3N0YXR1cyBMSUtFICdFJScpIAogICAgQU5EIGNoZWNrX3N0YXR1cyAhPSAnc3VjY2VzcycKICAgIEFORCBwb3NpdGlvbih0ZXN0X25hbWUsICdnbHVlJykgPiAwCk9SREVSIEJZIGNoZWNrX3N0YXJ0X3RpbWU=

will be fixed.

Otherwise, any pull request on data lakes will be reverted immediately.

[divanik]

@alexey-milovidov, what relation do Delta, Hive Metastore and Glue have with a feature that is specific to Iceberg? The Iceberg flaky test is going to be investigated as stated in the header of this PR, of course, it seems this PR is connected to the failure

[alexey-milovidov]

I want to prioritize the resolution of technical debt.

[divanik]

For now I failed to reproduce what id going on here :-(
It seems that I need to launch the whole job on a particular commit but I don't know how to do it

[divanik]

Status update: I managed to reproduce the test locally with help of @ fm4v (one test fails in a hundred of attempts, so feed back loop is going to be big, but at least now it exists), will try to establish a root cause of flaky test using this repro. Also will try to get repro of this commit to use CI for reproducing: a9c7ec1

[hanfei1991]

it looks very complex here. I suspect why this cache is needed:
now that we cache ManifestFileContent in IcebergMetadataFilesCache, it has cached:

• std::vector<ManifestFileEntry> data_files;
• std::vector<ManifestFileEntry> position_deletes_files;

That means if we hit this cache, we have almost no cost to get data/deletes files entry.

[hanfei1991]

so can we just remove these cache?

[divanik]

It theoretically can lead to unpredicted perfomance consequences, though I really also want to do it, so let's try and maybe return it if it is needed

[divanik]

We can remove cache but we can't remove ParsedDataFileInfo, unfortunately (it is used not only in cache)

[alexey-milovidov]

What happened in the stress test?

[hanfei1991]

The IcebergDataObjectInfo only contains one field ParsedDataFileInfo parsed_data_file_info, why do we need two different object info IcebergDataObjectInfo and ParserdDataFileInfo? what is the difference between them?

[divanik]

IcebergDataObjectInfo is also a derived from RelativePathWithMetadata, it can potentially take a lot of memory and we don't to store it explicitly

[hanfei1991]

In ManifestFile.h we have stored std::vector<ManifestFileEntry> data_files; and in IcebergMetadata we want to transform it to ParsedDataFileInfo to save memory use. How about using std::shared_ptr<ManifestFileEntry> in both files?

[divanik]

This doesn't work unfortunately, we can't allow ourselves to store ManifestFileEntry of all data files in a table, doesn't matter if we use shared_ptr or not. In ManifestFileContent only files which correspond to one manifest file are explicitly stored which is affordable

[hanfei1991]

Why will it store ManifestFileEntry of all data files in a table? It's called in iterate function in a streaming way, everytime we iterate we also read a ManifestFileContent, so I think the ManifestFileEntrys have same life span in iterators as in ManifestFileContent

[hanfei1991]

rest LGTM