You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "nathanb9 (via GitHub)" <gi...@apache.org> on 2026/09/23 03:47:54 UTC
[PR] Make RowValues::compare's current-offset assumption self-checking [datafusion]
nathanb9 opened a new pull request, #25643:
URL: https://github.com/apache/datafusion/pull/25643
## Which issue does this PR close?
- Closes #25097.
## Rationale for this change
`RowValues::compare` ignores its `l_idx`/`r_idx` arguments and compares cached current-row slices. Correct today, but the narrowing of the `CursorValues` contract was unenforced — a future caller passing arbitrary indices would silently produce a wrong merge order.
## What changes are included in this PR?
- Debug-only `current_offset` field on `RowValues`, recorded in `set_offset`, asserted against `l_idx`/`r_idx` in `compare`. Zero cost in release.
- Impl-level docs stating the deliberately narrowed contract.
- A `#[should_panic]` test covering the new assert.
## Are these changes tested?
Yes — new debug-only test; existing `sorts::` tests (124) pass with the asserts active.
## Are there any user-facing changes?
No.
## Possible later work
- `ArrayValues` forwards arbitrary indices by design; if `RowValues` is ever wrapped similarly, a fallback to `rows.row(idx)` could restore the full trait contract (rejected here to keep the merge hot path branch-free).
--
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] Make `RowValues::compare`'s current-offset assumption self-checking [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25643:
URL: https://github.com/apache/datafusion/pull/25643#issuecomment-5788922435
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25643?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 82.45%. Comparing base ([`ebdb657`](https://app.codecov.io/gh/apache/datafusion/commit/ebdb657777d512b20ad40f50ce8fb85a39ee3175?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1f59c63`](https://app.codecov.io/gh/apache/datafusion/commit/1f59c63c1686733ba7f4a5bdd35d4e6b2666616a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25643 +/- ##
=======================================
Coverage 82.45% 82.45%
=======================================
Files 1140 1140
Lines 436434 436445 +11
Branches 436434 436445 +11
=======================================
+ Hits 359840 359856 +16
+ Misses 54840 54832 -8
- Partials 21754 21757 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25643?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] Make `RowValues::compare`'s current-offset assumption self-checking [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #25643:
URL: https://github.com/apache/datafusion/pull/25643#discussion_r4100414630
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -192,6 +192,12 @@ pub struct RowValues {
/// Cached byte length for the current row.
current_len: usize,
+ /// The offset the cache was last refreshed for. Debug builds only: used
+ /// by [`CursorValues::compare`] to assert callers only compare at the
+ /// current offset (the narrowed contract this impl relies on).
+ #[cfg(debug_assertions)]
+ current_offset: usize,
Review Comment:
Worth saying in the PR description that gating the field on `debug_assertions` is deliberate rather than tidiness. `Cursor<RowValues>` is the struct the loser tree shuffles on every comparison, so eight extra bytes is the kind of thing that can land on the wrong side of a cache line. Keeping the release layout byte-identical is the reason to cfg a field that a `debug_assert_eq!` would already compile away on its own.
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -228,6 +234,8 @@ impl RowValues {
len,
current_ptr,
current_len,
+ #[cfg(debug_assertions)]
+ current_offset: 0,
Review Comment:
This initialiser now has to agree with something about forty lines away in a different impl: `Cursor::new` builds `Self { offset: 0, values }` and never calls `set_offset`, so it relies entirely on the constructor having seeded the cache for row 0. That was already true of `current_ptr`/`current_len`; this adds a third field to the same silent contract.
A line here saying the three fields must describe the same row, and that `Cursor::new` depends on that row being 0, would help. If someone later reorders the constructor, the assert starts firing from a call site that has nothing to do with the mistake.
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -267,9 +280,21 @@ impl CursorValues for RowValues {
}
fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering {
- // Merge callers always compare at current offsets; the cache is up
- // to date. (Debug-only: verify the invariant.)
- debug_assert!(l_idx < l.len && r_idx < r.len);
+ // Narrowed contract (see impl docs): merge callers always compare at
+ // the current offsets, so the cached slices are the requested rows.
+ // Debug builds verify the indices match the offsets the cache was
+ // refreshed for; zero cost in release.
+ #[cfg(debug_assertions)]
+ {
+ debug_assert_eq!(
+ l_idx, l.current_offset,
Review Comment:
One case worth confirming is intentional: `advance()` skips `set_offset` once the cursor runs off the end, so a finished cursor sits at `offset == len` with `current_offset == len - 1`, and this assert would fire on it.
That is not a regression — the previous `debug_assert!(l_idx < l.len)` would have fired on the same state — and the comment in `advance()` already says a finished cursor's stale cache is never read. Just worth a word in the description so a reviewer meeting the assert does not have to rediscover that the guard is upheld one layer up.
--
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] Make `RowValues::compare`'s current-offset assumption self-checking [datafusion]
Posted by "nathanb9 (via GitHub)" <gi...@apache.org>.
nathanb9 commented on code in PR #25643:
URL: https://github.com/apache/datafusion/pull/25643#discussion_r4110133509
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -228,6 +234,8 @@ impl RowValues {
len,
current_ptr,
current_len,
+ #[cfg(debug_assertions)]
+ current_offset: 0,
Review Comment:
Added a comment
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -267,9 +280,21 @@ impl CursorValues for RowValues {
}
fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering {
- // Merge callers always compare at current offsets; the cache is up
- // to date. (Debug-only: verify the invariant.)
- debug_assert!(l_idx < l.len && r_idx < r.len);
+ // Narrowed contract (see impl docs): merge callers always compare at
+ // the current offsets, so the cached slices are the requested rows.
+ // Debug builds verify the indices match the offsets the cache was
+ // refreshed for; zero cost in release.
+ #[cfg(debug_assertions)]
+ {
+ debug_assert_eq!(
+ l_idx, l.current_offset,
Review Comment:
I've added this explanation to the comment block above the asserts
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -192,6 +192,12 @@ pub struct RowValues {
/// Cached byte length for the current row.
current_len: usize,
+ /// The offset the cache was last refreshed for. Debug builds only: used
+ /// by [`CursorValues::compare`] to assert callers only compare at the
+ /// current offset (the narrowed contract this impl relies on).
+ #[cfg(debug_assertions)]
+ current_offset: usize,
Review Comment:
I've expanded the field's doc comment to spell it out: the `debug_assert_eq!` would compile away on its own, but the field itself would still widen `Cursor<RowValues>` by eight bytes in release, and that struct is what the loser tree shuffles on every comparison. The cfg keeps the release layout byte-identical.
I'll mention it in the description as well.
--
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] Make `RowValues::compare`'s current-offset assumption self-checking [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25643:
URL: https://github.com/apache/datafusion/pull/25643#issuecomment-5935696697
Thank you @nathanb9 and @zhuqi-lucas
--
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] Make `RowValues::compare`'s current-offset assumption self-checking [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #25643:
URL: https://github.com/apache/datafusion/pull/25643
--
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