You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "SubhamSinghal (via GitHub)" <gi...@apache.org> on 2026/10/08 06:18:38 UTC
[PR] perf: Partitioned topk emit row number [datafusion]
SubhamSinghal opened a new pull request, #26133:
URL: https://github.com/apache/datafusion/pull/26133
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#issuecomment-6063482292
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26133?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `87.88927%` with `70 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.75%. Comparing base ([`1c43063`](https://app.codecov.io/gh/apache/datafusion/commit/1c430632276cc0e0de2bf5b1a766da8cec84cba1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`318d470`](https://app.codecov.io/gh/apache/datafusion/commit/318d470a79aac5cc75ddbbd0acd11b4e747e945d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 14 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26133?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-plan/src/topk/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/26133?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Ftopk%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy90b3BrL21vZC5ycw==) | 89.84% | [6 Missing and 33 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26133?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...fusion/physical-plan/src/sorts/partitioned\_topk.rs](https://app.codecov.io/gh/apache/datafusion/pull/26133?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fsorts%2Fpartitioned_topk.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9zb3J0cy9wYXJ0aXRpb25lZF90b3BrLnJz) | 83.97% | [9 Missing and 20 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26133?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-optimizer/src/window\_topn.rs](https://app.codecov.io/gh/apache/datafusion/pull/26133?src=pr&el=tree&filepath=datafusion%2Fphysical-optimizer%2Fsrc%2Fwindow_topn.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1vcHRpbWl6ZXIvc3JjL3dpbmRvd190b3BuLnJz) | 83.33% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26133?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26133 +/- ##
==========================================
+ Coverage 82.73% 82.75% +0.02%
==========================================
Files 1147 1147
Lines 449389 450243 +854
Branches 449389 450243 +854
==========================================
+ Hits 371788 372594 +806
+ Misses 54943 54932 -11
- Partials 22658 22717 +59
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26133?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#discussion_r4247890221
##########
datafusion/physical-plan/src/sorts/partitioned_topk.rs:
##########
@@ -245,18 +261,40 @@ impl PartitionedTopKExec {
fetch: usize,
fn_kind: WindowFnKind,
) -> Result<Self> {
- let cache = Self::compute_properties(&input, expr.clone())?;
+ let cache =
+ Self::compute_properties(&input, expr.clone(), partition_prefix_len, None)?;
Ok(Self {
input,
expr,
partition_prefix_len,
fetch,
fn_kind,
+ ranking_field: None,
metrics_set: ExecutionPlanMetricsSet::new(),
cache: Arc::new(cache),
})
}
+ /// Append `field` to the output, holding each retained row's value for
+ /// [`Self::fn_kind`]'s ranking function, so the caller can drop the
+ /// `BoundedWindowAggExec` that would otherwise compute it.
+ ///
+ /// The output schema widens by that one column, so the plan properties
+ /// are recomputed under it.
+ pub fn with_ranking_field(self, field: FieldRef) -> Result<Self> {
Review Comment:
The PR description still says `try_new` takes a new `ranking_field` parameter and introduces a breaking API change, but its signature is unchanged. Could you update the description to reflect the new `with_ranking_field` builder and `ranking_field()` accessor, and replace the open question about function-name matching with the implemented built-in equality check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#issuecomment-6189434909
🚀
@SubhamSinghal
Thank you for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "SubhamSinghal (via GitHub)" <gi...@apache.org>.
SubhamSinghal commented on PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#issuecomment-6074441919
@kosiew @jayzhan-synnada @kumarUjjawal Can you help in reviewing this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#discussion_r4247890221
##########
datafusion/physical-plan/src/sorts/partitioned_topk.rs:
##########
@@ -245,18 +261,40 @@ impl PartitionedTopKExec {
fetch: usize,
fn_kind: WindowFnKind,
) -> Result<Self> {
- let cache = Self::compute_properties(&input, expr.clone())?;
+ let cache =
+ Self::compute_properties(&input, expr.clone(), partition_prefix_len, None)?;
Ok(Self {
input,
expr,
partition_prefix_len,
fetch,
fn_kind,
+ ranking_field: None,
metrics_set: ExecutionPlanMetricsSet::new(),
cache: Arc::new(cache),
})
}
+ /// Append `field` to the output, holding each retained row's value for
+ /// [`Self::fn_kind`]'s ranking function, so the caller can drop the
+ /// `BoundedWindowAggExec` that would otherwise compute it.
+ ///
+ /// The output schema widens by that one column, so the plan properties
+ /// are recomputed under it.
+ pub fn with_ranking_field(self, field: FieldRef) -> Result<Self> {
Review Comment:
<img width="804" height="170" alt="image" src="https://github.com/user-attachments/assets/a1c677ea-9291-4ae8-be46-0fe12ef8a006" />
The PR description still says `try_new` takes a new parameter and introduces a breaking API change, but its signature is unchanged. Could you update the description to reflect the new `with_ranking_field` builder and `ranking_field()` accessor, and replace the open question about function-name matching with the implemented built-in equality check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "SubhamSinghal (via GitHub)" <gi...@apache.org>.
SubhamSinghal commented on code in PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#discussion_r4255238246
##########
datafusion/physical-plan/src/sorts/partitioned_topk.rs:
##########
@@ -245,18 +261,40 @@ impl PartitionedTopKExec {
fetch: usize,
fn_kind: WindowFnKind,
) -> Result<Self> {
- let cache = Self::compute_properties(&input, expr.clone())?;
+ let cache =
+ Self::compute_properties(&input, expr.clone(), partition_prefix_len, None)?;
Ok(Self {
input,
expr,
partition_prefix_len,
fetch,
fn_kind,
+ ranking_field: None,
metrics_set: ExecutionPlanMetricsSet::new(),
cache: Arc::new(cache),
})
}
+ /// Append `field` to the output, holding each retained row's value for
+ /// [`Self::fn_kind`]'s ranking function, so the caller can drop the
+ /// `BoundedWindowAggExec` that would otherwise compute it.
+ ///
+ /// The output schema widens by that one column, so the plan properties
+ /// are recomputed under it.
+ pub fn with_ranking_field(self, field: FieldRef) -> Result<Self> {
Review Comment:
updated PR description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #26133:
URL: https://github.com/apache/datafusion/pull/26133
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#discussion_r4241349618
##########
datafusion/physical-optimizer/src/window_topn.rs:
##########
@@ -233,19 +246,47 @@ impl WindowTopN {
return None;
}
- let partitioned_topk = PartitionedTopKExec::try_new(
+ // The operator can emit the ranking column itself, which makes the
+ // `BoundedWindowAggExec` above it redundant — see step 7. All three
+ // ranking functions are derivable from the retained set as it is
+ // emitted (`PartitionedTopKExec` documents how per policy), so the only
+ // condition is that there is exactly one window expression: the
+ // operator appends a single column, and it has to reproduce the whole
+ // of the window node's output schema. With two or more the window node
+ // stays and computes them as before.
+ let ranking_field = if window_exprs.len() == 1 {
Review Comment:
This removes the window evaluator based only on the function name, but a custom UDWF named `row_number`, `rank`, or `dense_rank` can have different behavior. I reproduced a custom `row_number` returning constant 7 that produces `[1, 2]` instead of `[7, 7]` with WindowTopN enabled, even when no rows are pruned. Please check the actual `WindowUDF` identity in `supported_window_fn`, using equality with the built-in functions rather than checking the `Rank` implementation type, which is shared by several functions. Please also add regression tests for same-name custom functions returning `UInt64` and another type, and ideally a custom sibling expression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "SubhamSinghal (via GitHub)" <gi...@apache.org>.
SubhamSinghal commented on code in PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#discussion_r4241702363
##########
datafusion/physical-optimizer/src/window_topn.rs:
##########
@@ -233,19 +246,47 @@ impl WindowTopN {
return None;
}
- let partitioned_topk = PartitionedTopKExec::try_new(
+ // The operator can emit the ranking column itself, which makes the
+ // `BoundedWindowAggExec` above it redundant — see step 7. All three
+ // ranking functions are derivable from the retained set as it is
+ // emitted (`PartitionedTopKExec` documents how per policy), so the only
+ // condition is that there is exactly one window expression: the
+ // operator appends a single column, and it has to reproduce the whole
+ // of the window node's output schema. With two or more the window node
+ // stays and computes them as before.
+ let ranking_field = if window_exprs.len() == 1 {
Review Comment:
Addressed in [a3f5e79](https://github.com/apache/datafusion/pull/26133/commits/a3f5e793d707dd60fa1f80d4c6ca5cf7c728951d)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#issuecomment-6108395687
[benchmark run](https://github.com/apache/datafusion/pull/26204#issuecomment-6108312482)
```
group HEAD bench-26133
----- ---- -----------
h2o/window_sorted_q08_rank_100k_partitions_window_sorted 1.73 63.3±0.77ms ? ?/sec 1.00 36.6±2.57ms ? ?/sec
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#discussion_r4247890221
##########
datafusion/physical-plan/src/sorts/partitioned_topk.rs:
##########
@@ -245,18 +261,40 @@ impl PartitionedTopKExec {
fetch: usize,
fn_kind: WindowFnKind,
) -> Result<Self> {
- let cache = Self::compute_properties(&input, expr.clone())?;
+ let cache =
+ Self::compute_properties(&input, expr.clone(), partition_prefix_len, None)?;
Ok(Self {
input,
expr,
partition_prefix_len,
fetch,
fn_kind,
+ ranking_field: None,
metrics_set: ExecutionPlanMetricsSet::new(),
cache: Arc::new(cache),
})
}
+ /// Append `field` to the output, holding each retained row's value for
+ /// [`Self::fn_kind`]'s ranking function, so the caller can drop the
+ /// `BoundedWindowAggExec` that would otherwise compute it.
+ ///
+ /// The output schema widens by that one column, so the plan properties
+ /// are recomputed under it.
+ pub fn with_ranking_field(self, field: FieldRef) -> Result<Self> {
Review Comment:
<img width="804" height="170" alt="image" src="https://github.com/user-attachments/assets/a1c677ea-9291-4ae8-be46-0fe12ef8a006" />
The PR description still says `try_new` takes a new `ranking_field` parameter and introduces a breaking API change, but its signature is unchanged. Could you update the description to reflect the new `with_ranking_field` builder and `ranking_field()` accessor, and replace the open question about function-name matching with the implemented built-in equality check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Partitioned topk emit row number [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26133:
URL: https://github.com/apache/datafusion/pull/26133#issuecomment-6190099990
Removing from the queue so we can get this one in first (to make CI pass)
- https://github.com/apache/datafusion/pull/26352
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org