Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update REST Catalog impl for smoke tests and remove obsolete override for test #1

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Update REST Catalog impl for smoke tests and remove obsolete override for test #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 63 additions & 1 deletion ...rino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/RestCatalogTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,77 @@
import org.apache.hadoop.fs.Path;
import org.apache.iceberg.CatalogProperties;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.catalog.ViewCatalog;
import org.apache.iceberg.inmemory.InMemoryCatalog;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.jdbc.JdbcCatalog;
import org.apache.iceberg.jdbc.JdbcClientPool;
import org.apache.iceberg.view.View;
import org.apache.iceberg.view.ViewBuilder;
import org.assertj.core.util.Files;

import java.io.File;
import java.util.List;
import java.util.Map;
import java.util.function.Function;

public final class RestCatalogTestUtils
{
private RestCatalogTestUtils() {}

static class TestingJdbcCatalog
extends JdbcCatalog implements ViewCatalog
Comment on lines +50 to +51
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed since the upgraded 1.5 RestSessionCatalog will check if a view exists when performing a replace. This effectively means the underlying catalog being used must implement ViewCatalog otherwise the loadView call will fail since the response cannot be parsed.
To fix this, I'm implementing a TestingJdbcCatalog which implements ViewCatalog. We'll need this anyways in the REST View Catalog implementation PR. trinodb#19818

{
InMemoryCatalog viewCatalog;

public TestingJdbcCatalog()
{
this((Function) null, (Function) null, true);
}

public TestingJdbcCatalog(
Function<Map<String, String>, FileIO> ioBuilder,
Function<Map<String, String>, JdbcClientPool> clientPoolBuilder,
boolean initializeCatalogTables)
{
super(ioBuilder, clientPoolBuilder, initializeCatalogTables);
this.viewCatalog = new InMemoryCatalog();
viewCatalog.initialize("testing-views", ImmutableMap.of());
}

@Override
public List<TableIdentifier> listViews(Namespace namespace)
{
return viewCatalog.listViews(namespace);
}

@Override
public View loadView(TableIdentifier identifier)
{
return viewCatalog.loadView(identifier);
}

@Override
public ViewBuilder buildView(TableIdentifier identifier)
{
return viewCatalog.buildView(identifier);
}

@Override
public boolean dropView(TableIdentifier identifier)
{
return viewCatalog.dropView(identifier);
}

@Override
public void renameView(TableIdentifier from, TableIdentifier to)
{
viewCatalog.renameView(from, to);
}
}

public static Catalog backendCatalog(File warehouseLocation)
{
ImmutableMap.Builder<String, String> properties = ImmutableMap.builder();
Expand All @@ -50,7 +112,7 @@ public static Catalog backendCatalog(File warehouseLocation)
HdfsEnvironment hdfsEnvironment = new HdfsEnvironment(hdfsConfiguration, hdfsConfig, new NoHdfsAuthentication());
HdfsContext context = new HdfsContext(connectorSession);

JdbcCatalog catalog = new JdbcCatalog();
JdbcCatalog catalog = new TestingJdbcCatalog();
catalog.setConf(hdfsEnvironment.getConfiguration(context, new Path(warehouseLocation.getAbsolutePath())));
catalog.initialize("backend_jdbc", properties.buildOrThrow());

Expand Down
8 changes: 0 additions & 8 deletions ...a/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,6 @@ public void testDropTableWithMissingManifestListFile()
.hasMessageContaining("Table location should not exist");
}

@Test
@Override
public void testDropTableWithMissingDataFile()
{
assertThatThrownBy(super::testDropTableWithMissingDataFile)
.hasMessageContaining("Table location should not exist");
}

@Test
Comment on lines -191 to 189
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was failing expectedly after the upgrade because we made the change to remove Puffin files as part of https://github.com/apache/iceberg/pull/9305/files . We used to expect this test to fail in this override because Puffin files would linger after the "DROP" and we have an assertion that validates that all files are removed so the test would fail. Now, this override is no longer needed, we expect the base testDropTableWithMissingDataFile to pass. So we can just remove the test override and rely on the base.

@Override
public void testDropTableWithNonExistentTableLocation()
Expand Down