Add DDL statements to drop branches and tags #23614

agrawalreetika · 2024-09-10T15:17:54Z

Description

Add DDL statements to drop branches and tags

Motivation and Context

Resolves #22028

Impact

Resolves #22028

SQL support for dropping a branch from a table :

ALTER TABLE users DROP BRANCH 'branch1';

SQL support for dropping a tag from a table :

ALTER TABLE users DROP TAG 'tag1';

Test Plan

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* DDL support for dropping a branch from a table. :pr:`23614`
* DDL support for dropping a tag from a table. :pr:`23614`

Iceberg Connector Changes
* Support for dropping a branch from an Iceberg table. :pr:`23614`
* Support for dropping a branch from an Iceberg table.  :pr:`23614`

github-actions · 2024-09-10T15:18:54Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff c00f8af...6a679d8.

Notify	File(s)
@aditi-pandit	presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@elharo	presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@kaikalur	presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@rschlussel	presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4

steveburnett

LGTM! (docs)

Pull branch, local docs build, looks good. Thanks!

ZacBlanco

Some high level feedback. Most of the code looks good. I will take a closer look on 2nd pass.

Also, I wanted to bring a thought I had about parser extensions. Since many connectors will not support this branching and tagging, I was thinking that maybe we ought to consider designing a SQL syntax plugin extension interface. Spark allows custom syntax extensions through implementing some set of interfaces or bringing your own parser. The upstream iceberg project now maintains their own spark SQL syntax extensions.

I'm not proposing we need that for this PR, but maybe it's something we should start thinking about if connectors start adding more radically different features that would be best left to some syntax extensions/optional plugins, especially for things outside the SQL specification.

ZacBlanco · 2024-09-11T03:14:03Z

presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java

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


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

A group or user(s) can only access a certain set of branches or tags

A group or user(s) can only create branches starting from a specific branch

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?

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.

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.

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?

@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?

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java

agrawalreetika · 2024-09-13T04:57:43Z

@ZacBlanco Can you please take another pass?

hantangwangd

The whole change overall looks good to me. Some little nits, and one problem for discussing about the behavior of if exists on branch and tag.

hantangwangd · 2024-09-17T17:10:41Z

presto-parser/src/main/java/com/facebook/presto/sql/tree/DropBranch.java

+        return tableExists;
+    }
+
+    public boolean isbranchExists()


Suggested change

public boolean isbranchExists()

public boolean isBranchExists()

hantangwangd · 2024-09-17T17:14:31Z

presto-parser/src/main/java/com/facebook/presto/sql/tree/DropBranch.java

+    @Override
+    public int hashCode()
+    {
+        return Objects.hash(tableName, tableExists, branchName);


Did you miss branchExists?

hantangwangd · 2024-09-17T17:14:44Z

presto-parser/src/main/java/com/facebook/presto/sql/tree/DropBranch.java

+        return Objects.equals(tableName, that.tableName) &&
+                Objects.equals(branchName, that.branchName) &&
+                Objects.equals(tableExists, that.tableExists) &&
+                Objects.equals(branchExists, branchExists);


Suggested change

Objects.equals(branchExists, branchExists);

Objects.equals(branchExists, that.branchExists);

hantangwangd · 2024-09-17T17:15:37Z

presto-parser/src/main/java/com/facebook/presto/sql/tree/DropTag.java

+        return Objects.equals(tableName, that.tableName) &&
+                Objects.equals(tagName, that.tagName) &&
+                Objects.equals(tableExists, that.tableExists) &&
+                Objects.equals(tagExists, tagExists);


Suggested change

Objects.equals(tagExists, tagExists);

Objects.equals(tagExists, that.tagExists);

hantangwangd · 2024-09-17T17:16:15Z

presto-parser/src/main/java/com/facebook/presto/sql/tree/DropTag.java

+    @Override
+    public int hashCode()
+    {
+        return Objects.hash(tableName, tableExists, tagName);


Missed tagExists?

hantangwangd · 2024-09-17T17:27:34Z

presto-main/src/main/java/com/facebook/presto/execution/DropTagTask.java

+        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()) {
+                throw new SemanticException(NOT_SUPPORTED, statement, "'%s' is a materialized view, and drop tag is not supported", tableName);
+            }
+            return immediateFuture(null);
+        }
+
+        ConnectorId connectorId = metadata.getCatalogHandle(session, tableName.getCatalogName())
+                .orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog does not exist: " + tableName.getCatalogName()));
+        accessControl.checkCanDropTag(session.getRequiredTransactionId(), session.getIdentity(), session.getAccessControlContext(), tableName);
+
+        metadata.dropTag(session, tableHandleOptional.get(), Optional.of(statement.getTagName().toString()));


The same as above, did not consider the if exists flag for tag.

hantangwangd · 2024-09-17T17:30:09Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java

+        else {
+            throw new PrestoException(NOT_FOUND, format("Branch %s doesn't exist in table %s", branchName.get(), icebergTableHandle.getSchemaTableName().getTableName()));
+        }


Should we take the value of if exists into consider here?

hantangwangd · 2024-09-17T17:30:39Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java

+        else {
+            throw new PrestoException(NOT_FOUND, format("Tag %s doesn't exist in table %s", tagName.get(), icebergTableHandle.getSchemaTableName().getTableName()));
+        }


The same as above, should we take the value of if exists into consider here?

hantangwangd · 2024-09-17T17:32:14Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java

+        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'");
+        assertQueryFails("ALTER TABLE test_table_branch DROP BRANCH IF EXISTS 'testBranchNotExist'", "Branch testBranchNotExist doesn't exist in table test_table_branch");


Is this a correct behavior? As I understand, this statement should not fail.

hantangwangd · 2024-09-17T17:32:58Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java

+        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'");
+        assertQueryFails("ALTER TABLE test_table_tag DROP TAG IF EXISTS 'testTagNotExist'", "Tag testTagNotExist doesn't exist in table test_table_tag");


The same as above, should this statement fail?

agrawalreetika requested review from steveburnett, elharo, hantangwangd, ZacBlanco, a team, jaystarshot, feilong-liu and ClarenceThreepwood as code owners September 10, 2024 15:17

agrawalreetika requested a review from presto-oss September 10, 2024 15:17

agrawalreetika self-assigned this Sep 10, 2024

steveburnett previously approved these changes Sep 10, 2024

View reviewed changes

tdcmeehan self-assigned this Sep 10, 2024

agrawalreetika dismissed steveburnett’s stale review via ee54cd3 September 10, 2024 19:20

agrawalreetika force-pushed the iceberg-tag-branch-drop branch from 3fe4d56 to ee54cd3 Compare September 10, 2024 19:20

ZacBlanco requested changes Sep 11, 2024

View reviewed changes

agrawalreetika force-pushed the iceberg-tag-branch-drop branch 2 times, most recently from 8f4d3fa to 1e47c05 Compare September 11, 2024 21:18

hantangwangd requested changes Sep 17, 2024

View reviewed changes

agrawalreetika added 4 commits September 20, 2024 01:27

Add DROP BRANCH sql support

69f980f

Add support for drop branch for Iceberg Table

4b86b9b

Add DROP TAG sql support

fdf0c9e

Add support for drop tag for Iceberg Table

6a679d8

agrawalreetika force-pushed the iceberg-tag-branch-drop branch from 1e47c05 to 6a679d8 Compare September 19, 2024 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DDL statements to drop branches and tags #23614

Add DDL statements to drop branches and tags #23614

agrawalreetika commented Sep 10, 2024 •

edited

Loading

github-actions bot commented Sep 10, 2024 •

edited

Loading

steveburnett left a comment

ZacBlanco left a comment •

edited

Loading

ZacBlanco Sep 11, 2024 •

edited

Loading

agrawalreetika Sep 11, 2024

ZacBlanco Sep 11, 2024 •

edited

Loading

agrawalreetika Sep 11, 2024 •

edited

Loading

agrawalreetika Sep 13, 2024 •

edited

Loading

agrawalreetika commented Sep 13, 2024

hantangwangd left a comment

hantangwangd Sep 17, 2024

hantangwangd Sep 17, 2024

hantangwangd Sep 17, 2024

hantangwangd Sep 17, 2024

hantangwangd Sep 17, 2024

hantangwangd Sep 17, 2024

hantangwangd Sep 17, 2024

hantangwangd Sep 17, 2024

hantangwangd Sep 17, 2024

hantangwangd Sep 17, 2024

	public boolean isbranchExists()
	public boolean isBranchExists()

	Objects.equals(branchExists, branchExists);
	Objects.equals(branchExists, that.branchExists);

	Objects.equals(tagExists, tagExists);
	Objects.equals(tagExists, that.tagExists);

Add DDL statements to drop branches and tags #23614

Are you sure you want to change the base?

Add DDL statements to drop branches and tags #23614

Conversation

agrawalreetika commented Sep 10, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

github-actions bot commented Sep 10, 2024 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

ZacBlanco left a comment • edited Loading

Choose a reason for hiding this comment

ZacBlanco Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacBlanco Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

agrawalreetika Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

agrawalreetika Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

agrawalreetika commented Sep 13, 2024

hantangwangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawalreetika commented Sep 10, 2024 •

edited

Loading

github-actions bot commented Sep 10, 2024 •

edited

Loading

ZacBlanco left a comment •

edited

Loading

ZacBlanco Sep 11, 2024 •

edited

Loading

ZacBlanco Sep 11, 2024 •

edited

Loading

agrawalreetika Sep 11, 2024 •

edited

Loading

agrawalreetika Sep 13, 2024 •

edited

Loading