MOD-11190: Add IndexSpec RDB Comparison Tests - Fix Index RDB field initialization#6817
MOD-11190: Add IndexSpec RDB Comparison Tests - Fix Index RDB field initialization#6817
Conversation
kei-nan
force-pushed
the
master_jk_add_rdb_index_spec_tests
branch
from
b3b6849 to
8b84c58
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6817 +/- ##
==========================================
- Coverage 86.76% 86.72% -0.04%
==========================================
Files 288 288
Lines 46302 46309 +7
Branches 9195 9195
==========================================
- Hits 40173 40162 -11
- Misses 5978 5996 +18
Partials 151 151
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
src/spec.c
Outdated
| int16_t numFields) { | ||
| sp->flags = flags; | ||
| sp->numFields = numFields; | ||
| sp->fields = rm_calloc(sizeof(FieldSpec), numFields); |
There was a problem hiding this comment.
Can we also add the index error initialization loop here?
There was a problem hiding this comment.
When creating an index it would mean doing an init 1024 times.
Seems a bit excessive, no?
There was a problem hiding this comment.
Not if we attempt to free all of the 1024 later... how are we avoiding crashes there at the moment? We change the sp->numFields later on to the actual number?
There was a problem hiding this comment.
Allocating 1024 fields is excessive on its own; maybe we can avoid that completely?
There was a problem hiding this comment.
I'll try passing 0 and see if the unit tests pass , don't want to cause an issue elsewhere in the code but I guess it would be better behaviour.
I think we do a realloc when creating the first field anyhow - so the duration in the code we would be without a field is small.
Wondering if a schema with zero fields is even possible.
kei-nan
force-pushed
the
master_jk_add_rdb_index_spec_tests
branch
from
e28ff31 to
0ce401c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.8
git worktree add -d .worktree/backport-6817-to-2.8 origin/2.8
cd .worktree/backport-6817-to-2.8
git switch --create backport-6817-to-2.8
git cherry-pick -x a73f3f7c57c49c3d24ae86d6ed9b4fafd32ae419 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.6
git worktree add -d .worktree/backport-6817-to-2.6 origin/2.6
cd .worktree/backport-6817-to-2.6
git switch --create backport-6817-to-2.6
git cherry-pick -x a73f3f7c57c49c3d24ae86d6ed9b4fafd32ae419 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.10
git worktree add -d .worktree/backport-6817-to-2.10 origin/2.10
cd .worktree/backport-6817-to-2.10
git switch --create backport-6817-to-2.10
git cherry-pick -x a73f3f7c57c49c3d24ae86d6ed9b4fafd32ae419 |
…nitialization (#6817) * initial commit * prepare code for CI run * move terms and fieldIdToIndex initialization to initializeIndexSpec * cleanup test a bit * remove extra RWlock init (#6823) * fix code review comments * remove print from mock * remove print from mock functions * code review comment * remove log remainder * switch rm_calloc arg order * add initializeFieldSpec function --------- Co-authored-by: Joan Fontanals <[email protected]> (cherry picked from commit a73f3f7)
|
Successfully created backport PR for |
…ield initialization (#6866) MOD-11190: Add IndexSpec RDB Comparison Tests - Fix Index RDB field initialization (#6817) * initial commit * prepare code for CI run * move terms and fieldIdToIndex initialization to initializeIndexSpec * cleanup test a bit * remove extra RWlock init (#6823) * fix code review comments * remove print from mock * remove print from mock functions * code review comment * remove log remainder * switch rm_calloc arg order * add initializeFieldSpec function --------- (cherry picked from commit a73f3f7) Co-authored-by: kei-nan <[email protected]> Co-authored-by: Joan Fontanals <[email protected]>
3 participants
We noticed some fields do not get initialized correctly when the index spec is initialized from an RDB.
Added mock rdb support for tests and added tests to compare an index to itself after it stored to an RDB and loaded again.
A clear and concise description of what the PR is solving, including:
Main objects this PR modified
Mark if applicable