Use plugin class loader for the operations of IcebergPageSource

[findinpath]

Description

The problem of using the App ClassLoader instead of the Plugin ClassLoader was uncovered within: 4753ecc

Since then, there are multiple operations that relate to core SQL functionality on Iceberg connector.
See for reference:

• Iceberg rollback_to_snapshot procedure fails due to classloader issues #12887 (comment)
• Reading Iceberg table with delete files sometimes fail due to invalid class loader #13035

This PR makes use of wrapper classes in order to hide the aspect related to
ensuring the usage of the plugin class loader before calling the
page source operations.

NOTE that this PR fixed the class loader safe handling only for the Iceberg connector. Other connectors need to be eventually handled separately as well.

Is this change a fix, improvement, new feature, refactoring, or other?

Fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Iceberg connector

How would you describe this change to a non-technical end user or system administrator?

Basic SQL statements like DELETE or CREATE TABLE AS are failing on Iceberg due to the fact that engine refuses to make use of the app class loader in situations where it is expected to work with the plugin class loader.
This PR ensures that this condition is met for the Iceberg connector operations.

Related issues, pull requests, and links

Relates to the PR: #13036

The mentioned PR tries to handle in a generic fashion the issue of dealing with a class loader safe Connector PageSource.
However, Iceberg deals with an UpdatablePageSource which would imply using instanceof checks within ClassLoaderSafeConnectorPageSourceProvider.

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Iceberg
* Fix query failure when reading an Iceberg table with deletion files. ({issue}`13035`)

Fixes #13035

[losipiuk]

Generally LGTM but should you change the tests which expose the problem you are fixing.

[losipiuk]

Do you need to wrap internal method calls or would just this be enough:

        Supplier<ConnectorPageSink> positionDeleteSink = () -> {
            try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) {
                // Make sure we are using the PluginClassLoader when initialising and using the sink instance
                return new IcebergPositionDeletePageSink(
                        split.getPath(),
                        partitionSpec,
                        partition,
                        locationProvider,
                        fileWriterFactory,
                        hdfsEnvironment,
                        hdfsContext,
                        jsonCodec,
                        session,
                        split.getFileFormat(),
                        table.getStorageProperties(),
                        split.getFileRecordCount());
            };

The alternative mathces what we do in ClassLoaderSafeConnectorPageSourceProvider

[homar]

I think it is enough to either do try/catch here or use ClassLoaderSafeConnectorPageSink

[findinpath]

try/catch is used for initialising the IcebergPositionDeletePageSink.
I thought that the ClassLoaderSafeConnectorPageSink wrapper is needed because the page sink is used within the methods of UpdateablePageSource (deleteRows/updateRows).
However, in the meantime I've wrapped the UpdateablePageSource with a class loader safe wrapper, so probably there is no need anymore to do the wrapping for the page sinks as well.

[homar]

If you use ClassLoaderSafeConnectorPageSink there is no nee to do this try/catch here it is done inside

[homar]

Same here, maybe I am missing something but I think it is enough to use ClassLoaderSafeConnectorPageSink without try/catch above. You can see for example how ClassLoaderSafeConnectorPageSinkProvider is used

[homar]

Also I don't understand why here you did this double wrapping and below with IcebergPageSource you didn't

[findinpath]

#13078 (comment)

[findinpath]

After addressing the changes I got to a similar state as what is present in #13036
I still need to find relevant tests to pinpoint the classloader issue to accompany this PR.

cc @homar

[electrum]

Let’s change ConfigurationInstantiator to simply use the correct class loader.

[findinpath]

Let’s change ConfigurationInstantiator to simply use the correct class loader.

Can you please detail this request?

In the master version of Trino, the methods of IcebergPageSource are called from within the engine and they use therefore the AppClassLoader. This is why I opted to call the the methods on IcebergPageSource through a class loader safe UpdateableSource wrapper.

There are (at the moment) two ConfigurationInstantiator classes. (one in hive, one in phoenix).
The changes from 4753ecc were thought to uncover possible class loader related issues that would otherwise lead to bogus class not found errors.

[findinpath]

@electrum I have added a new commit to the PR which is supposed to ensure the usage of the plugin class loader while instantiating a Configuration object.

This commit undoes part of changes from the commit 4753ecc

cc @findepi

[lhofhansl]

It seems that Iceberg (v2) support has to be considered non-functional without this (and #13112).

answered

[findepi]

test with TestClassLoaderSafeWrappers

[findepi]

remove @ForClassLoaderSafe.
the class is not going to be used in Guice.

[findepi]

use io.trino.spi.classloader.ThreadContextClassLoader

(add trino-spi as a dep)

[findinpath]

In the context of accepting these changes, do we still need the class loader related changes made into RollbackToSnapshotProcedure:

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

Lines 71 to 78 in 159b84d

	// this line guarantees that classLoader that we stored in the field will be used inside try/catch
	// as we captured reference to PluginClassLoader during initialization of this class
	// we can use it now to correctly execute the procedure
	try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) {
	SchemaTableName schemaTableName = new SchemaTableName(schema, table);
	Table icebergTable = catalogFactory.create(clientSession.getIdentity()).loadTable(clientSession, schemaTableName);
	icebergTable.manageSnapshots().setCurrentSnapshot(snapshotId).commit();
	}

?