Skip to content

Conversation

@szehon-ho
Copy link
Member

@szehon-ho szehon-ho commented Jul 4, 2025

What changes were proposed in this pull request?

Add a new ExternalCatalog and SessionCatalog API alterTableSchema that will supersede alterTableDataSchema.

Why are the changes needed?

Because ExternalCatalog::alterTableDataSchema takes dataSchema only (without partition columns), we lost the context of the partition column. This will make it impossible for us to support column order where partition column are not at the end.

See #51342 for context

More generally, this is a better intuitive API than alterTableDataSchema, because the caller no longer needs to strip out partition columns. Also, it is not immediately intuitive that data schema means without partition columns.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing test, move test for alterTableDataSchema to the new API

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

No

@szehon-ho szehon-ho force-pushed the alter_table_schema branch from 929adf2 to 29f253b Compare July 4, 2025 05:36
}
if (changes.exists(_.isInstanceOf[TableChange.ColumnChange])) {
catalog.alterTableDataSchema(ident.asTableIdentifier, schema)
catalog.alterTableSchema(ident.asTableIdentifier, schema)
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, so the schema here is already the full schema

Copy link
Member Author

@szehon-ho szehon-ho Jul 4, 2025

Choose a reason for hiding this comment

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

yea i noticed and had a pr to fix that #51372 but we can fix both if we do this one.

@cloud-fan
Copy link
Contributor

thanks, merging to master!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants