-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Defer copying of non-key columns at TopN::addInput(). #7172
Conversation
✅ Deploy Preview for meta-velox canceled.
|
The failed CI is unrelated: #7154 |
@usurai Do you have performance numbers to share? |
Converting to draft due to #7298 |
…Input. At first, only decodes the key columns. While processing the input, only copies the key columns for the row that's added to priority queue to RowContainer(to be used for later comparison). After the whole batch of input is all compared, decodes and copies the passed input rows to RowContainer.
@xumingming No concrete number at my hand since there is no benchmark for this operator. I guess the diff would mostly depend on the workload, e.g. if non-key columns are complex type, this change will make some difference. |
@mbasmanova Any chance you could help to review this? Thanks! |
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.
Some comments.
velox/exec/TopN.cpp
Outdated
decodedVectors_[col].decode(*input->childAt(col)); | ||
} | ||
|
||
// Maps passed rows of 'data_' to the corresponding input row number. These | ||
// input rows of non-key columns are later decoded and stored into data_. | ||
std::unordered_map<char*, size_t> passedRows; |
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.
use F14FastMap
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.
F14FastMap with char*
as key type caused some issue so I use void*
instead.
- Skip at some path if no non-key columns and decode all the rows. - Use F14FastMap. - Enforce non-duplicated sorting key at TopNNode.
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.
Looks good % a few minor comments.
- Update TopN ctor: move to cpp and check the result of insert. - Add sanity check. - Update the documentation.
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.
Looks good % some nits.
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.
Thanks.
Move #include to PlanNode.cpp and fix naming issues.
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@mbasmanova merged this pull request in 26e4c06. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Current implementation copies all the columns for the row that's added to the
priority queue to RowContainer. If a row is added to priority queue and then is
replaced by another row from the same batch of input, copying non-key columns
to RowContainer is wasteful.
This commit adds the following optimization. First, loop over the input rows,
identify rows that need to be added to the priority queue, copy key columns for
these to RowContainer and add these to the priority queue. For the input rows
that remain in the priority queue, copy non-key columns to the RowContainer.
This logic avoids copying non-key columns for rows that do not stay in the
priority queue.
Also, add a check for duplicate sorting keys to TopNNode's constructor.