-
Notifications
You must be signed in to change notification settings - Fork 412
Block schema field drop if it is reference by an active partition or sort field #2410
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.
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
base: main
Are you sure you want to change the base?
Conversation
d4928bd to
81732d4
Compare
Anton-Tarazi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, left a minor comment :)
…or sort order fields reference schema fields
81732d4 to
7e47009
Compare
|
@Anton-Tarazi thanks for the review (this PR went off my radar) I have rebased and applied recommended changes |
|
|
||
| for field in self.fields: | ||
| source_field = schema_fields.get(field.source_id) | ||
| if allow_missing_fields and source_field: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be:
if allow_missing_fields and source_field is None:
continue
| while parent_id: | ||
| parent_type = schema.find_type(parent_id) | ||
| if not parent_type.is_struct: | ||
| raise ValidationError("Invalid partition field parent: %s", parent_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we also use f string here to align with others
Closes #2166
Rationale for this change
We should block when an user wants to drop a column if that column is being referenced by either a active partition spec or sort order field.
Are these changes tested?
Yes, I added unit tests for every incompatible schema change in partitions and sort orders. Also added two new integration tests in
test_catalogto test for this scenarioAre there any user-facing changes?
No