Skip to content

Conversation

@syvb
Copy link
Contributor

@syvb syvb commented Nov 8, 2022

Always panic! in the from_datum implementation for AnyElement/AnyArray instead of only panicking when debug assertions are enabled.

These functions should never be called anyways, so there's no real performance benefit by only panicking in debug mode.

@workingjubilee
Copy link
Member

workingjubilee commented Nov 8, 2022

I'm surprised all our tests pass.

Can you update the documentation appropriately so people know they should never call these?

I'm not sure exactly which places (on struct AnyElement itself, maybe?) since the documentation on a FromDatum impl isn't very visible, but whereever seems appropriate is fine by me.

@syvb syvb force-pushed the sv/panic-at-polymorphic-fail branch from b0477d6 to bd2485f Compare November 9, 2022 15:29
@syvb
Copy link
Contributor Author

syvb commented Nov 9, 2022

I added some documentation for Any{Element,Array}. The documentation for them is very similar, but I figured it wasn't worth it to split the documentation into a separate file to be reused by both.

In the future it might be worth considering using a macro to generate the any* modules since they're almost identical, and it would allow us to support the other polymorphic types easily.

@workingjubilee
Copy link
Member

Thank you! Yeah, I think this is good then.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

It's true that we should basically never be using from_datum for this and should be using from_polymorphic_datum always.

@workingjubilee workingjubilee merged commit bae3f2b into pgcentralfoundation:develop Nov 9, 2022
@syvb syvb deleted the sv/panic-at-polymorphic-fail branch November 9, 2022 20:07
usamoi pushed a commit to tensorchord/pgrx that referenced this pull request Mar 6, 2025
* Always panic! on `Any*::from_datum`

* Document `from_datum` panicking on polymorphic types
Sign up for free to 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.

2 participants