Skip to content

[SPARK-51771][SQL] Add DSv2 APIs for ALTER TABLE ADD/DROP CONSTRAINT - #50561

Closed
gengliangwang wants to merge 8 commits into
apache:masterfrom
gengliangwang:alterConstraint
Closed

[SPARK-51771][SQL] Add DSv2 APIs for ALTER TABLE ADD/DROP CONSTRAINT#50561
gengliangwang wants to merge 8 commits into
apache:masterfrom
gengliangwang:alterConstraint

Conversation

@gengliangwang

@gengliangwanggengliangwang commented Apr 10, 2025

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This PR adds the following DSv2 TableChange as per SPIP doc:

  • AddConstraint
  • DropConstraint

Why are the changes needed?

For constraints support in Spark

Does this PR introduce any user-facing change?

No

How was this patch tested?

New unit tests

Was this patch authored or co-authored using generative AI tooling?

No

@gengliangwang

gengliangwang commented Apr 10, 2025

Copy link
Copy Markdown
MemberAuthor

cc @aokolnychyi as well

gengliangwang
gengliangwang commented Apr 10, 2025
View reviewed changes
Comment thread...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala

case a: AlterTableCommand if a.table.resolved =>
val table = a.table.asInstanceOf[ResolvedTable]
ResolveTableConstraints.validateCatalogForTableChange(

@gengliangwanggengliangwangApr 10, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Note: there will be new tests for this one when the alter table with constraint is supported end-to-end.

aokolnychyi
aokolnychyi reviewed Apr 11, 2025
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala
val newConstraint = add.getConstraint
val existingConstraint = findExistingConstraint(newConstraint.name)
if (existingConstraint.isDefined) {
throw new AnalysisException(

@aokolnychyiaokolnychyiApr 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When do we define dedicated error methods in classes like QueryComplicationErrors? Do we encourage external connectors to use built-in error classes in Spark?

@gengliangwanggengliangwangApr 11, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

All the spark errors are with error classes. This is a utility method for external developers to use directly.

Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
/** A TableChange to alter table and add a constraint. */
final class AddConstraint implements TableChange {
private final Constraint constraint;
private final boolean validate;

@aokolnychyiaokolnychyiApr 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

During the discussion, the SPIP design evolved to be like this:

class AddConstraint implements TableChange {
private final Constraint constraint;
private final String validatedTableVersion; // if known and was validated …
}

If Spark validated the constraint, it should set the validation status in the constraint to VALID. The table version will be there, if known.

interface Table {
…
// reports the current table version, if supported
default String currentVersion() { return null; }
…
}

@gengliangwanggengliangwangApr 11, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

TBH I am a bit confused on this one. Shall we make it simple and let the external table catalog to track the validatedTableVersion?

@aokolnychyiaokolnychyiApr 12, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The problem is that Spark will construct a scan to do the validation. We need a way to pass which version of the table was scanned/validated back to the connector so that the connector can check changes that happened concurrently.

Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
szehon-ho
szehon-ho reviewed Apr 12, 2025
View reviewed changes
Comment threadcommon/utils/src/main/resources/error/error-conditions.json
},
"CONSTRAINT_ALREADY_EXISTS" : {
"message" : [
"Constraint '<constraintName>' already exists. Please delete the existing constraint first.",

@szehon-hoszehon-hoApr 11, 2025

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.

is constraint name scoped to table? wondering should we also put tableName like the other message?

@gengliangwanggengliangwangApr 12, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This error is from alter table statement, which is usually simple and straightforward. I prefer keeping it as it is now

viirya
viirya reviewed Apr 12, 2025
View reviewed changes
Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
viirya
viirya reviewed Apr 12, 2025
View reviewed changes
Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
viirya
viirya reviewed Apr 12, 2025
View reviewed changes
Comment threadsql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java Outdated
@Udaundo1973

Udaundo1973 commented Apr 14, 2025

Copy link
Copy Markdown

James Channel

cloud-fan
cloud-fan reviewed Apr 15, 2025
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala
errorClass = "CONSTRAINT_ALREADY_EXISTS",
messageParameters =
Map("constraintName" -> existingConstraint.get.name,
"oldConstraint" -> existingConstraint.get.toDDL))

@cloud-fancloud-fanApr 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we really need to display the old constraints? Are there security issues?

@gengliangwanggengliangwangApr 15, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It would be easier for users to know what is in the old constraint without running a describe table command and find details.
Constraints should be simple. We will show all the constraints in describe table command anyway.

cloud-fan
cloud-fan reviewed Apr 15, 2025
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala Outdated
/**
* Apply Table Constraints changes to an existing table and return the result.
*/
def applyConstraintChanges(

@cloud-fancloud-fanApr 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is not apply, but more like collect constraints.

@gengliangwanggengliangwangApr 15, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

good point. renamed

@gengliangwang

gengliangwang commented Apr 15, 2025

Copy link
Copy Markdown
MemberAuthor

@viirya@aokolnychyi@cloud-fan Thanks for the review. Merging this one to master. If there are more comments, I can resolve them in follow-up PRs

@dongjoon-hyundongjoon-hyun mentioned this pull request Nov 18, 2025
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan left review comments
@szehon-hoszehon-hoszehon-ho left review comments
@viiryaviiryaviirya approved these changes
@aokolnychyiaokolnychyiaokolnychyi approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gengliangwang@Udaundo1973@viirya@cloud-fan@aokolnychyi@szehon-ho