refactor(TopNRowNumber): Abstract computeNextRankInMemory(InSpill) functions in getOutput() logic by aditi-pandit · Pull Request #13860 · facebookincubator/velox

aditi-pandit · 2025-06-24T05:38:40Z

Last refactoring towards #11554

netlify · 2025-06-24T05:38:45Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`cb72f94`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/68ced6267eb178000850716f

JkSelf

LGTM. Except one small nit. Thanks.

velox/exec/TopNRowNumber.h

velox/exec/TopNRowNumber.cpp

jinchengchenghh · 2025-06-26T07:46:17Z

velox/exec/TopNRowNumber.cpp

+}
+
+void TopNRowNumber::setupNextOutput(const RowVectorPtr& output) {
+  auto resetNextRank = [this]() { nextRank_ = 1; };


Why you reset the nextRank_ = 1 and use <= to replace < before?

@jinchengchenghh I will be updating this logic for both rank and dense_rank computation in the next PR.

In rank and dense_rank the result value changes only if order by keys are different between current row and next. By setting starting rank as 0 and then incrementing in place if different from adjacent row is complicated logic...Its simpler to set current rank as 1 and increment it when advancing to the next row. So I changed how the result value is computed.

I'm not too familiar with this operator.
We can use 1 because we have at least one row result (if it doesn't match any other row) that has rank/row_number 1.

@czentgr : Yes.

jinchengchenghh · 2025-07-17T01:39:25Z

velox/exec/TopNRowNumber.h


-  // Row number for the first row in the next output batch from the spiller.
-  int32_t nextRowNumber_{0};
+  // Row number ( or rank or dense_rank in the future) for the next row being


( or -> (or

jinchengchenghh

Approve for the code, I'm not familiar with TopN

aditi-pandit · 2025-08-14T06:11:23Z

@xiaoxmeng : Please can you help with the review. Have bunch of approvals from maintainers already.

xiaoxmeng

@aditi-pandit LGTM % minors. Thanks!

xiaoxmeng · 2025-08-28T06:22:48Z

velox/exec/TopNRowNumber.cpp

    vector_size_t index,
-    SpillMergeStream* next) {
+    const SpillMergeStream* next,
+    vector_size_t startColumn,


Why we pass startColumn and endColumn but not 0, numPartitionKeys_? And why call this as compareSpillRowColumns?

@xiaoxmeng : This code will be enhanced to do the topn optimization for rank functions as well... and those require to compare the rows on the order_by columns. So I abstracted the function for compareSpillRowColumns which will be reused to compare the order by columns as well.

xiaoxmeng · 2025-08-28T06:23:23Z

velox/exec/TopNRowNumber.cpp

-    if (index > 0 && isNewPartition(output, index, next)) {
-      rowNumber = 0;
+    if (index > 0) {
+      computeNextRankInSpill(output, index, next);


s/computeNextRankInSpill/computeNextRankFromSpill/

xiaoxmeng · 2025-08-28T06:26:14Z

velox/exec/TopNRowNumber.cpp

+void TopNRowNumber::setupNextOutput(const RowVectorPtr& output) {
+  auto resetNextRank = [this]() { nextRank_ = 1; };
+
+  auto* lookAhead = merge_->next();


Let's just call this next?

The loop below uses the next variable naming at line 595, so I stuck with the original lookAhead name here.

I mean let's just use next why have two variables: next and lookAhead?

velox/exec/TopNRowNumber.cpp

aditi-pandit · 2025-09-10T05:03:36Z

@xiaoxmeng : Have addressed your review comments. PTAL.

… for TopNRowNumber output loop

facebook-github-bot · 2025-09-23T04:30:00Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this in D83028157.

facebook-github-bot · 2025-09-23T11:09:48Z

@xiaoxmeng merged this pull request in 9578779.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 24, 2025

aditi-pandit changed the title ~~refactor(TopNRowNumber): Abstract computeRankInMemory and computeRankInSpill functions in getOutput() logic~~ refactor(TopNRowNumber): Abstract computeNextRankInMemory and computeNextRankInSpill functions in getOutput() logic Jun 24, 2025

aditi-pandit changed the title ~~refactor(TopNRowNumber): Abstract computeNextRankInMemory and computeNextRankInSpill functions in getOutput() logic~~ refactor(TopNRowNumber): Abstract computeNextRankInMemory(InSpill) functions in getOutput() logic Jun 24, 2025

aditi-pandit requested review from JkSelf, czentgr, jinchengchenghh, majetideepak, rui-mo and xiaoxmeng June 24, 2025 05:48

aditi-pandit marked this pull request as draft June 24, 2025 06:37

aditi-pandit changed the title ~~refactor(TopNRowNumber): Abstract computeNextRankInMemory(InSpill) functions in getOutput() logic~~ [Do not review] refactor(TopNRowNumber): Abstract computeNextRankInMemory(InSpill) functions in getOutput() logic Jun 24, 2025

aditi-pandit removed request for JkSelf, czentgr, jinchengchenghh, majetideepak, rui-mo and xiaoxmeng June 24, 2025 06:38

aditi-pandit force-pushed the topn_get_output branch from 40e7bc7 to 1f9be71 Compare June 24, 2025 15:25

aditi-pandit changed the title ~~[Do not review] refactor(TopNRowNumber): Abstract computeNextRankInMemory(InSpill) functions in getOutput() logic~~ refactor(TopNRowNumber): Abstract computeNextRankInMemory(InSpill) functions in getOutput() logic Jun 24, 2025

aditi-pandit requested review from JkSelf, czentgr, jinchengchenghh, majetideepak, rui-mo and xiaoxmeng June 24, 2025 18:10

aditi-pandit marked this pull request as ready for review June 24, 2025 18:10

JkSelf reviewed Jun 26, 2025

View reviewed changes

velox/exec/TopNRowNumber.h Outdated Show resolved Hide resolved

jinchengchenghh reviewed Jun 26, 2025

View reviewed changes

aditi-pandit force-pushed the topn_get_output branch from 1f9be71 to 4ae0ede Compare June 26, 2025 17:48

jinchengchenghh reviewed Jul 17, 2025

View reviewed changes

jinchengchenghh approved these changes Jul 17, 2025

View reviewed changes

aditi-pandit force-pushed the topn_get_output branch from 4ae0ede to f204bc7 Compare July 17, 2025 21:08

aditi-pandit force-pushed the topn_get_output branch from f204bc7 to 7de74c0 Compare August 14, 2025 06:07

xiaoxmeng reviewed Aug 28, 2025

View reviewed changes

aditi-pandit force-pushed the topn_get_output branch 2 times, most recently from 83db96b to 9baa46f Compare September 10, 2025 05:02

refactor: Abstract computeRankInMemory and computeRankInSpill methods…

cb72f94

… for TopNRowNumber output loop

aditi-pandit force-pushed the topn_get_output branch from 9baa46f to cb72f94 Compare September 20, 2025 16:28

xiaoxmeng approved these changes Sep 23, 2025

View reviewed changes

facebook-github-bot closed this in 9578779 Sep 23, 2025

facebook-github-bot added the Merged label Sep 23, 2025

Conversation

aditi-pandit commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

JkSelf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinchengchenghh left a comment

Choose a reason for hiding this comment

Uh oh!

aditi-pandit commented Aug 14, 2025

Uh oh!

xiaoxmeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditi-pandit commented Sep 10, 2025

Uh oh!

facebook-github-bot commented Sep 23, 2025

Uh oh!

facebook-github-bot commented Sep 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aditi-pandit commented Jun 24, 2025 •

edited

Loading

netlify bot commented Jun 24, 2025 •

edited

Loading

aditi-pandit Jun 26, 2025 •

edited

Loading