Fix iproto index name to ID mapping#10902
Merged
sergepetrenko merged 1 commit intotarantool:masterfrom Dec 24, 2024
Merged
Conversation
64492da to
ac51970
Compare
drewdzzz
requested changes
Dec 6, 2024
Contributor
drewdzzz
left a comment
There was a problem hiding this comment.
Thanks for the patch! See my comments below.
Regarding commit message:
Header:
- Do not use capital letter after the category name (
Correct->correct) - Let's replace word "correct" with "fix" - "correct" is often used as an adjective and "fix" is always a verb.
Body:
- Please, do not use multiple spaces between sentences.
- I would rephrase it in such way: explain the problem (that Tarantool resolved name to dense id but Tarantool requests actually expect id observed by user -
index->def->iid) and after that something like "The commit fixed this problem". What's about the current message - it's still not clear why you replaceddense_idwithindex->def->iid. - "fail due to incorrect index ID resolution when fetch_schema was false" - did you check it? Does it really works with
fetch_schema = true?
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
92520c0 to
2eee83a
Compare
drewdzzz
requested changes
Dec 9, 2024
Contributor
drewdzzz
left a comment
There was a problem hiding this comment.
Thanks for the fixes!
I have no objections regarding the fix, commit message and changelog. Let's just slightly improve the test.
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
2eee83a to
6859c1f
Compare
31a9d1f to
ac51730
Compare
drewdzzz
requested changes
Dec 9, 2024
Contributor
drewdzzz
left a comment
There was a problem hiding this comment.
Thanks for the fixes! Looks good, just a few nitpicks.
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
ac51730 to
a40756f
Compare
drewdzzz
approved these changes
Dec 13, 2024
Contributor
drewdzzz
left a comment
There was a problem hiding this comment.
Thanks for the patch, LGTM!
CuriousGeorgiy
approved these changes
Dec 13, 2024
Member
CuriousGeorgiy
left a comment
There was a problem hiding this comment.
Hey Maria, nice to meet you! Great first patch to start your Tarantool journey!
I have left some comments below regarding polishing of the test. These comments are mostly nitpicks, lifehacks, tricks and conventional Tarantool wisdom.
Please expect a lot of them in the beginning of your journey — they do not in any way diminish your hard skills, their goal is rather to calibrate you to our development traditions and expectations.
changelogs/unreleased/gh-9923-fix-invalid-iproto-name-mapping-on-the-server-side.md
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
33eda19 to
15d5e2d
Compare
xuniq
approved these changes
Dec 18, 2024
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_9923_invalid_iproto_index_name_mapping_on_the_server_side_test.lua
Outdated
Show resolved
Hide resolved
15d5e2d to
770973b
Compare
Previously, index names were resolved to dense IDs, which caused failures when accessing indexes by name. This happened because the index_map lookup expected user-visible IDs (index->def->iid), not dense IDs. The fix replaces dense_id with index->def->iid to ensure correct mapping of index names to their numeric IDs. Closes tarantool#9923 NO_DOC=bug fix
770973b to
524976b
Compare
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
iproto: fix IPROTO index name to ID mapping
Previously, index names were resolved to dense IDs, which caused
failures when accessing indexes by name. This happened because the
index_map lookup expected user-visible IDs (index->def->iid), not dense IDs.
The fix replaces dense_id with index->def->iid to ensure correct mapping
of index names to their numeric IDs, aligning with the expected behavior
when accessing indexes by name.
Closes #9923