Iceberg: use LocationProvider instead of hardcoded path

[jackye1995]

Use Iceberg's LocationProvider instead of hard-coding file paths. With the current hard-coded Hive-based file paths to write data files, users on cloud storage cannot enjoy the benefit of Iceberg's ObjectStorageLocationProvider. This PR fixes this issue.

[findepi]

cc @losipiuk @alexjo2144

[findepi]

We do not use Java serialization. In particular, it can lead to security issues.

[jackye1995]

Sure, there are multiple ways we can go with this. We can also get the location provider through the dependency injection of table operation, since there is a general agreement that HiveTableOperations will be used across the board.

[jackye1995]

Thought about it more, it would be inefficient to initialize a table operation, read table metadata and get the location provider. The alternative way is to pass in table properties. Please see if the new version works, thanks!

[alexjo2144]

The code changes seem reasonable, just one broader thought. A table could specify any custom LocationProvider implementation in the properties. Is that location guaranteed to be compatible with org.apache.hadoop.fs.Path?

Alternatively, we could scope this down to just supporting DefaultLocationProvider and ObjectStoreLocationProvider by introducing a config or table property option and initializing the LocationProvider directly rather than through LocationProviders#locationsFor.

[jackye1995]

@alexjo2144 thanks for the feedback.

we could scope this down to just supporting DefaultLocationProvider and ObjectStoreLocationProvider

Sure I can introduce another static util method instead of using the Iceberg one.

Is that location guaranteed to be compatible with org.apache.hadoop.fs.Path?

This is related to the multi-catalog discussion, the conclusion there is that:

1. multi-catalog will be supported, but only ones explicitly listed by Trino through an Enum, no custom loading is supported.
2. only HdfsFileIO is used across all catalogs as the IO operator

So yes, it's guaranteed to be compatible.

[jackye1995]

@alexjo2144 I think this is the best we can do on Trino side, the 2 location provider are protected and not public classes, so we can only block the creation if locaiton-impl table property is set.

[findepi]

(just skimming)

[findepi]

doesn't seem to belong to "pass in table properties to initialize location provider in sink prov…" commit

[findepi]

doesn't seem to belong to "pass in table properties to initialize location provider in sink prov…" commit

[alexjo2144]

@findepi did you have an opinion on allowing custom LocationProviders vs restricting?

@jackye1995 this looks good to me, can you just add a test case to TestSparkCompatibility with a test case

• Create table in Spark with ObjectStorageLocationProvider
• Insert data from Spark
• Insert data from Trino
• Read from both and ensure both get the same data
• Read the "$path" column and ensure data files have the correct path structure

[jackye1995]

@findepi thanks for the comments, I am still getting familiar with the standards here so sorry for the bad commit message. I was used to commit with random message and merge with squash. I have force updated everything into a single commit, please let me know if this works.

@alexjo2144 test added, should be good for another look

[alexjo2144]

It's important to do an INSERT from Trino here too. That's what runs the changes you made in the IcebergPageSink

[jackye1995]

Yeah actually this is a good point, it does not test anything by inserting it using Spark, so I directly changed it to be executed on onTrino.

If we want to test Trino able to read Spark data written by object storage location provider, I think it should be placed in the testPrestoReadingSparkData test. But LocationProvider is a write side feature, on read side it's just using the file path in the manifest, so I think there is little value for adding that test case.

[alexjo2144]

Looks good to me. The option to create tables using ObjectStoreLocationProvider from Trino would be nice, but in separate Issue/PR

[jackye1995]

@alexjo2144 @findepi any updates on this? Please let me know if there is any change needed, otherwise could you merge the PR? Thanks!

[electrum]

If I'm understanding this correctly, the location provider should be specific to the catalog implementation, correct? If so, then we should add this to the upcoming catalog API. Or is this something that the user should configure, for example, depending on if they are using HDFS or S3?

Either way, this shouldn't be or be passed in a table property. We can discuss this on Slack if that's easier.

[electrum]

This parameter is for a filename, so an empty filename would seem to be invalid, and thus a location provider might reject it. We could use a default name like data-file instead.

[jackye1995]

@electrum thanks for the comment, I will both replay here just for visibility.

We discussed about the use of table properties in Trino versus Iceberg last time, what I found out is that they are not really the same concept after all. The fundamental difference is that:

On Trino side, table property is only used at table creation time, and it is up to the connector to decide what to do with each property. For Iceberg's case, we have:

1. partitioning: used to input partition transform information for Iceberg tables, stored as a partition spec in Iceberg table metadata JSON file
2. location: used to input table location, stored as the location field of the table metadata.
3. format, used to specify table format, stored as the write.format.default (Iceberg TableProperties.DEFAULT_FILE_FORMAT) in the table metadata's table properties map.

On Iceberg side, table properties are directly stored as a part of the table metadata JSON file, which is the format case in Trino Iceberg connector table property. The property is used to determine read/write behavior at runtime.

With this understanding, now we look at location provider configuration, which is related to the following Iceberg table properties:

1. write.object-storage.enabled: boolean to determine if object storage location provider should be used or not
2. write.object-storage.path: root path for data files in object storage mode
3. write.location-provider.impl: any custom location provider impl, which Trino will directly reject loading.

What this PR is trying to support is the case for which:

1. a user writes an Iceberg table with the above configurations set using some other systems like Spark/Hive
2. then the user tries to use Trino to also perform additional write operations

In the case described above, Trino should respect the configurations and write to the configured location instead of the hard-coded table location. This has nothing to do with user configuring any Trino table property.

I think what you are concerned about is the case where user can create a table with object storage mode or not based on a Trino table property or some other table features like the storage provider. I agree that behavior should be controlled. I am trying to keep this PR small, we can introduce a boolean table property like object_storage_mode and map to the Iceberg table properties behind the scene, similar to the format case.

[findepi]

@jackye1995

• can write.object-storage.path be used without specifying write.location-provider.impl?
• can write.object-storage.path be used without specifying write.object-storage.enabled? what should be the behavior then?

perhaps we can narrow down the scope of the change to

• respect write.object-storage.path (and write.object-storage.enabled if they must be used in tandem) -- instead of writing to table's directory (determined from location), write to location pointed to by write.object-storage.path
• throw when attempting to write when write.location-provider.impl is set

Such changes should require significant code changes, should they?

Actually, isn't the following implementing the aforementioned behavior already?

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

Line 258 in b8bcec2

return LocationProviders.locationsFor(metadata.location(), metadata.properties());

[jackye1995]

can write.object-storage.path be used without specifying write.location-provider.impl
can write.object-storage.path be used without specifying write.object-storage.enabled? what should be the behavior then?

write.object-storage.path is only used by ObjectStorageLocationProvider when write.object-storage.enabled is true. There is no other way to enable this as the class is not public. Other location provider implementations might leverage the same config, but Trino will reject it through write.location-provider.impl config.

perhaps we can narrow down the scope of the change to ...

Yes this PR is exactly trying to achieve the 2 goals you listed.

[jackye1995]

Actually, isn't the following implementing the aforementioned behavior already?

Yes, but to use it in the page sink provider, we need to construct a new HiveTableOperations which is quite expensive. That's why IcebergUtil.getLocationProvider is directly called.

[findepi]

Presto -> Trino

[findepi]

We should run this test for all supported formats, since page sink can have format-dependent behavior (currently it does not have). Requires #8751

[findepi]

is using OPTIONS different from using TBLPROPERTIES?

[jackye1995]

it's the same. I can change to TBLPROPERTIES for consistency.

[findepi]

inline sparkTableDefinition variable, it's redundant

(i know it's a copy of existing test code; i am doing this cleanup for existing tests in 566d41a)

[findepi]

in case of failure, this assertion will produce useless error message

user Assertions.assertThat.

[jackye1995]

@findepi thanks for the comments.

We should run this test for all supported formats

It looks like we just need to rebase based on which PR gets in first?

Apart from that, hopefully I have addressed all your comments except for the table deletion one.

we should have two delete modes

This is actually an interesting topic that I would like to also get fixed, because the current behavior is broken even for Spark Iceberg tables that are not using the object storage location provider.

In Iceberg there is another config write.folder-storage.path that can allow writing to a different location with a default location provider. Currently Trino just calls Hive to purge data, which would not work with this config.

The correct behavior is to call Iceberg's CatalogUtil.dropTableData which will recursively search all manifests and all data files in a table and delete them. I think we should just have a PR specifically for this fix, and it does not need any Spark-Trino compatibility test, we can directly test with normal Trino Iceberg unit test.

[jackye1995]

@findepi I think this is the best we can do for now for all the Iceberg path overrides. We know they come from external systems like Spark, so we will not drop the table to avoid leaving files not deleted during a drop table operation. I can work on the solution to this after this PR is merged, when we start to enable creating Trino Iceberg table with those overrides.

[losipiuk]

LGTM % minor comments.