Block schema field drop if it is reference by an active partition or sort field #2410
Conversation
…or sort order fields reference schema fields
gabeiglio
force-pushed
the
partition-spec-validation
branch
from
d4928bd to
81732d4
Compare
| @pytest.mark.integration | ||
| @pytest.mark.parametrize("test_catalog", CATALOGS) | ||
| def test_incompatible_partitioned_schema_evolution( | ||
| test_catalog: Catalog, test_schema: Schema, test_partition_spec: PartitionSpec, database_name: str, table_name: str | ||
| ) -> None: | ||
| if isinstance(test_catalog, HiveCatalog): | ||
| pytest.skip("HiveCatalog does not support schema evolution") | ||
|
|
||
| identifier = (database_name, table_name) | ||
| test_catalog.create_namespace(database_name) | ||
| table = test_catalog.create_table(identifier, test_schema, partition_spec=test_partition_spec) | ||
| assert test_catalog.table_exists(identifier) | ||
|
|
||
| with pytest.raises(ValidationError): | ||
| with table.update_schema() as update: | ||
| update.delete_column("VendorID") | ||
|
|
||
| # Assert column was not dropped | ||
| assert "VendorID" in table.schema().column_names | ||
|
|
||
| with table.transaction() as transaction: | ||
| with transaction.update_spec() as spec_update: | ||
| spec_update.remove_field("VendorID") | ||
|
|
||
| with transaction.update_schema() as schema_update: | ||
| schema_update.delete_column("VendorID") | ||
|
|
||
| assert table.spec() == PartitionSpec(PartitionField(2, 1001, DayTransform(), "tpep_pickup_day"), spec_id=1) | ||
| assert table.schema() == Schema(NestedField(2, "tpep_pickup_datetime", TimestampType(), False)) |
There was a problem hiding this comment.
Part of me wonders if it makes sense to test this here? Like yes we should be testing commit_table generally with different types of updates on all catalogs but this test seems maybe a little further away? Not going to turn down more tests but just a thought!
There was a problem hiding this comment.
I see your concern on adding catalog tests for every small edge case. I do see catalog tests as a way to check any action that writes/reads metadata.json so this fitted my current description, but Im open to get some consensus on what should be tested here 👍🏼
| return f"SortOrder({fields}order_id={self.order_id})" | ||
|
|
||
| @staticmethod | ||
| def check_compatibility(sort_order: SortOrder, schema: Schema) -> None: |
There was a problem hiding this comment.
It strikes me as a bit of an anti-pattern to have a static method whose first argument is of the type of its class, could we make this a normal method?
Maybe renaming to check_compatible in that case would be clearer
| return path | ||
|
|
||
| @staticmethod | ||
| def check_compatibility(partition_spec: PartitionSpec, schema: Schema, allow_missing_fields: bool = False) -> None: |
3 participants
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 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