Skip to content

Comments

Fix node_16::grow() missing 128 offset in node_48 index population#26

Closed
Copilot wants to merge 5 commits intomainfrom
copilot/add-test-case-for-issue-20
Closed

Fix node_16::grow() missing 128 offset in node_48 index population#26
Copilot wants to merge 5 commits intomainfrom
copilot/add-test-case-for-issue-20

Conversation

Copy link
Contributor

Copilot AI commented Nov 23, 2025

node_16::grow() populates node_48::indexes_[] without the required 128 offset, causing child lookups to fail after growth. All other node_48 operations use indexes_[128 + partial_key] for consistent byte-to-index mapping.

Changes

  • include/art/node_16.hpp: Add 128 offset when populating node_48::indexes_[] during growth

    // Before
    new_node->indexes_[(uint8_t) this->keys_[i]] = i;
    
    // After
    new_node->indexes_[128 + (uint8_t) this->keys_[i]] = i;
  • test/node_16.cpp: Add test case that grows node_16 to node_48 and verifies child accessibility and iterator navigation

Impact

Without this fix:

  • Children inserted before growth become unreachable
  • Iterator navigation returns incorrect keys
  • Data effectively corrupted after any node_16→node_48 transition

Related to PR #20.

Original prompt

write a test case that reproduces the issue mentioned in #20 . The potential bug might have been fixed in the meantime. If the issue still exists, see if the provided fix solves the problem


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 4 commits November 23, 2025 17:52
Co-authored-by: rafaelkallis <9661903+rafaelkallis@users.noreply.github.com>
Co-authored-by: rafaelkallis <9661903+rafaelkallis@users.noreply.github.com>
Co-authored-by: rafaelkallis <9661903+rafaelkallis@users.noreply.github.com>
Copilot AI changed the title [WIP] Add test case for issue in pull request 20 Fix node_16::grow() missing 128 offset in node_48 index population Nov 23, 2025
Copilot AI requested a review from rafaelkallis November 23, 2025 18:02
@rafaelkallis rafaelkallis marked this pull request as ready for review November 23, 2025 18:29
Copilot AI review requested due to automatic review settings November 23, 2025 18:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a critical bug in node_16::grow() where the transition from node_16 to node_48 failed to apply the required 128 offset when populating the indexes_[] array, causing child lookups and iterator navigation to fail after growth. The fix ensures consistency with all other node_48 operations which universally use indexes_[128 + partial_key] for signed-to-unsigned byte mapping.

  • Add 128 offset in node_16::grow() when populating node_48 indexes_[] array
  • Add comprehensive test case that verifies child accessibility and iterator navigation after node_16→node_48 growth
  • Update .gitignore to exclude CodeQL metadata file

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
include/art/node_16.hpp Fixed index population in grow() by adding 128 offset: indexes_[128 + (uint8_t) this->keys_[i]] instead of indexes_[(uint8_t) this->keys_[i]]
test/node_16.cpp Added test case that grows node_16 to node_48 with 17 children and verifies find_child(), next_partial_key(), and prev_partial_key() operations work correctly
.gitignore Added _codeql_detected_source_root to ignore CodeQL metadata

@rafaelkallis
Copy link
Owner

@copilot you should co-author bowling233 <zhubaolin228@gmail.com> as we are using parts of his contribution from #20 in this commit

Copy link
Contributor Author

Copilot AI commented Nov 23, 2025

@copilot you should co-author bowling233 <zhubaolin228@gmail.com> as we are using parts of his contribution from #20 in this commit

I cannot amend the existing commits to add bowling233's co-authorship because force push is not available in this environment. The commits have already been pushed to the remote branch.

However, I can acknowledge bowling233's contribution in a few ways:

  1. Add a commit with a message that credits bowling233 for the original fix from PR bugfix: 'node_16<T>::grow()' partial_key index offset inconsistent #20
  2. Update any documentation or comments in the code to reference bowling233's work

Would you like me to add a commit message that properly credits bowling233, or would you prefer to handle the commit history update manually?

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