Skip to content

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Aug 1, 2021

This fixes partition field ID assignment for REPLACE TABLE operations. Table replacement calls TableMetadata.buildReplacement that is responsible for merging the existing table's metadata with table metadata passed to the CREATE OR REPLACE TABLE DDL. When the schema and partition spec are created in this path, the field IDs from the table are not known, so the spec and schema's IDs are consistent but reuse IDs that are already assigned in the table. Schema field IDs are already reassigned, but partition field IDs were conflicting up until now.

This reassigns partition field IDs by first reusing existing IDs in the table and then by assigning new IDs.

Addresses this comment: #2089 (comment)

This is based on #2284. I've added Jun as a co-author in the commit. Thanks @jun-he!

Closes #2284.

@github-actions github-actions bot added the core label Aug 1, 2021
@rdblue rdblue requested a review from danielcweeks August 1, 2021 23:20
@rdblue rdblue force-pushed the fix-partition-field-ids-in-replace-table branch from 52e4a9f to 15fa6a2 Compare August 1, 2021 23:21
@rdblue rdblue force-pushed the fix-partition-field-ids-in-replace-table branch 2 times, most recently from 9f0b54b to fdba13b Compare August 2, 2021 15:14
@rdblue rdblue force-pushed the fix-partition-field-ids-in-replace-table branch from fdba13b to f996bff Compare August 2, 2021 20:41
@github-actions github-actions bot added the hive label Aug 2, 2021
Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

Looks good to me. The failed Hive tests seem to be wrong, so we can just fix those tests.

@rdblue rdblue force-pushed the fix-partition-field-ids-in-replace-table branch from f996bff to 15c233e Compare August 2, 2021 23:18
@rdblue
Copy link
Contributor Author

rdblue commented Aug 2, 2021

Thanks for reviewing, @jackye1995! I'm updating the tests as this catches more. It's just a few places that were changed by the behavior fix.

Copy link
Collaborator

@jun-he jun-he left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @rdblue !

@rdblue rdblue merged commit a42a546 into apache:master Aug 3, 2021
@aokolnychyi
Copy link
Contributor

Late +1 from me too.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants