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.

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

Jump to bottom

Add DDL statements to drop branches and tags #23614

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add DDL statements to drop branches and tags #23614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e2c8be
Add DROP BRANCH sql support
agrawalreetika Aug 24, 2024
11bcf92
Add support for drop branch for Iceberg Table
agrawalreetika Aug 24, 2024
464303e
Add DROP TAG sql support
agrawalreetika Sep 8, 2024
5af0a0d
Add support for drop tag for Iceberg Table
agrawalreetika Sep 8, 2024
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
4 changes: 4 additions & 0 deletions presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/utils/StatementUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,15 @@
import com.facebook.presto.sql.tree.Delete;
import com.facebook.presto.sql.tree.DescribeInput;
import com.facebook.presto.sql.tree.DescribeOutput;
import com.facebook.presto.sql.tree.DropBranch;
import com.facebook.presto.sql.tree.DropColumn;
import com.facebook.presto.sql.tree.DropConstraint;
import com.facebook.presto.sql.tree.DropFunction;
import com.facebook.presto.sql.tree.DropMaterializedView;
import com.facebook.presto.sql.tree.DropRole;
import com.facebook.presto.sql.tree.DropSchema;
import com.facebook.presto.sql.tree.DropTable;
import com.facebook.presto.sql.tree.DropTag;
import com.facebook.presto.sql.tree.DropView;
import com.facebook.presto.sql.tree.Explain;
import com.facebook.presto.sql.tree.Grant;
Expand Down Expand Up @@ -129,6 +131,8 @@ private StatementUtils() {}
builder.put(RenameColumn.class, QueryType.DATA_DEFINITION);
builder.put(DropColumn.class, QueryType.DATA_DEFINITION);
builder.put(DropTable.class, QueryType.DATA_DEFINITION);
builder.put(DropBranch.class, QueryType.DATA_DEFINITION);
builder.put(DropTag.class, QueryType.DATA_DEFINITION);
builder.put(DropConstraint.class, QueryType.DATA_DEFINITION);
builder.put(AddConstraint.class, QueryType.DATA_DEFINITION);
builder.put(AlterColumnNotNull.class, QueryType.DATA_DEFINITION);
Expand Down
4 changes: 4 additions & 0 deletions presto-docs/src/main/sphinx/connector/iceberg.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,6 +1173,10 @@ Alter table operations are supported in the Iceberg connector::

ALTER TABLE iceberg.web.page_views DROP COLUMN location;

ALTER TABLE iceberg.web.page_views DROP BRANCH 'branch1';

ALTER TABLE iceberg.web.page_views DROP TAG 'tag1';

To add a new column as a partition column, identify the transform functions for the column.
The table is partitioned by the transformed value of the column::

Expand Down
10 changes: 10 additions & 0 deletions presto-docs/src/main/sphinx/sql/alter-table.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ Synopsis
ALTER TABLE [ IF EXISTS ] name ADD [ CONSTRAINT constraint_name ] { PRIMARY KEY | UNIQUE } ( { column_name [, ...] } ) [ { ENABLED | DISABLED } ] [ [ NOT ] RELY ] [ [ NOT ] ENFORCED } ]
ALTER TABLE [ IF EXISTS ] name DROP CONSTRAINT [ IF EXISTS ] constraint_name
ALTER TABLE [ IF EXISTS ] ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL
ALTER TABLE [ IF EXISTS ] name DROP BRANCH [ IF EXISTS ] branch_name
ALTER TABLE [ IF EXISTS ] name DROP TAG [ IF EXISTS ] tag_name

Description
-----------
Expand Down Expand Up @@ -89,6 +91,14 @@ Drop not null constraint from column ``zip`` in the ``users`` table::

ALTER TABLE users ALTER COLUMN zip DROP NOT NULL;

Drop branch ``branch1`` from the ``users`` table::

ALTER TABLE users DROP BRANCH 'branch1';

Drop tag ``tag1`` from the ``users`` table::

ALTER TABLE users DROP TAG 'tag1';

See Also
--------

Expand Down
10 changes: 10 additions & 0 deletions presto-hive/src/main/java/com/facebook/presto/hive/security/LegacyAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,16 @@ public void checkCanShowRoleGrants(ConnectorTransactionHandle transactionHandle,
{
}

@Override
public void checkCanDropBranch(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
{
}

@Override
public void checkCanDropTag(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
{
}

@Override
public void checkCanDropConstraint(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
{
Expand Down
38 changes: 38 additions & 0 deletions presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,13 @@
import static com.facebook.presto.spi.security.AccessDeniedException.denyCreateView;
import static com.facebook.presto.spi.security.AccessDeniedException.denyCreateViewWithSelect;
import static com.facebook.presto.spi.security.AccessDeniedException.denyDeleteTable;
import static com.facebook.presto.spi.security.AccessDeniedException.denyDropBranch;
import static com.facebook.presto.spi.security.AccessDeniedException.denyDropColumn;
import static com.facebook.presto.spi.security.AccessDeniedException.denyDropConstraint;
import static com.facebook.presto.spi.security.AccessDeniedException.denyDropRole;
import static com.facebook.presto.spi.security.AccessDeniedException.denyDropSchema;
import static com.facebook.presto.spi.security.AccessDeniedException.denyDropTable;
import static com.facebook.presto.spi.security.AccessDeniedException.denyDropTag;
import static com.facebook.presto.spi.security.AccessDeniedException.denyDropView;
import static com.facebook.presto.spi.security.AccessDeniedException.denyGrantRoles;
import static com.facebook.presto.spi.security.AccessDeniedException.denyGrantTablePrivilege;
Expand Down Expand Up @@ -269,6 +271,42 @@ public void checkCanDropColumn(ConnectorTransactionHandle transaction, Connector
}
}

@Override
public void checkCanDropBranch(ConnectorTransactionHandle transaction, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
{
MetastoreContext metastoreContext = new MetastoreContext(
identity, context.getQueryId().getId(),
context.getClientInfo(),
context.getClientTags(),
context.getSource(),
Optional.empty(),
false,
HiveColumnConverterProvider.DEFAULT_COLUMN_CONVERTER_PROVIDER,
context.getWarningCollector(),
context.getRuntimeStats());
if (!isTableOwner(transaction, identity, metastoreContext, tableName)) {
denyDropBranch(tableName.toString());
}
}

@Override
public void checkCanDropTag(ConnectorTransactionHandle transaction, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
Copy link
Contributor

@ZacBlanco ZacBlanco Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder about the granularity of these methods. In other implementation (e.g. spark?) at what granularity do they enforce the ability to do CRUD operations on tags and branches?

I'm thinking about a few cases

  1. A group or user(s) can only access a certain set of branches or tags
  2. A group or user(s) can only create branches starting from a specific branch
  3. A group or user(s) can create tags

I know we're only implementing DROP but I want to understand the whole story for access control around branches and tags. Would we ever need to pass the branch/tag to these methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the granularity of access control methods in Spark for CRUD operations on Iceberg tags and branches is limited by Spark's integration with external systems (such as file systems, catalogs, and security frameworks like Apache Ranger).
For example, Ranger policies can define access controls at the table level, which could be extended to manage specific branches or tag-based access.
And like for cloud-based catalogs like AWS Glue, you can control access to Iceberg metadata (branches and tags) via IAM policies that grant or restrict specific operations.

Copy link
Contributor

@ZacBlanco ZacBlanco Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the info. Would the parameters passed here as context have enough information for us to act at a similar granularity? I don't see anything in the method parameters that contains the branch name which I assume we would need to perform access control at a similar level.

Copy link
Member Author

@agrawalreetika agrawalreetika Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to discuss around this, Systems like Ranger can define access controls at the table level, column level. So in this case I think access of drop branch & tags could be table based. As per I can think of branch & tag level policies then has to be maintained on engine side if we introduce branch name / tag name in here?

Copy link
Member Author

@agrawalreetika agrawalreetika Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tdcmeehan What do you think about access control for branches and tags? Would be based on the parent table itself or based on the tags/branches?
My thinking was that since no policies are enforced based on branch/tags via security frameworks, should this honor the same access policies as table? Or if we don't even need access control exposed for dropTag & dropBranch?

{
MetastoreContext metastoreContext = new MetastoreContext(
identity, context.getQueryId().getId(),
context.getClientInfo(),
context.getClientTags(),
context.getSource(),
Optional.empty(),
false,
HiveColumnConverterProvider.DEFAULT_COLUMN_CONVERTER_PROVIDER,
context.getWarningCollector(),
context.getRuntimeStats());
if (!isTableOwner(transaction, identity, metastoreContext, tableName)) {
denyDropTag(tableName.toString());
}
}

@Override
public void checkCanDropConstraint(ConnectorTransactionHandle transaction, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
{
Expand Down
12 changes: 12 additions & 0 deletions ...o-hive/src/main/java/com/facebook/presto/hive/security/SystemTableAwareAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,18 @@ public void checkCanShowRoleGrants(ConnectorTransactionHandle transactionHandle,
delegate.checkCanShowRoleGrants(transactionHandle, identity, context, catalogName);
}

@Override
public void checkCanDropBranch(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
{
delegate.checkCanDropBranch(transactionHandle, identity, context, tableName);
}

@Override
public void checkCanDropTag(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
{
delegate.checkCanDropTag(transactionHandle, identity, context, tableName);
}

@Override
public void checkCanDropConstraint(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
{
Expand Down
33 changes: 33 additions & 0 deletions presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@
import static com.facebook.presto.iceberg.optimizer.IcebergPlanOptimizer.getEnforcedColumns;
import static com.facebook.presto.iceberg.util.StatisticsUtil.calculateBaseTableStatistics;
import static com.facebook.presto.iceberg.util.StatisticsUtil.calculateStatisticsConsideringLayout;
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED;
import static com.facebook.presto.spi.statistics.TableStatisticType.ROW_COUNT;
import static com.google.common.base.Verify.verify;
Expand Down Expand Up @@ -636,6 +637,38 @@ public void rollback()
// TODO: cleanup open transaction
}

@Override
public void dropBranch(ConnectorSession session, ConnectorTableHandle tableHandle, String branchName, boolean branchExists)
{
IcebergTableHandle icebergTableHandle = (IcebergTableHandle) tableHandle;
verify(icebergTableHandle.getIcebergTableName().getTableType() == DATA, "only the data table can have branch dropped");
Table icebergTable = getIcebergTable(session, icebergTableHandle.getSchemaTableName());
if (icebergTable.refs().containsKey(branchName) && icebergTable.refs().get(branchName).isBranch()) {
icebergTable.manageSnapshots().removeBranch(branchName).commit();
}
else {
if (!branchExists) {
throw new PrestoException(NOT_FOUND, format("Branch %s doesn't exist in table %s", branchName, icebergTableHandle.getSchemaTableName().getTableName()));
}
}
}

@Override
public void dropTag(ConnectorSession session, ConnectorTableHandle tableHandle, String tagName, boolean tagExists)
{
IcebergTableHandle icebergTableHandle = (IcebergTableHandle) tableHandle;
verify(icebergTableHandle.getIcebergTableName().getTableType() == DATA, "only the data table can have tag dropped");
Table icebergTable = getIcebergTable(session, icebergTableHandle.getSchemaTableName());
if (icebergTable.refs().containsKey(tagName) && icebergTable.refs().get(tagName).isTag()) {
icebergTable.manageSnapshots().removeTag(tagName).commit();
}
else {
if (!tagExists) {
throw new PrestoException(NOT_FOUND, format("Tag %s doesn't exist in table %s", tagName, icebergTableHandle.getSchemaTableName().getTableName()));
}
}
}

@Override
public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column)
{
Expand Down
54 changes: 54 additions & 0 deletions presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1633,6 +1633,60 @@ public void testDecimal(boolean decimalVectorReaderEnabled)
}
}

@Test
public void testDropBranch()
agrawalreetika marked this conversation as resolved.
Show resolved Hide resolved
{
assertUpdate("CREATE TABLE test_table_branch (id1 BIGINT, id2 BIGINT)");
assertUpdate("INSERT INTO test_table_branch VALUES (0, 00), (1, 10)", 2);

Table icebergTable = loadTable("test_table_branch");
icebergTable.manageSnapshots().createBranch("testBranch1").commit();

assertUpdate("INSERT INTO test_table_branch VALUES (2, 30), (3, 30)", 2);
icebergTable.manageSnapshots().createBranch("testBranch2").commit();
assertUpdate("INSERT INTO test_table_branch VALUES (4, 40), (5, 50)", 2);
assertEquals(icebergTable.refs().size(), 3);

assertQuery("SELECT count(*) FROM test_table_branch FOR SYSTEM_VERSION AS OF 'testBranch1'", "VALUES 2");
assertQuery("SELECT count(*) FROM test_table_branch FOR SYSTEM_VERSION AS OF 'testBranch2'", "VALUES 4");
assertQuery("SELECT count(*) FROM test_table_branch FOR SYSTEM_VERSION AS OF 'main'", "VALUES 6");

assertQuerySucceeds("ALTER TABLE test_table_branch DROP BRANCH 'testBranch1'");
icebergTable = loadTable("test_table_branch");
assertEquals(icebergTable.refs().size(), 2);
assertQueryFails("ALTER TABLE test_table_branch DROP BRANCH 'testBranchNotExist'", "Branch testBranchNotExist doesn't exist in table test_table_branch");
assertQuerySucceeds("ALTER TABLE test_table_branch DROP BRANCH IF EXISTS 'testBranch2'");
assertQuerySucceeds("ALTER TABLE test_table_branch DROP BRANCH IF EXISTS 'testBranchNotExist'");
assertQuerySucceeds("DROP TABLE test_table_branch");
}

@Test
public void testDropTag()
agrawalreetika marked this conversation as resolved.
Show resolved Hide resolved
{
assertUpdate("CREATE TABLE test_table_tag (id1 BIGINT, id2 BIGINT)");
assertUpdate("INSERT INTO test_table_tag VALUES (0, 00), (1, 10)", 2);

Table icebergTable = loadTable("test_table_tag");
icebergTable.manageSnapshots().createTag("testTag1", icebergTable.currentSnapshot().snapshotId()).commit();

assertUpdate("INSERT INTO test_table_tag VALUES (2, 30), (3, 30)", 2);
icebergTable.manageSnapshots().createTag("testTag2", icebergTable.currentSnapshot().snapshotId()).commit();
assertUpdate("INSERT INTO test_table_tag VALUES (4, 40), (5, 50)", 2);
assertEquals(icebergTable.refs().size(), 3);

assertQuery("SELECT count(*) FROM test_table_tag FOR SYSTEM_VERSION AS OF 'testTag1'", "VALUES 2");
assertQuery("SELECT count(*) FROM test_table_tag FOR SYSTEM_VERSION AS OF 'testTag2'", "VALUES 4");
assertQuery("SELECT count(*) FROM test_table_tag FOR SYSTEM_VERSION AS OF 'main'", "VALUES 6");

assertQuerySucceeds("ALTER TABLE test_table_tag DROP TAG 'testTag1'");
icebergTable = loadTable("test_table_tag");
assertEquals(icebergTable.refs().size(), 2);
assertQueryFails("ALTER TABLE test_table_tag DROP TAG 'testTagNotExist'", "Tag testTagNotExist doesn't exist in table test_table_tag");
assertQuerySucceeds("ALTER TABLE test_table_tag DROP TAG IF EXISTS 'testTag2'");
assertQuerySucceeds("ALTER TABLE test_table_tag DROP TAG IF EXISTS 'testTagNotExist'");
assertQuerySucceeds("DROP TABLE test_table_tag");
}

@Test
public void testRefsTable()
{
Expand Down
77 changes: 77 additions & 0 deletions presto-main/src/main/java/com/facebook/presto/execution/DropBranchTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.facebook.presto.execution;

import com.facebook.presto.Session;
import com.facebook.presto.common.QualifiedObjectName;
import com.facebook.presto.metadata.Metadata;
import com.facebook.presto.spi.ConnectorId;
import com.facebook.presto.spi.MaterializedViewDefinition;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.TableHandle;
import com.facebook.presto.spi.WarningCollector;
import com.facebook.presto.spi.security.AccessControl;
import com.facebook.presto.sql.analyzer.SemanticException;
import com.facebook.presto.sql.tree.DropBranch;
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.transaction.TransactionManager;
import com.google.common.util.concurrent.ListenableFuture;

import java.util.List;
import java.util.Optional;

import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName;
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_TABLE;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.NOT_SUPPORTED;
import static com.google.common.util.concurrent.Futures.immediateFuture;

public class DropBranchTask
implements DDLDefinitionTask<DropBranch>
{
@Override
public String getName()
{
return "DROP BRANCH";
}

@Override
public ListenableFuture<?> execute(DropBranch statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List<Expression> parameters, WarningCollector warningCollector)
{
QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getTableName());
Optional<TableHandle> tableHandleOptional = metadata.getMetadataResolver(session).getTableHandle(tableName);

if (!tableHandleOptional.isPresent()) {
if (!statement.isTableExists()) {
throw new SemanticException(MISSING_TABLE, statement, "Table '%s' does not exist", tableName);
}
return immediateFuture(null);
}

Optional<MaterializedViewDefinition> optionalMaterializedView = metadata.getMetadataResolver(session).getMaterializedView(tableName);
if (optionalMaterializedView.isPresent()) {
if (!statement.isTableExists()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we throw an error in cases when if exists is present in the query and it is a materialized view? If drop branch is just not supported for MVs, ideally error should be thrown irrespective of exists check?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternate way of handling this can be introducing an enum Type in DropBranch and have only TABLE as the supported type.

throw new SemanticException(NOT_SUPPORTED, statement, "'%s' is a materialized view, and drop branch is not supported", tableName);
}
return immediateFuture(null);
}

ConnectorId connectorId = metadata.getCatalogHandle(session, tableName.getCatalogName())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the variable connectorId is not getting used and can be removed. Also I have refactored and extracted this piece of code in MetadataUtil class in this commit - dec2952.
So we can probably do a similar change here as well so that all the Task classes use the same methods?i

.orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog does not exist: " + tableName.getCatalogName()));
accessControl.checkCanDropBranch(session.getRequiredTransactionId(), session.getIdentity(), session.getAccessControlContext(), tableName);

metadata.dropBranch(session, tableHandleOptional.get(), statement.getBranchName(), statement.isBranchExists());
return immediateFuture(null);
}
}
Loading
Loading