rdblue commented on code in PR #7913: URL: https://github.com/apache/iceberg/pull/7913#discussion_r1367565262
########## core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java: ########## @@ -971,4 +996,261 @@ public void commitTransaction(SessionContext context, List<TableCommit> commits) headers(context), ErrorHandlers.tableCommitHandler()); } + + @Override + public List<TableIdentifier> listViews(SessionContext context, Namespace namespace) { + checkNamespaceIsValid(namespace); + + ListTablesResponse response = + client.get( + paths.views(namespace), + ListTablesResponse.class, + headers(context), + ErrorHandlers.namespaceErrorHandler()); + return response.identifiers(); + } + + @Override + public View loadView(SessionContext context, TableIdentifier identifier) { + checkViewIdentifierIsValid(identifier); + + LoadViewResponse response = + client.get( + paths.view(identifier), + LoadViewResponse.class, + headers(context), + ErrorHandlers.viewErrorHandler()); + + AuthSession session = tableSession(response.config(), session(context)); + ViewMetadata metadata = response.metadata(); + + RESTViewOperations ops = + new RESTViewOperations( + client, + paths.view(identifier), + session::headers, + tableFileIO(context, response.config()), + metadata); + + trackFileIO(ops); + + return new BaseView(ops, ViewUtil.fullViewName(name(), identifier)); + } + + @Override + public RESTViewBuilder buildView(SessionContext context, TableIdentifier identifier) { + return new RESTViewBuilder(context, identifier); + } + + @Override + public boolean dropView(SessionContext context, TableIdentifier identifier) { + checkViewIdentifierIsValid(identifier); + + try { + client.delete( + paths.view(identifier), null, headers(context), ErrorHandlers.viewErrorHandler()); + return true; + } catch (NoSuchViewException e) { + return false; + } + } + + @Override + public void renameView(SessionContext context, TableIdentifier from, TableIdentifier to) { + checkViewIdentifierIsValid(from); + checkViewIdentifierIsValid(to); + + RenameTableRequest request = + RenameTableRequest.builder().withSource(from).withDestination(to).build(); + + client.post( + paths.renameView(), request, null, headers(context), ErrorHandlers.viewErrorHandler()); + } + + private class RESTViewBuilder implements ViewBuilder { + private final SessionContext context; + private final TableIdentifier identifier; + private final Map<String, String> properties = Maps.newHashMap(); + private final List<ViewRepresentation> representations = Lists.newArrayList(); + private Namespace defaultNamespace = null; + private String defaultCatalog = null; + private Schema schema = null; + private String location = null; + + private RESTViewBuilder(SessionContext context, TableIdentifier identifier) { + checkViewIdentifierIsValid(identifier); + this.identifier = identifier; + this.context = context; + } + + @Override + public ViewBuilder withSchema(Schema newSchema) { + this.schema = newSchema; + return this; + } + + @Override + public ViewBuilder withQuery(String dialect, String sql) { + representations.add( + ImmutableSQLViewRepresentation.builder().dialect(dialect).sql(sql).build()); + return this; + } + + @Override + public ViewBuilder withDefaultCatalog(String catalog) { + this.defaultCatalog = catalog; + return this; + } + + @Override + public ViewBuilder withDefaultNamespace(Namespace namespace) { + this.defaultNamespace = namespace; + return this; + } + + @Override + public ViewBuilder withProperties(Map<String, String> newProperties) { + this.properties.putAll(newProperties); + return this; + } + + @Override + public ViewBuilder withProperty(String key, String value) { + this.properties.put(key, value); + return this; + } + + @Override + public ViewBuilder withLocation(String newLocation) { + this.location = newLocation; + return this; + } + + @Override + public View create() { + Preconditions.checkState( + !representations.isEmpty(), "Cannot create view without specifying a query"); + Preconditions.checkState(null != schema, "Cannot create view without specifying schema"); + Preconditions.checkState( + null != defaultNamespace, "Cannot create view without specifying a default namespace"); + + ViewVersion viewVersion = + ImmutableViewVersion.builder() + .versionId(1) + .schemaId(schema.schemaId()) + .addAllRepresentations(representations) + .defaultNamespace(defaultNamespace) + .defaultCatalog(defaultCatalog) + .timestampMillis(System.currentTimeMillis()) + .putSummary("operation", "create") + .build(); + + CreateViewRequest request = + ImmutableCreateViewRequest.builder() + .name(identifier.name()) + .location(location) + .schema(schema) + .viewVersion(viewVersion) + .properties(properties) + .build(); + + LoadViewResponse response = + client.post( + paths.views(identifier.namespace()), + request, + LoadViewResponse.class, + headers(context), + ErrorHandlers.viewErrorHandler()); + + return viewFromResponse(response); + } + + @Override + public View createOrReplace() { + try { + return replace(loadView()); + } catch (NoSuchViewException e) { + return create(); + } + } + + @Override + public View replace() { + return replace(loadView()); + } + + private LoadViewResponse loadView() { + return client.get( + paths.view(identifier), + LoadViewResponse.class, + headers(context), + ErrorHandlers.viewErrorHandler()); + } + + private View replace(LoadViewResponse response) { + Preconditions.checkState( + !representations.isEmpty(), "Cannot replace view without specifying a query"); + Preconditions.checkState(null != schema, "Cannot replace view without specifying schema"); + Preconditions.checkState( + null != defaultNamespace, "Cannot replace view without specifying a default namespace"); + + ViewMetadata metadata = response.metadata(); + + int maxVersionId = + metadata.versions().stream() + .map(ViewVersion::versionId) + .max(Integer::compareTo) + .orElseGet(metadata::currentVersionId); + + ViewVersion viewVersion = + ImmutableViewVersion.builder() + .versionId(maxVersionId + 1) + .schemaId(schema.schemaId()) + .addAllRepresentations(representations) + .defaultNamespace(defaultNamespace) + .defaultCatalog(defaultCatalog) + .timestampMillis(System.currentTimeMillis()) + .putSummary("operation", "replace") + .build(); + + ViewMetadata.Builder builder = + ViewMetadata.buildFrom(metadata) + .setProperties(properties) + .setCurrentVersion(viewVersion, schema); + + if (null != location) { + builder.setLocation(location); + } + + ViewMetadata replacement = builder.build(); + + UpdateTableRequest request = + UpdateTableRequest.create(identifier, ImmutableList.of(), replacement.changes()); Review Comment: I don't think this should create its own request and call the route directly. This duplicates logic that is in `RESTViewOperations`. Instead, this should create `RESTViewOperations` initially and then use it to commit the replacement metadata. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org