Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: Use HashMap instead of BTreeMap for storing fields by id in StructType #14

Merged
merged 1 commit into from
Jul 30, 2023

Conversation

amogh-jahagirdar
Copy link
Contributor

This change updates the id_lookup table in StructType to be a HashMap implementation instead of BTreeMap. Since there won't be a need to do min/max or any kind of ordering, a BTreeMap shouldn't be necessary, and lookups for a field based on id can just be done in O(1).

cc: @JanKaul @Xuanwo @Fokko

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Collaborator

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

I would also lean towards a HashMap. Probably this is also more memory efficient (fewer pointers than with a BTree).

Don't worry, we're going to use the BTree when we're digging into SequenceNumbers

Copy link
Contributor

@ZENOTME ZENOTME left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@amogh-jahagirdar
Copy link
Contributor Author

Thanks for all the reviews @Fokko @Xuanwo @ZENOTME @liurenjie1024. Since there's a few approvals and it's small change, I'll be merging.

@amogh-jahagirdar amogh-jahagirdar merged commit 831e93c into apache:main Jul 30, 2023
7 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants