Skip Atempting to Load Key Name - [MOD-9419]#5959
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5959 +/- ##
==========================================
- Coverage 87.09% 87.02% -0.08%
==========================================
Files 211 211
Lines 38595 38616 +21
Branches 1893 1893
==========================================
- Hits 33613 33604 -9
- Misses 4975 5005 +30
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The coverage report is wrong. It ran the test, and it passed. This means the code must be covered |
| if (nkeys == 1 && !strcmp(keys[0]->path, UNDERSCORE_KEY)) { | ||
| // Return a thin RP that doesn't actually loads anything or access to the key space | ||
| // Returning without turning on the `QEXEC_S_HAS_LOAD` flag | ||
| return RPKeyNameLoader_New(keys[0]); |
There was a problem hiding this comment.
How is it reported as not covered?
There was a problem hiding this comment.
I think something got messed up when I rebased the branch from 2.10 to master. The test runs and passes on the coverage test, so we surely use this code
GuyAv46
force-pushed
the
guyav-skip_atempt_to_load_key_name
branch
2 times, most recently
from
adbeed1 to
3f01a3e
Compare
raz-mon
left a comment
raz-mon
left a comment
There was a problem hiding this comment.
Nice optimization and nicely done 🥇
Some more docs would be nice
|
|
||
| /*********************************************************************************/ | ||
|
|
||
| typedef struct { |
There was a problem hiding this comment.
Would be nice to have some docs here, e.g., the purpose of this RP
| ResultProcessor *RPLoader_New(AREQ *r, RLookup *lk, const RLookupKey **keys, size_t nkeys, bool forceLoad) { | ||
| if (RSGlobalConfig.enableUnstableFeatures) { | ||
| if (nkeys == 1 && !strcmp(keys[0]->path, UNDERSCORE_KEY)) { | ||
| // Return a thin RP that doesn't actually loads anything or access to the key space |
There was a problem hiding this comment.
| // Return a thin RP that doesn't actually loads anything or access to the key space | |
| // Return a thin RP that doesn't actually load anything or access to the key space |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.10
git worktree add -d .worktree/backport-5959-to-2.10 origin/2.10
cd .worktree/backport-5959-to-2.10
git switch --create backport-5959-to-2.10
git cherry-pick -x b4ffb1aa01d3e33542f52ecd27cde1529c505cde |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 8.0
git worktree add -d .worktree/backport-5959-to-8.0 origin/8.0
cd .worktree/backport-5959-to-8.0
git switch --create backport-5959-to-8.0
git cherry-pick -x b4ffb1aa01d3e33542f52ecd27cde1529c505cde |
* implement new feature * added a test * reorder implementation * fix test for master * fix spells * improve test * review fixes (cherry picked from commit b4ffb1a)
* implement new feature * added a test * reorder implementation * fix test for master * fix spells * improve test * review fixes (cherry picked from commit b4ffb1a)
* implement new feature * added a test * reorder implementation * fix test for master * fix spells * improve test * review fixes
* implement new feature * added a test * reorder implementation * fix test for master * fix spells * improve test * review fixes
3 participants
Describe the changes in the pull request
Implement an optimization that skips attempting to load the value of
__keywhen requesting to load the key name.The optimization can be enabled by setting
ENABLE_UNSTABLE_FEATURESonThe optimization is applied only if
__keyis the only field the loader was requested to load, or when all the other fields are implicitly loaded (SORTABLE+UNF).Mark if applicable