Skip to content

Conversation

@XanthosXanthopoulos
Copy link
Contributor

@XanthosXanthopoulos XanthosXanthopoulos commented Mar 19, 2025

Add Enumeration::index_of API which returns an std::optional<uint64_t> containing the internal index of a value in an Enumeration if it exists.

User code who needs to extend an enumeration given a list of values need to recreate the internal hash map structure to efficiently find which values are missing from the enumeration. Also remapping the indexes from a subset of values to the values the enumeration holds also requires the creation of the values hash map. Providing access to the internal hash map of the enumeration eliminates the need to recreate the map which might me an expensive step.


TYPE: FEATURE
DESC: Add Enumeration::index_of API

@rroelke
Copy link
Member

rroelke commented Mar 19, 2025

I'll also add that for tables, we (currently?) treat attributes with enumerations as columns of the enumeration data type, and then we have to map inserted values to the keys. Similarly we build our own hash map of value to key in order to do this, this API will make this process more efficient as well.

Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

That... is significantly simpler than I was expecting. Nice work!

Though we should add tests of the C++ API as well. They look right to me, but we should also have tests to make sure they continue to work.

Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

+1

@XanthosXanthopoulos XanthosXanthopoulos force-pushed the xan/sc-64451/expose-enumeration-index-of branch from 11ba58c to 4b4eef4 Compare March 20, 2025 16:26
@ypatia ypatia merged commit b171d12 into main Mar 26, 2025
59 checks passed
@ypatia ypatia deleted the xan/sc-64451/expose-enumeration-index-of branch March 26, 2025 12:01
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.

5 participants