Added ability to have unique table location for each iceberg table

[sshkvar]

#5632

Added new iceberg configuration property iceberg.unique-table-location

By default this property = false, so table directory will have the same name as table.
In case iceberg.unique-table-location = true unique UUID will be added to the table directory name, so each table will have unique location

[electrum]

A few minor comments, otherwise looks good

[sshkvar]

A few minor comments, otherwise looks good

@electrum Thanks for the comments, I have performed code fixes for all of them

[sshkvar]

@electrum is it possible to have this changes in the next release?

[sshkvar]

@electrum could you please approve this PR in case of all looks good?

[sshkvar]

@electrum I would be really appreciate for your review of this PR

[raunaqmorarka]

Please add test to AbstractTestIcebergSmoke to verify that some UUID as added to table location.

Does iceberg.unique-table-location have any purpose beyond making it possible for Trino to delete data on drop of table ?
If not, it would be easier to have just one flag iceberg.delete-files-on-table-drop and implement both unique table location and data removal behind that flag rather than requiring users to set 2 flags.

Should we also be providing this as a table property ? From user point of view it's easier to create a "managed" table using table property rather than changing config to set a catalog property.

[findepi]

@sshkvar thanks for working on this.

Can you please rebase?

We should also have a test for this that would cover
CREATE + DROP
CREATE + RENAME + DROP

showing that table data gets correctly dropped.

[findepi]

Thanks for adding the test.
Overall LGTM. Some editorial comments in the test.

[findepi]

cc @rdblue

[findepi]

Please update formatting

• all parameters on one line OR each parameter on its own line (including the first one)
• for indentation please refer to https://github.com/airlift/codestyle/

[sshkvar]

Thanks for the link to code style.
Formatting updated

[findepi]

We call it connectorProperties in other XxxQueryRunner classes.

also, no need for optional, since empty map has the same meaning

           Map<String, String> connectorProperties)

also, please add it right after extraProperties (also seems usual)

[sshkvar]

done

[findepi]

Why two levels of nesting?
Would it be fine to have just

metastoreDir = Files.createTempDirectory("....");

?

[findepi]

however, it seems the test doesn't need to know the metastoreDir, so instead of setting it up (and cleaning it later), it should be sufficient to pass Optional.empty() for the metastoreDir, right?

[sshkvar]

we need metastoreDir because it is used in createTestingFileHiveMetastore(metastoreDir),
but I removed two levels nesting

[findepi]

use default

new IcebergConfig().getFileFormat()

[findepi]

as a follow up we should remove format parameter and use the new connectorProperties instead

[sshkvar]

replaced with new IcebergConfig().getFileFormat()

[findepi]

Do not use assertTrue with complex expressions. In case of failure, it produces unhelpful message.

Use

[org.assertj.core.api.Assertions.] assertThat(location).matches(....);

instead

[sshkvar]

done

[findepi]

@sshkvar could you please remove the merge commits from the git history?
you should be able to do this with git rebase.

[sshkvar]

@sshkvar could you please remove the merge commits from the git history?
you should be able to do this with git rebase.

done

[findepi]

@sshkvar thanks!
is the separation into 4 commits significant here? if not, can you squash them into single commit?
(again, git rebase may be useful here)

[sshkvar]

@findepi I combined changes to single commit

[findepi]

Merged, thanks!

[sshkvar]

Merged, thanks!

Thanks you for the review!