Skip to content

Fix metadata table names conflicts #1772

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

Closed
wants to merge 5 commits into from
Closed

Fix metadata table names conflicts #1772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e8cba2f
Overriding metadata existence check
fivetran-kostaszoumpatianos May 30, 2025
5cadeb6
Simplified code
fivetran-kostaszoumpatianos May 30, 2025
8d6c0e3
Added test
fivetran-kostaszoumpatianos Jun 2, 2025
ebb7e5e
Spotless
fivetran-kostaszoumpatianos Jun 2, 2025
cd47fcf
Removed not-required property
fivetran-kostaszoumpatianos Jun 2, 2025
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
17 changes: 11 additions & 6 deletions ...s/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,7 @@
import java.lang.reflect.Method;
import java.net.URI;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.UUID;
import java.util.*;
Copy link
Contributor

@eric-maynard eric-maynard Jun 2, 2025

Choose a reason for hiding this comment

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

We try to avoid star imports, so let's change this back

Copy link
Contributor Author

@fivetran-kostaszoumpatianos fivetran-kostaszoumpatianos Jun 2, 2025

Choose a reason for hiding this comment

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

Indeed, that looked weird. Either spotlessApply or intelliJ for some reason thought that this is ok. I will revert it back.

import java.util.stream.Stream;
import org.apache.hadoop.conf.Configuration;
import org.apache.iceberg.BaseTable;
Expand Down Expand Up @@ -563,6 +558,16 @@ public void testCreateTableWithOverriddenBaseLocationCannotOverlapSibling() {
.hasMessageContaining("because it conflicts with existing table or namespace");
}

@Test
public void testCreateTableWithMetadataConflictingName() {
Catalog catalog = managementApi.getCatalog(currentCatalogName);
restCatalog.createNamespace(Namespace.of("ns1"));
TableIdentifier tableIdentifier = TableIdentifier.of("ns1", "history");
restCatalog.buildTable(tableIdentifier, SCHEMA).withProperty("stage-create", "true").create();
Table table = restCatalog.loadTable(tableIdentifier);
assertThat(table).isNotNull().isInstanceOf(BaseTable.class);
}

@Test
public void testCreateTableWithOverriddenBaseLocationMustResideInNsDirectory() {
Catalog catalog = managementApi.getCatalog(currentCatalogName);
Expand Down
8 changes: 8 additions & 0 deletions service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,14 @@ public IcebergCatalog(
this.polarisEventListener = polarisEventListener;
}

@Override
public boolean tableExists(TableIdentifier identifier) {
if (isValidIdentifier(identifier)) {
return newTableOps(identifier).current() != null;
Copy link
Contributor

@eric-maynard eric-maynard Jun 2, 2025

Choose a reason for hiding this comment

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

Instead of this, can we just use loadTable directly like the current code does?

Copy link
Contributor Author

@fivetran-kostaszoumpatianos fivetran-kostaszoumpatianos Jun 2, 2025

Choose a reason for hiding this comment

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

This kind of defies the purpose, as this will call BaseMetastoreCatalog::loadTable and this will try to resolve the table as a metadata table - if it doesn't exist. We should avoid that if we want to allow Polaris to create tables named after these keywords.

Copy link
Contributor

@eric-maynard eric-maynard Jun 2, 2025

Choose a reason for hiding this comment

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

I see. For context, this will force a trip to object storage to load the table's metadata, which #433 (being re-implemented) proposes to skip via a trip to the metastore. I know this method isn't called often, but it's a shame if we have to lose that performance optimization.

Copy link
Contributor Author

@fivetran-kostaszoumpatianos fivetran-kostaszoumpatianos Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

loadTable(...) currently does exactly the same thing internally.
Once we optimize that and table metadata are promoted to metastore we could adapt this method as well.

}
return false;
}

@Override
public String name() {
return catalogName;
Expand Down
Loading