Skip to content

Retrieve the table handle only when dealing with a system table #15568

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

Merged
ebyhr merged 2 commits into from
Mar 8, 2023
Merged

Retrieve the table handle only when dealing with a system table #15568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34c4c1b
Retrieve the table handle only when dealing with a system table
findinpath Dec 30, 2022
20e802a
Use `ENGLISH` instead of `ROOT` locale
findinpath Mar 8, 2023
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
46 changes: 21 additions & 25 deletions plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,11 +345,11 @@ public IcebergTableHandle getTableHandle(
throw new TrinoException(NOT_SUPPORTED, "Read table with start version is not supported");
Comment thread
findinpath marked this conversation as resolved.
Outdated
}

IcebergTableName name = IcebergTableName.from(tableName.getTableName());
if (name.getTableType() != DATA) {
if (!IcebergTableName.isDataTable(tableName.getTableName())) {
// Pretend the table does not exist to produce better error message in case of table redirects to Hive
return null;
}
IcebergTableName name = IcebergTableName.from(tableName.getTableName());

BaseTable table;
try {
Expand Down Expand Up @@ -437,15 +437,15 @@ public Optional<SystemTable> getSystemTable(ConnectorSession session, SchemaTabl

private Optional<SystemTable> getRawSystemTable(ConnectorSession session, SchemaTableName tableName)
{
IcebergTableName name = IcebergTableName.from(tableName.getTableName());
if (name.getTableType() == DATA) {
if (IcebergTableName.isDataTable(tableName.getTableName())) {
return Optional.empty();
}

// load the base table for the system table
// Only when dealing with an actual system table proceed to retrieve the base table for the system table
String name = IcebergTableName.tableNameFrom(tableName.getTableName());
Table table;
try {
table = catalog.loadTable(session, new SchemaTableName(tableName.getSchemaName(), name.getTableName()));
table = catalog.loadTable(session, new SchemaTableName(tableName.getSchemaName(), name));
}
catch (TableNotFoundException e) {
return Optional.empty();
Expand All @@ -455,25 +455,21 @@ private Optional<SystemTable> getRawSystemTable(ConnectorSession session, Schema
return Optional.empty();
}

SchemaTableName systemTableName = new SchemaTableName(tableName.getSchemaName(), name.getTableNameWithType());
switch (name.getTableType()) {
case DATA:
// Handled above.
break;
case HISTORY:
return Optional.of(new HistoryTable(systemTableName, table));
case SNAPSHOTS:
return Optional.of(new SnapshotsTable(systemTableName, typeManager, table));
case PARTITIONS:
return Optional.of(new PartitionTable(systemTableName, typeManager, table, getCurrentSnapshotId(table)));
case MANIFESTS:
return Optional.of(new ManifestsTable(systemTableName, table, getCurrentSnapshotId(table)));
case FILES:
return Optional.of(new FilesTable(systemTableName, typeManager, table, getCurrentSnapshotId(table)));
case PROPERTIES:
return Optional.of(new PropertiesTable(systemTableName, table));
}
return Optional.empty();
Optional<TableType> tableType = IcebergTableName.tableTypeFrom(tableName.getTableName());
if (tableType.isEmpty()) {
return Optional.empty();
}
IcebergTableName icebergTableName = new IcebergTableName(name, tableType.get());
SchemaTableName systemTableName = new SchemaTableName(tableName.getSchemaName(), icebergTableName.getTableNameWithType());
return switch (icebergTableName.getTableType()) {
case DATA -> Optional.empty(); // Handled above.
case HISTORY -> Optional.of(new HistoryTable(systemTableName, table));
case SNAPSHOTS -> Optional.of(new SnapshotsTable(systemTableName, typeManager, table));
case PARTITIONS -> Optional.of(new PartitionTable(systemTableName, typeManager, table, getCurrentSnapshotId(table)));
case MANIFESTS -> Optional.of(new ManifestsTable(systemTableName, table, getCurrentSnapshotId(table)));
case FILES -> Optional.of(new FilesTable(systemTableName, typeManager, table, getCurrentSnapshotId(table)));
case PROPERTIES -> Optional.of(new PropertiesTable(systemTableName, table));
};
}

@Override
Expand Down
59 changes: 55 additions & 4 deletions plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableName.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@

import io.trino.spi.TrinoException;

import java.util.Locale;
import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static io.trino.plugin.iceberg.TableType.DATA;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static java.lang.String.format;
import static java.util.Locale.ENGLISH;
import static java.util.Objects.requireNonNull;

public class IcebergTableName
Expand Down Expand Up @@ -50,7 +52,7 @@ public TableType getTableType()

public String getTableNameWithType()
{
return tableName + "$" + tableType.name().toLowerCase(Locale.ROOT);
return tableName + "$" + tableType.name().toLowerCase(ENGLISH);
}

@Override
Expand All @@ -69,10 +71,10 @@ public static IcebergTableName from(String name)
String table = match.group("table");
String typeString = match.group("type");

TableType type = TableType.DATA;
TableType type = DATA;
if (typeString != null) {
try {
type = TableType.valueOf(typeString.toUpperCase(Locale.ROOT));
type = TableType.valueOf(typeString.toUpperCase(ENGLISH));
}
catch (IllegalArgumentException e) {
throw new TrinoException(NOT_SUPPORTED, format("Invalid Iceberg table name (unknown type '%s'): %s", typeString, name));
Expand All @@ -81,4 +83,53 @@ public static IcebergTableName from(String name)

return new IcebergTableName(table, type);
}

public static String tableNameFrom(String name)
{
Matcher match = TABLE_PATTERN.matcher(name);
if (!match.matches()) {
throw new TrinoException(NOT_SUPPORTED, "Invalid Iceberg table name: " + name);
}

return match.group("table");
}

public static Optional<TableType> tableTypeFrom(String name)
{
Matcher match = TABLE_PATTERN.matcher(name);
if (!match.matches()) {
throw new TrinoException(NOT_SUPPORTED, "Invalid Iceberg table name: " + name);
}
String typeString = match.group("type");
if (typeString == null) {
return Optional.of(DATA);
}
try {
return Optional.of(TableType.valueOf(typeString.toUpperCase(ENGLISH)));
}
catch (IllegalArgumentException e) {
return Optional.empty();
}
}

public static boolean isDataTable(String name)
{
Matcher match = TABLE_PATTERN.matcher(name);
if (!match.matches()) {
throw new TrinoException(NOT_SUPPORTED, "Invalid Iceberg table name: " + name);
}
String typeString = match.group("type");
if (typeString == null) {
return true;
}
else {
try {
TableType type = TableType.valueOf(typeString.toUpperCase(ENGLISH));
return type == DATA;
}
catch (IllegalArgumentException e) {
return false;
}
}
}
}
40 changes: 40 additions & 0 deletions plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergTableName.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,13 @@

import org.testng.annotations.Test;

import java.util.Optional;

import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.trino.testing.assertions.TrinoExceptionAssert.assertTrinoExceptionThrownBy;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;

public class TestIcebergTableName
{
Expand All @@ -40,6 +44,42 @@ public void testFrom()
assertInvalid("abc$manifests@456", "Invalid Iceberg table name: abc$manifests@456");
}

@Test
public void testIsDataTable()
{
assertTrue(IcebergTableName.isDataTable("abc"));
assertTrue(IcebergTableName.isDataTable("abc$data"));

assertFalse(IcebergTableName.isDataTable("abc$history"));
assertFalse(IcebergTableName.isDataTable("abc$invalid"));
}

@Test
public void testTableNameFrom()
{
assertEquals(IcebergTableName.tableNameFrom("abc"), "abc");
assertEquals(IcebergTableName.tableNameFrom("abc$data"), "abc");
assertEquals(IcebergTableName.tableNameFrom("abc$history"), "abc");
assertEquals(IcebergTableName.tableNameFrom("abc$invalid"), "abc");
}

@Test
public void testTableTypeFrom()
{
assertEquals(IcebergTableName.tableTypeFrom("abc"), Optional.of(TableType.DATA));
assertEquals(IcebergTableName.tableTypeFrom("abc$data"), Optional.of(TableType.DATA));
assertEquals(IcebergTableName.tableTypeFrom("abc$history"), Optional.of(TableType.HISTORY));

assertEquals(IcebergTableName.tableTypeFrom("abc$invalid"), Optional.empty());
}

@Test
public void testGetTableNameWithType()
{
assertEquals(new IcebergTableName("abc", TableType.DATA).getTableNameWithType(), "abc$data");
assertEquals(new IcebergTableName("abc", TableType.HISTORY).getTableNameWithType(), "abc$history");
}

private static void assertInvalid(String inputName, String message)
{
assertTrinoExceptionThrownBy(() -> IcebergTableName.from(inputName))
Expand Down
15 changes: 15 additions & 0 deletions ...duct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergRedirectionToHive.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,21 @@ public void testRedirectPartitionsToUnpartitioned()
onTrino().executeQuery("DROP TABLE " + hiveTableName);
}

@Test(groups = {HIVE_ICEBERG_REDIRECTIONS, PROFILE_SPECIFIC_TESTS})
public void testRedirectInvalidSystemTable()
{
String tableName = "hive_invalid_table_" + randomNameSuffix();
String hiveTableName = "hive.default." + tableName;
String icebergTableName = "iceberg.default." + tableName;

createHiveTable(hiveTableName, false);

assertQueryFailure(() -> onTrino().executeQuery("TABLE iceberg.default.\"" + tableName + "$invalid\""))
.hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): Table '" + icebergTableName + "$invalid' redirected to '" + hiveTableName + "$invalid', but the target table '" + hiveTableName + "$invalid' does not exist");

onTrino().executeQuery("DROP TABLE " + hiveTableName);
}

@Test(groups = {HIVE_ICEBERG_REDIRECTIONS, PROFILE_SPECIFIC_TESTS})
public void testRedirectPartitionsToPartitioned()
{
Expand Down