Skip to content

Fix deepcopy of primitive types#857

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

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

Conversation

@ndrluis
Copy link
Collaborator

@ndrluis ndrluis commented Jun 25, 2024 •
edited
Loading

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

@ndrluis ndrluis force-pushed the deepcopy-primitive-type branch from 0aba3ac to dc42e88 Compare June 25, 2024 18:19
@ndrluis ndrluis mentioned this pull request Jun 25, 2024
Closed
@ndrluis ndrluis force-pushed the deepcopy-primitive-type branch from dc42e88 to a68add0 Compare June 25, 2024 19:38
sungwy
sungwy approved these changes Jun 25, 2024
View reviewed changes
Copy link
Collaborator

@sungwy sungwy left a comment

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
Collaborator Author

ndrluis commented Jun 25, 2024

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
Collaborator

sungwy commented Jun 25, 2024

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

@Fokko Fokko added this to the PyIceberg 0.7.0 release milestone Jun 25, 2024
@ndrluis ndrluis force-pushed the deepcopy-primitive-type branch from 5bad46b to db8b151 Compare June 26, 2024 00:10
kevinjqliu
kevinjqliu reviewed Jun 26, 2024
View reviewed changes
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this 🐛!

@ndrluis ndrluis force-pushed the deepcopy-primitive-type branch from db8b151 to 4fb7090 Compare June 26, 2024 16:51
@ndrluis ndrluis force-pushed the deepcopy-primitive-type branch from 4fb7090 to 76127a5 Compare June 26, 2024 16:52
@ndrluis ndrluis requested a review from kevinjqliu June 26, 2024 16:52
kevinjqliu
kevinjqliu approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM!

@syun64 wdyt

sungwy
sungwy approved these changes Jun 26, 2024
View reviewed changes
Copy link
Collaborator

@sungwy sungwy left a comment

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
Collaborator Author

ndrluis commented Jun 26, 2024

@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
Contributor

Fokko commented Jun 26, 2024

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

HonahX
HonahX approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@HonahX HonahX left a comment

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?

@sungwy sungwy mentioned this pull request Jun 27, 2024
Closed
@Fokko
Copy link
Contributor

Fokko commented Jun 27, 2024 •
edited
Loading

@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:main Jun 27, 2024
@Fokko
Copy link
Contributor

Fokko commented Jun 27, 2024

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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevinjqliu kevinjqliu kevinjqliu approved these changes

@sungwy sungwy sungwy approved these changes

@HonahX HonahX HonahX approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

PyIceberg 0.7.0 release

Development

Successfully merging this pull request may close these issues.

5 participants

@ndrluis @sungwy @Fokko @kevinjqliu @HonahX