- Notifications
You must be signed in to change notification settings - Fork875
Bugfix/cor 74 incorrect order arangosearch constrained sort#22188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:devel
Are you sure you want to change the base?
Bugfix/cor 74 incorrect order arangosearch constrained sort#22188
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 28
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit theCursor dashboard to activate Pro and start your 14-day free trial.
| } | ||
| sortBucket.postfix += a.afData.field->at(i).name; | ||
| sortBucket.postfix += a.attr[i].name; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Bug: Incorrect postfix string separator condition causes malformed output
The conditionif (i != a.afData.postfix + 1) for adding the dot separator is incorrect. Sincei starts atfieldSize and iterates up, the check for skipping the dot on the first iteration needs to compare againstfieldSize, nota.afData.postfix + 1. With the current condition, whenfieldSize equals 2 anda.afData.postfix equals 2, the first iteration (i=2) incorrectly adds a leading dot before the first attribute, producing malformed postfix strings like.l3l4 instead ofl3.l4.
| // primarySort: { field: "l1.l2", asc: true } | ||
| // W.r.t to the sort field ("l1.l2"), the query attribute in the sort clause | ||
| // has a postfix of "l3.l4" | ||
| auto splitPostfixAttrs = [](const std::string& input) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure how "hot" his code is but:
theattr temporary andpush_back are not needed.postfixAttrs.emplace_back(part.begin(), part.end()); should do the same with no temp objects.
Also, my experience with the ranges lib is limited but I would assume the you can just pass theinput directly to it, without the extrastring_view.
Uh oh!
There was an error while loading.Please reload this page.
Scope & Purpose
(Please describe the changes in this PR for reviewers, motivation, rationale -mandatory)
Checklist
Related Information
(Please reference tickets / specification / other PRs etc)
Note
Fixes incorrect ordering for constrained ArangoSearch SORT by properly splitting/traversing nested postfix attributes and building correct postfix in optimizer heap sort mapping.
IResearchViewExecutorBase.tpp: UpdategetStoredValueto split dot-separatedpostfixand traverse nested object attributes step-by-step (supportsdoc.a.b...), with minor includes added.OptimizerRulesIResearchView.cpp: Fix construction ofHeapSortElement.postfixwhen attributes are partially covered by primary sort/stored values by iterating fromfieldSizeovera.afData.postfixand usinga.attr[i].nameto compose the remaining path.Written byCursor Bugbot for commit9163f4a. This will update automatically on new commits. Configurehere.