Skip to content

[Simplified Version] Add table redirection support to SPI and Hive's redirection to Iceberg#7016

Closed
lxynov wants to merge 4 commits intotrinodb:masterfrom
lxynov:table-redirection-simple
Closed

[Simplified Version] Add table redirection support to SPI and Hive's redirection to Iceberg#7016
lxynov wants to merge 4 commits intotrinodb:masterfrom
lxynov:table-redirection-simple

Conversation

@lxynov
Copy link
Copy Markdown
Member

@lxynov lxynov commented Feb 24, 2021

Removed DDL, INSERT, COMMENT, ANALYZE, etc. support w.r.t. #5160.

The engine tries to do redirection only for table reads and metadata listing in this PR.

@cla-bot cla-bot bot added the cla-signed label Feb 24, 2021
@sopel39
Copy link
Copy Markdown
Member

sopel39 commented Feb 24, 2021

Curious if you have seen io.trino.spi.connector.ConnectorMetadata#applyTableScanRedirect. Do you think it could be used for your use case?

@lxynov
Copy link
Copy Markdown
Member Author

lxynov commented Feb 25, 2021

Curious if you have seen io.trino.spi.connector.ConnectorMetadata#applyTableScanRedirect. Do you think it could be used for your use case?

@sopel39 ConnectorMetadata#applyTableScanRedirect is expected to return a mapping of source column handles to destination table columns. However, an Iceberg table might not have correct Hive metadata stored in HMS, which is required to generate correct Hive column handles.

Copy link
Copy Markdown
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

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

Reviewed first 3 commits.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if we want to do this. may be we can keep these features isolated for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We're now returning a stream version of List<GrantInfo> which's expected to have additional info on tablename and redirection. So may be naming TableGrantInfo instead of ListTablePrivilegesResult is more appropriate?

or using a stream of Map.Entry<SchemaTableName, Optional<GrantInfo>> might be easier to relate with the current listTablePrivileges method?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the call verifyNoAggregateWindowOrGroupingFunctions should not use table.getName for error message.

visitTable -> analyzeFiltersAndMasks -> analyzeColumnMasks -> verifyNoAggregateWindowOrGroupingFunctions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if redirected view doesn't exist, we don't return the original view here. but in the unfiltered case, we don't check whether redirected view exists, so it appears. This seems to introduce another inconsistency between filtered/unfiltered case, right?

this seems to hint towards having another similar api for listViews, but not sure about a good design for it.

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented Feb 25, 2021

@sopel39 ConnectorMetadata#applyTableScanRedirect is expected to return a mapping of source column handles to destination table columns. However, an Iceberg table might not have correct Hive metadata stored in HMS, which is required to generate correct Hive column handles.

@lxynov Could you elaborate more? We don't need to generate target Hive column handles when redirecting, but just proper column names. I expect during redirection one knows which source column handle maps to which target column name.

@lxynov
Copy link
Copy Markdown
Member Author

lxynov commented Feb 26, 2021

@sopel39 Sure. This is to handle the case of redirecting from Hive Connector to Iceberg Connector. To use ConnectorMetadata#applyTableScanRedirect, we must have valid source (Hive) column handles, which might not be possible if an Iceberg table doesn't have valid Hive metadata (stored in HMS). Apart from this, ConnectorMetadata#applyTableScanRedirect doesn't have support for metadata listing. But this PR tries to make queries like SELECT * FROM hive.information_schema.columns also return Iceberg table columns.

When enabled, Hive Connector would redirect table reads to an Iceberg
catalog when it is an Iceberg table.
@phd3
Copy link
Copy Markdown
Member

phd3 commented Apr 20, 2021

Superseded by #7606

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants