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.

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

Support views in iceberg connector #2703

Closed
wants to merge 4 commits into from

Conversation

alexey-fin
Copy link
Contributor

CREATE/GET/DROP VIEW
I copied functions from HiveMetadata, adjusting for not having SemiTransactionalHiveMetastore

@cla-bot cla-bot bot added the cla-signed label Feb 1, 2020
@electrum
Copy link
Member

electrum commented Feb 1, 2020

Please add a test in TestIcebergSmoke. You can probably copy TestHiveIntegrationSmokeTest#testRenameView.

@vrozov
Copy link
Contributor

vrozov commented Feb 20, 2020

@alexey-fin Please see why few checks fail.

@vrozov
Copy link
Contributor

vrozov commented Feb 26, 2020

@alexey-fin Any update on the PR? It will be good to enable view support in the iceberg connector.

@alexey-fin
Copy link
Contributor Author

Sorry, @vrozov, will get back to it as soon as i can

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Overall looks good. When updating, please squash the commits so that we have two commits titled

  • Extract commit view functions to HiveUtil
  • Add support for views to Iceberg

@@ -1710,24 +1685,11 @@ public void dropView(ConnectorSession session, SchemaTableName viewName)
@Override
public Optional<ConnectorViewDefinition> getView(ConnectorSession session, SchemaTableName viewName)
{
return metastore.getTable(new HiveIdentity(session), viewName.getSchemaName(), viewName.getTableName())
HiveIdentity identity = new HiveIdentity(session);
Copy link
Member

Choose a reason for hiding this comment

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

This can stay inlined in the getTable call

.setParameters(properties)
.setViewOriginalText(Optional.of(encodeViewData(definition)))
.setViewExpandedText(Optional.of("/* Presto View */"));
Table table = HiveUtil.buildViewTable(definition, viewName, owner, session.getQueryId(), prestoVersion);
Copy link
Member

Choose a reason for hiding this comment

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

Static import buildViewTable

.build();
String owner = session.getUser();

Table table = HiveUtil.buildViewTable(definition, viewName, owner, session.getQueryId(), prestoVersion);
Copy link
Member

Choose a reason for hiding this comment

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

Static import buildViewTable

@electrum
Copy link
Member

Note that we still need tests in TestIcebergSmoke

@electrum
Copy link
Member

@alexey-fin Are you still interested in this? Would you like someone else to take this over?

@raunaqmorarka
Copy link
Member

Superseded by #8540

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.

4 participants