Add table redirection support to SPI and Hive's redirection to Iceberg

[lxynov]

Add table redirection support to SPI

Added these methods to ConnectorMetadata. Please review the Javadoc comments for intended engine behaviors.

/**
/**
 * Redirects to another table or view which may or may not be in the same catalog.
 * <p>
 * This method is called whenever the engine references a table or view name, including both accessing and creating one. Connectors are
 * expected to return {@link Optional#empty()} if the redirection doesn't happen. The engine calls this method in an iterative manner,
 * i.e., using the redirected name as the argument for the next call, until an {@link Optional#empty()} is returned. A
 * {@link StandardWarningCode#TABLE_REDIRECTION} warning is raised every time a redirection happens. Access control checks are performed
 * on the final table, i.e., the end of the redirection chain. Whether a table is a view or not is also determined by the final table.
 * E.g., suppose table a.b.c is redirected to x.y.z, it's a view if and only if x.y.z is a view.
 * </p>
 * <em>Caveats</em>:
 * <li>
 * If you choose to implement this method, you should consider implementing {@link #listTableColumnsStream}, {@link #getViewsStream},
 * {@link #listTablePrivilegesStream}, too. Please review their doc for expected behaviors on redirected tables. They are called to answer
 * information schema queries. You should also consider implementing {@link #redirectTableRename}, which is used to redirect rename targets.
 * </li>
 * <li>
 * Be careful if you implement redirection to another catalog. Different catalogs have different sets of roles. In other words, GRANT/REVOKE
 * queries may break due to the non-existence of certain roles in the new catalog. Different connectors may have different table properties,
 * system tables, hidden columns, type support, etc., too.
 * </li>
 */
default Optional<CatalogSchemaTableName> redirectTable(ConnectorSession session, SchemaTableName tableName)
{
    return Optional.empty();
}

/**
 * Redirects the target table or view name of a rename operation to another one which may or may not be in the same catalog.
 * <p>
 * This method is called before the engine tries to rename a table or view. Connectors are expected to return {@link Optional#empty()} if
 * the redirection doesn't happen. The engine calls this method and {@link #redirectTable} to redirect both source and target tables in an
 * iterative manner, until both methods return {@link Optional#empty()}. A {@link StandardWarningCode#TABLE_REDIRECTION} warning is raised
 * every time a redirection happens. Access control checks are performed on the final table,  i.e., the end of the redirection chain.
 * </p>
 * <em>Caveats</em>: See {@link #redirectTable}'s documentation for caveats of implementing table redirection.
 */
default Optional<CatalogSchemaTableName> redirectTableRename(ConnectorSession session, SchemaTableName sourceTableName, SchemaTableName targetTableName)
{
    return Optional.empty();
}
/**
 * Gets the metadata for all columns that match the specified table prefix. Returns a {@link ListTableColumnsResult}
 * with an Optional.empty() field value if a table is redirected.
 */
default Stream<ListTableColumnsResult> listTableColumnsStream(ConnectorSession session, SchemaTablePrefix prefix)
{
    return listTableColumns(session, prefix).entrySet().stream().map(entry -> new ListTableColumnsResult(entry.getKey(), Optional.of(entry.getValue())));
}

/**
 * Gets the definitions of views, possibly filtered by schema. Returns a {@link GetViewsResult}
 * with an Optional.empty() field value if a table is redirected.
 */
default Stream<GetViewsResult> getViewsStream(ConnectorSession session, Optional<String> schemaName)
{
    return getViews(session, schemaName).entrySet().stream().map(entry -> new GetViewsResult(entry.getKey(), Optional.of(entry.getValue())));
}

/**
 * List the table privileges granted to the specified grantee for the tables that have the specified prefix considering the selected session role.
 * Returns a {@link ListTablePrivilegesResult} with an Optional.empty() field value if a table is redirected.
 */
default Stream<ListTablePrivilegesResult> listTablePrivilegesStream(ConnectorSession session, SchemaTablePrefix prefix)
{
    return listTablePrivileges(session, prefix).stream().map(grantInfo -> new ListTablePrivilegesResult(grantInfo.getSchemaTableName(), Optional.of(grantInfo)));
}

Add support to redirect from Hive to an Iceberg catalog

To enable this, we can either set Hive configs

hive.redirect-to-iceberg-enabled=true
hive.redirect-to-iceberg-catalog=iceberg

and keep session properties as default, or change session properties to

SET SESSION hive.redirect_to_iceberg_enabled = true;
SET SESSION hive.redirect_to_iceberg_catalog = 'iceberg';

---

Issue: #4442
Umbrella issue: #1324

[dain]

@electrum, do we want to include the redirection details here? If so, maybe we should make a helper, or sub class for table redirection.

[dain]

It looks like redirection is always taking this form. Maybe we should just warn in the MetadataManager?

Thinking more about this, if we did the warn in the MetadataManager then we might not need to know that a redirection occurred to the method could return the original name if no redirection happens?

[lxynov]

I firstly tried to include redirection details but later found that the DefaultWarningCollector stores warnings in a map keyed on WarningCodes, so a redirection warning's message can be overwritten by another one. So we'll need to implement another WarningCollector if we want to include details here. If we don't want to do so, it makes sense to move the warning to MetadataManager. @dain @electrum I'd leave it to you to make a call.

[dain]

Interesting. Let's leave this alone for now.

[electrum]

The warning calls everywhere makes the code really cluttered. They need to go inside MetadataManager.

Another issue is how to handle metadata operations like DESCRIBE, information_schema, etc., and how those relate to the low level ConnectorMetadata calls.

This PR is good for review purposes of the SPI, but before we merge it, we need to add another commit with the Iceberg connector changes, and full test coverage of all the affected areas (i.e. make sure all the DDL commands are tested with redirection). We may need to introduce redirection into another connector like Memory for testing purposes (e.g., we could redirect from one memory catalog to another).

[electrum]

We can introduce a replacement for ConnectorMetadata.listTableColumns():

Stream<ListColumnsResult> listColumns(ConnectorSession session, SchemaTablePrefix prefix);

class ListColumnsResult
{
    SchemaTableName tableName;
    Optional<CatalogSchemaTableName> redirection;
    Optional<List<ColumnMetadata>> columns;
}

The redirection and columns properties are a union type (exactly one is set). For any results any results containing a redirection, the engine will recursively follow them and make the appropriate listColumns() calls.

The engine guarantees that the Stream is closed, in case connectors need that for resource management. The streaming part is an improvement over the existing API as it means LIMIT queries could execute faster.

[electrum]

We need a similar change for getViews().

[electrum]

The engine changes look good. I'm still reviewing the Iceberg commit

[electrum]

Add error code and include original name

throw new PrestoException(TABLE_REDIRECTION_LIMIT, format("Too many redirections (%s) for table: %s", MAX_TABLE_REDIRECTIONS, originalTableName));

[electrum]

Same comments as for redirectTable()

[electrum]

We might be able to add a helper method in this class

QualifiedObjectName targetTable = getRedirectedTable(insert, insert.getTarget());

private QualifiedObjectName getRedirectedTable(Node node, QualifiedName name) { ... }

[lxynov]

Hmm this helper method won't help reduce the amount of code so I didn't use it in the updated PR. Please let me know if you think it's still better to use it.

[findepi]

Maybe include the whole cycle?
Otherwise with a redirect like A -> B -> C -> D -> E -> D it will be hard to identity D and E loop

[findepi]

What's the point in returning map entries with Optional.empty?
Is not returning them equally correct?

the existing code doesn't seem to use those empty entries, so if there is a future intent, it requires documenting (eg via @apiNote)

[lxynov]

Optional.empty means the table is redirected. Since it may be in another catalog, MetadataManager::listTablePrivileges should redirect it firstly and then call ConnectorMetadata::listTablePrivileges

[findepi]

Optional.empty means the table is redirected.

This i understand. But is it for? I didn't find any usage of those empty values, they are just filtered out . Should i search again?

[lxynov]

Hmm so in MetadataManager::listTablePrivileges:

    public List<GrantInfo> listTablePrivileges(Session session, QualifiedTablePrefix prefix)
    {
        ImmutableList.Builder<GrantInfo> grantInfos = ImmutableList.builder();
        ImmutableList.Builder<QualifiedObjectName> redirectedTables = ImmutableList.builder();
        listTablePrivilegesWithRedirection(session, prefix).forEach((name, grantInfo) -> {
            if (grantInfo.isPresent()) {
                grantInfos.addAll(grantInfo.get());
            }
            else {
                redirectedTables.add(new QualifiedObjectName(
                        prefix.getCatalogName(),
                        name.getSchemaName(),
                        name.getTableName()));
            }
        });
        for (QualifiedObjectName redirectedTable : redirectedTables.build()) {
            QualifiedObjectName finalTable = redirectTable(session, redirectedTable, WarningCollector.NOOP);
            listTablePrivilegesWithRedirection(session, finalTable.asQualifiedTablePrefix()).values()
                    .forEach(tableGrantInfos -> tableGrantInfos.ifPresent(grantInfos::addAll));
        }
        return grantInfos.build();
    }

When the value is present, grant infos are added to grantInfos directly; otherwise the key is put into redirectedTables list. And for redirected tables, listTablePrivilegesWithRedirection is called again.

[findepi]

It seems the implementation of listTablePrivilegesWithRedirection method could be simplified.
Before suggesting that i have a question on the listTablePrivilegesWithRedirection return type. Added there.

[findepi]

Why the change here?
Is listTableColumns deprecated and should not be called?

Or, we should test both: listTableColumns and listTableColumnsWithRedirection here?

(this comment applies to other test places as well)

[lxynov]

listTableColumns is removed from HiveMetadata

[findepi]

See previous comments.

.

[lxynov]

@electrum @findepi Thanks for the reviews! I've updated the PR. The failing check is unrelated. We can look into cancelled checks later. @findepi I changed the info schema related SPI methods to return Streams as @electrum suggested in #5160 (comment). We can add a patch later to make InformationSchemaPageSource utilize these Stream SPIs.

[findepi]

CI failed due to #5892 but not only

[findepi]

I have a general-level concern that the requirement to call metadata.redirectTable before calling a any other method on metadata can be error prone in practice, as it seems to easy not to call the redirectTable and overlook the missing call also in the review process. I am sure you have thought about this very deeply already... if you happen to have any notes about alternatives considered, i would be curious to learn more.

[lxynov]

This concern does make sense. I had a similar feeling when I was implementing this PR. However, the table redirection approach seems to be an appropriate one to address the hive-iceberg migration issue. A few other approaches that have been discussed:

• Federation connector: Introduce a new connector which federates between Hive and Iceberg Connectors.
• Search path support

cc @electrum

[findepi]

Thanks.
But also if we settle up on redirect, we could try to model the API so that there is a help from type system to make sure redirection is considered.
Like we have in functions -- function name, resolved function, etc.

[findepi]

include "RenameRedirection" (or at least "redirect") in the toString.

Also consider replacing , with =>

[lxynov]

Included "Rename" as the pair reflects a rename relationship

[findepi]

Add a check in getTableHandle and getTableHandleForStatisticsCollection like

verify(redirectTable(...).isEmpty(), "Table access should have been redirected for %s", tableName);

This will make sure (or improve probability that) if we miss to add metadata.redirectTable somewhere (in existing or new code), this will not lead to incorrect results or data corruption.

[lxynov]

Good idea. Added in them as well as some other methods.

[findepi]

Per general style guidelines, the .filter and .map above should be preceded with a new line.
I'd recommend to do this as a follow up though, not to incur bigger diff within this PR.

[findepi]

Does this if buy as much?

regardless of the case, we call listTables(session, prefix).
then, in redirect case we call redirectTable on each item of the list, which is a very cheap operation if redirect is not enabled.
Thus, you could safely handle both cases with single code path.

[lxynov]

I don't understand clearly... Can you share a code snippet?

[findepi]

@Override
public Stream<ListTablePrivilegesResult> listTablePrivilegesStream(ConnectorSession session, SchemaTablePrefix prefix)
{
    ImmutableList.Builder<SchemaTableName> redirectedTables = ImmutableList.builder();
    ImmutableList.Builder<SchemaTableName> notRedirectedTables = ImmutableList.builder();
    listTables(session, prefix).forEach(table -> {
        if (redirectTable(session, table).isPresent()) {
            redirectedTables.add(table);
        }
        else {
            notRedirectedTables.add(table);
        }
    });
    return Stream.concat(
            accessControlMetadata.listTablePrivileges(session, notRedirectedTables.build()).stream()
                    .map(grantInfo -> new ListTablePrivilegesResult(grantInfo.getSchemaTableName(), Optional.of(grantInfo))),
            redirectedTables.build().stream()
                    .map(table -> new ListTablePrivilegesResult(table, Optional.empty())));
}

[findepi]

Would that work?

[findepi]

Are all these needed? i guess not, so please remove them, so the relevant configuration is easier to fish out

[lxynov]

Removed irrelevant configs.

[findepi]

irrelevant?

[lxynov]

The Iceberg Connector is needed to test the redirection.

[findepi]

Surely. I was commenting on the last line of the config only (iceberg.file-format=PARQUET)

[findepi]

❤️ for a product test!

Did you create or plan to create a QueryRunner-based one too?
That wouldn't make the PT obsolete, but could be helpful for fine-grained testing or do debug things, should anything ever malfunction.

[lxynov]

Didn't create a QueryRunner-based one because we'll need to install both the Iceberg and Hive plugins in one query runner.

[findepi]

Can you use both from presto-tests?

[lxynov]

@findepi Thanks for the review! I've updated the PR. @electrum Could you give this another review?

[findepi]

make it final and perhaps @Immutable

[findepi]

@lxynov was this covered?

[phd3]

preparatory commits look good. Reviewing main commits.

"Allow multiple warnings with the same warning code"
"Add WarningCollector to data definition task interface"

[phd3]

Reviewed commit Add table redirection methods to SPI.

I've the same high level concern as #5160 (comment). Did we have any further discussions or consensus on it?

[phd3]

Suggested change

	StringBuilder sb = new StringBuilder();
	visitedTableNames.forEach(name -> {
	sb.append(name);
	sb.append(" -> ");
	});
	throw new TrinoException(TABLE_REDIRECTION_LIMIT, format("Too many table redirections (%d): %s", MAX_TABLE_REDIRECTIONS, sb.substring(0, sb.length() - 4)));
	String redirections = visitedTableNames.stream()
	.map(QualifiedObjectName::toString)
	.collect(Collectors.joining(" -> "));
	throw new TrinoException(TABLE_REDIRECTION_LIMIT, format("Too many table redirections (%d): %s", MAX_TABLE_REDIRECTIONS, redirections));

[phd3]

say c1.s1.t1 is redirected to c2.s2.t2. Then it seems that SHOW CREATE for view will return CREATE VIEW c1.s1.t1 .... but for table, it'll return CREATE TABLE c2.s2.t2. is that right? we may want to keep it consistent.

[phd3]

same suggested change using Collectors.joining

[phd3]

link ListTablePrivilegesResult

[phd3]

could you explain why we need this?

[phd3]

this can cause some possible confusing behavior from SQL perspective, if A.T1 redirects to B.T2. May be just return original name?

> select table_schema, table_name from C.information_schema.tables where table_catalog ='C' and table_schema = 'A' and table_name='T1';

 table_schema  |  table_name   
---------------+---------------
 B             | T2 

[phd3]

nit: add a comment on this method saying that the caller should pass in the redirected table name

[phd3]

may be we should also document applyTableScanRedirect to clarify the difference, but this can be done as followup.

[phd3]

Suggested change

	StringBuilder sb = new StringBuilder();
	visitedTableNames.forEach(name -> {
	sb.append(name);
	sb.append(" -> ");
	});
	sb.append(tableName);
	throw new TrinoException(TABLE_REDIRECTION_LOOP, "Table redirections form a loop: " + sb.toString());
	String redirectionChain = new StringBuilder()
	.append(visitedTableNames.stream()
	.map(QualifiedObjectName::toString)
	.collect(Collectors.joining(" -> ")))
	.append(" -> ")
	.append(tableName)
	.toString();
	throw new TrinoException(TABLE_REDIRECTION_LOOP, "Table redirections form a loop: " + redirectionChain);

(similarly in other method)

[phd3]

may be change the order here, since that's what engine's method is doing?

[electrum]

Make this a Javadoc comment on the getter

[electrum]

Make this a Javadoc comment on the getter

[electrum]

Make all these result classes final

[electrum]

Nit: wrap stream operations on separate line

return listTableColumns(session, prefix).entrySet().stream()
        .map(entry -> new ListTableColumnsResult(entry.getKey(), Optional.of(entry.getValue())));

Same for others

[electrum]

Make this a Javadoc comment on the getter

[electrum]

I think this can use join from String:

String redirections = join(" -> ", visitedTableNames);

[electrum]

Since we use a Set for visitedTableNames, it won't show the full chain, e.g., a -> b -> a. We could have a separate List<String> chain that we use for the error message:

chain.add(tableName);
if (!visitedTableNames.add(tableName)) {
    break;
}

[electrum]

Was using forEach not possible?

[electrum]

Let's introduce a method

verifyNotRedirected(session, tableName);

[sopel39]

@lxynov Could we extract 76a813f as a separte PR?

[phd3]

@sopel39 I've recently taken over this work from @lxynov, will create a PR with only the SPI changes from #7016 first.

[sopel39]

@sopel39 I've recently taken over this work from @lxynov, will create a PR with only the SPI changes from #7016 first.

Good to know. Thanks!

[sopel39]

@phd3 did yo have a chance to extract 76a813f?

[phd3]

Superseded by #7606