Remove `initial_change` when CreateTableTransaction apply table updates on an empty metadata #1219

HonahX · 2024-10-06T08:28:00Z

Fixes #864

Remove initial_change as described in Kevin's #950. This PR relies on Pydantic model's model_construct to create a model without validation. This gives us the flexibility to upgrade from TableMetadataV1 to TableMetadataV2 without worrying about validators terminating the process because of some incomplete fields.

kevinjqliu

thanks for the PR and adding the deprecation for initial_change.

I went down a deep rabbit hole on initial_change, the default values for TableMetadata, and how table updates are applied.
Let me clean up my notes and I'll post it here for reference

kevinjqliu

Thanks for the PR @HonahX. I like this solution. I've verified using the original issue's environment and it works.

Do you mind also removing these instances of initial_change?

iceberg-python/pyiceberg/table/__init__.py

Lines 706 to 721 in 9a6a9a1

    
               AddSchemaUpdate(schema_=schema, last_column_id=schema.highest_field_id, initial_change=True), 
        
               SetCurrentSchemaUpdate(schema_id=-1), 
        
           ) 
        
           spec: PartitionSpec = table_metadata.spec() 
        
           if spec.is_unpartitioned(): 
        
               self._updates += (AddPartitionSpecUpdate(spec=UNPARTITIONED_PARTITION_SPEC, initial_change=True),) 
        
           else: 
        
               self._updates += (AddPartitionSpecUpdate(spec=spec, initial_change=True),) 
        
           self._updates += (SetDefaultSpecUpdate(spec_id=-1),) 
        
           sort_order: Optional[SortOrder] = table_metadata.sort_order_by_id(table_metadata.default_sort_order_id) 
        
           if sort_order is None or sort_order.is_unsorted: 
        
               self._updates += (AddSortOrderUpdate(sort_order=UNSORTED_SORT_ORDER, initial_change=True),) 
        
           else: 
        
               self._updates += (AddSortOrderUpdate(sort_order=sort_order, initial_change=True),)

Also, should we remove the unrelated changes from this PR? such as the library version upgrade/etc.

dev/Dockerfile

kevinjqliu · 2024-10-29T22:24:18Z

I want to get this PR in before the next release, please let me know if there's anything I can do to move it along!

HonahX · 2024-10-30T05:26:36Z

@kevinjqliu Thanks for reviewing this! I've updated the PR. It will be great to include this in the next release.

kevinjqliu

LGTM!

Fokko

Looks good, thanks for catching this @kevinjqliu and fixing this @HonahX

Fokko · 2024-11-01T12:22:53Z

pyiceberg/table/update/__init__.py

@@ -267,11 +285,10 @@ def _(
    elif update.format_version == base_metadata.format_version:
        return base_metadata

-    updated_metadata_data = copy(base_metadata.model_dump())
-    updated_metadata_data["format-version"] = update.format_version
+    updated_metadata = base_metadata.model_copy(update={"format_version": update.format_version})


Fokko · 2024-11-01T12:24:40Z

pyiceberg/table/update/__init__.py

@@ -90,7 +90,13 @@ class AddSchemaUpdate(IcebergBaseModel):
    # This field is required: https://github.com/apache/iceberg/pull/7445
    last_column_id: int = Field(alias="last-column-id")

-    initial_change: bool = Field(default=False, exclude=True)
+    initial_change: bool = Field(


It is clean to go through the validation cycle, but I would be surprised if anyone would be relying on these properties

kevinjqliu · 2024-11-01T18:42:23Z

Thanks @HonahX for the fix and @Fokko for the review! One step closer to 0.8.0!

…es on an empty metadata (apache#1219) * make table metadata without validaiton * update deletes test * remove info * add deprecation message * revert lib version updates * remove initial_changes usage in code * move test to integration * fix typo * update error string

HonahX added 4 commits October 5, 2024 20:55

make table metadata without validaiton

c936c41

update deletes test

07ac53c

remove info

b8adf4d

add deprecation message

6c98bb0

HonahX requested a review from kevinjqliu October 6, 2024 08:28

HonahX mentioned this pull request Oct 6, 2024

Remove initial_change when dealing with table updates #950

Closed

Merge branch 'main' into honahx_remove_initial_changes

50df890

kevinjqliu reviewed Oct 9, 2024

View reviewed changes

kevinjqliu reviewed Oct 29, 2024

View reviewed changes

dev/Dockerfile Outdated Show resolved Hide resolved

HonahX added 6 commits October 29, 2024 20:33

revert lib version updates

20f6a94

remove initial_changes usage in code

37c5ae7

move test to integration

dce2b49

fix typo

f512cb6

update error string

cdeb0e6

Merge branch 'main' into honahx_remove_initial_changes

d3ea218

HonahX changed the title ~~Another way to remove initial_change when CreateTableTransaction apply table updates on an empty metadata~~ Remove initial_change when CreateTableTransaction apply table updates on an empty metadata Oct 30, 2024

HonahX marked this pull request as ready for review October 30, 2024 05:25

kevinjqliu approved these changes Oct 30, 2024

View reviewed changes

kevinjqliu requested review from Fokko and sungwy October 30, 2024 15:40

kevinjqliu added this to the PyIceberg 0.8.0 release milestone Oct 30, 2024

Fokko approved these changes Nov 1, 2024

View reviewed changes

kevinjqliu merged commit 3bdd458 into apache:main Nov 1, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `initial_change` when CreateTableTransaction apply table updates on an empty metadata #1219

Remove `initial_change` when CreateTableTransaction apply table updates on an empty metadata #1219

HonahX commented Oct 6, 2024 •

edited

Loading

kevinjqliu left a comment

kevinjqliu left a comment

kevinjqliu commented Oct 29, 2024

HonahX commented Oct 30, 2024

kevinjqliu left a comment

Fokko left a comment

Fokko Nov 1, 2024

Fokko Nov 1, 2024

kevinjqliu commented Nov 1, 2024

	AddSchemaUpdate(schema_=schema, last_column_id=schema.highest_field_id, initial_change=True),
	SetCurrentSchemaUpdate(schema_id=-1),
	)

	spec: PartitionSpec = table_metadata.spec()
	if spec.is_unpartitioned():
	self._updates += (AddPartitionSpecUpdate(spec=UNPARTITIONED_PARTITION_SPEC, initial_change=True),)
	else:
	self._updates += (AddPartitionSpecUpdate(spec=spec, initial_change=True),)
	self._updates += (SetDefaultSpecUpdate(spec_id=-1),)

	sort_order: Optional[SortOrder] = table_metadata.sort_order_by_id(table_metadata.default_sort_order_id)
	if sort_order is None or sort_order.is_unsorted:
	self._updates += (AddSortOrderUpdate(sort_order=UNSORTED_SORT_ORDER, initial_change=True),)
	else:
	self._updates += (AddSortOrderUpdate(sort_order=sort_order, initial_change=True),)

Remove initial_change when CreateTableTransaction apply table updates on an empty metadata #1219

Remove initial_change when CreateTableTransaction apply table updates on an empty metadata #1219

Conversation

HonahX commented Oct 6, 2024 • edited Loading

kevinjqliu left a comment

Choose a reason for hiding this comment

kevinjqliu left a comment

Choose a reason for hiding this comment

kevinjqliu commented Oct 29, 2024

HonahX commented Oct 30, 2024

kevinjqliu left a comment

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

Fokko Nov 1, 2024

Choose a reason for hiding this comment

Fokko Nov 1, 2024

Choose a reason for hiding this comment

kevinjqliu commented Nov 1, 2024

Remove `initial_change` when CreateTableTransaction apply table updates on an empty metadata #1219

Remove `initial_change` when CreateTableTransaction apply table updates on an empty metadata #1219

HonahX commented Oct 6, 2024 •

edited

Loading