Skip to content

Fix deepcopy of primitive types - #857

Merged
Fokko merged 1 commit into
apache:mainfrom
ndrluis:deepcopy-primitive-type
Jun 27, 2024
Merged

Fix deepcopy of primitive types#857
Fokko merged 1 commit into
apache:mainfrom
ndrluis:deepcopy-primitive-type

Conversation

@ndrluis

@ndrluisndrluis commented Jun 25, 2024

Copy link
Copy Markdown
Collaborator

The IcebergRootModel inherits from Pydantic RootModel, which has its own implementation of deepcopy. When deepcopy runs, it calls this __deepcopy__ method and ignores that it's a Singleton. So, my solution was to change the order of inheritance and implement a __deepcopy__ method for singletons that returns itself.

https://github.com/pydantic/pydantic/blob/f024d03b832d1bbcbadf76184ed14d92571a71ca/pydantic/root_model.py#L108-L116

@sungwysungwy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is really cool 🔥 - thank you for putting in this bug fix so quickly!

This is a nit: I think we could add a test to test_metadata.py to deepcopy a TableMetadata object and ensure that the output is equal to the original one with a Schema representation that contains a mix of the issue affected types.

Theoretically this will work, but its never a bad thing to add an explicit test to demonstrate that the issue is resolved. But other than that, this looks great! Such a speedy resolution of a rather opaque issue 🚀

@ndrluis

Copy link
Copy Markdown
CollaboratorAuthor

Thank you, @syun64. I have added the test and used an existing metadata definition to avoid duplication.

Regarding the solution, to be honest, it was not quick. It took me several weeks of learning and then stepping away from the problem for a few weeks to return with fresh ideas on how to solve it.

@sungwy

Copy link
Copy Markdown
Collaborator

Hi @HonahX@Fokko could we ask for your help in triggering this workflow?

@FokkoFokko added this to the PyIceberg 0.7.0 release milestone Jun 25, 2024
@ndrluis
ndrluisforce-pushed the deepcopy-primitive-type branch from 5bad46b to db8b151CompareJune 26, 2024 00:10

@kevinjqliukevinjqliu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing this 🐛!

Comment threadpyiceberg/utils/singleton.py
Comment threadtests/table/test_metadata.py Outdated
@ndrluis
ndrluisforce-pushed the deepcopy-primitive-type branch from db8b151 to 4fb7090CompareJune 26, 2024 16:51
@ndrluis
ndrluisforce-pushed the deepcopy-primitive-type branch from 4fb7090 to 76127a5CompareJune 26, 2024 16:52
@ndrluis
ndrluis requested a review from kevinjqliuJune 26, 2024 16:52

@kevinjqliukevinjqliu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@syun64 wdyt

@sungwysungwy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yep! Fantastic fix 💯

@ndrluis

Copy link
Copy Markdown
CollaboratorAuthor

@HonahX@Fokko, can you please trigger the workflow? I have already run the tests in my fork and everything is green.
image

@Fokko

Copy link
Copy Markdown
Contributor

@ndrluis Thanks for pinging me here. This approval should be gone once the first PR has been merged. Which should be soon 🚀

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the great fix!

This approval should be gone once the first PR has been merged.

@Fokko I might have lost track. Which PR are you referring to as the first one?

@sungwysungwy mentioned this pull request Jun 27, 2024
@Fokko

Fokko commented Jun 27, 2024

Copy link
Copy Markdown
Contributor

@HonahX Sorry for the ambigous comment there. I was referring to approving the CI to run, which is very annoying. Now @ndrluis has his first PR in, this approval should be gone and the CI should be kicked off right away.

@Fokko
Fokko merged commit c3c3468 into apache:mainJun 27, 2024
@Fokko

Copy link
Copy Markdown
Contributor

Thanks @ndrluis for working on this, and @syun64, @kevinjqliu and @HonahX for the quick review 🚀

@ndrluis
ndrluis deleted the deepcopy-primitive-type branch July 1, 2024 23:13
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ndrluis@sungwy@Fokko@kevinjqliu@HonahX