fix:[9018]Fixed RunArray slice offsets by manishkr · Pull Request #9036 · apache/arrow-rs

manishkr · 2025-12-23T13:01:06Z

Which issue does this PR close?

Part of Kernels don't account for slicing of RunArrays #9018 .

Rationale for this change

To consider offset in slicing of RunArray.

What changes are included in this PR?

Considered offset in slicing of RunArray.
Enhanced RunArray slice API.

Are these changes tested?

yes

Are there any user-facing changes?

Yes, extended API to access RunArray slices directly than getting it from index.

manishkr · 2025-12-23T14:31:32Z

Looking into test(intermittent) case failure.

manishkr · 2025-12-24T14:05:08Z

Need help on.
Archery test With other arrows
The hosted runner lost communication with the server. Anything in your workflow that terminates the runner process, starves it for CPU/Memory, or blocks its network access can cause this error.

Jefffrey · 2025-12-24T14:26:19Z

Need help on. Archery test With other arrows The hosted runner lost communication with the server. Anything in your workflow that terminates the runner process, starves it for CPU/Memory, or blocks its network access can cause this error.

Can ignore, it's an unrelated issue: #9024

Jefffrey

Thanks for picking this up. Left some comments, also changed PR body to not close original issue as I still need to check what other kernels/code might need fixing.

arrow-select/src/filter.rs

arrow-array/src/array/run_array.rs

arrow-buffer/src/buffer/run.rs

arrow-select/src/concat.rs

manishkr · 2025-12-27T07:01:06Z

Thanks for picking this up. Left some comments, also changed PR body to not close original issue as I still need to check what other kernels/code might need fixing.

Thanks for reviewing and feedback. If you permit, once this PR is good to go, can take this.

arrow-select/src/concat.rs

arrow-select/src/filter.rs

Jefffrey · 2025-12-30T02:30:20Z

arrow-buffer/src/buffer/run.rs

+    /// Similar to [`values`] but accounts for logical slicing, returning only the values
+    /// that are part of the logical slice of this buffer.
+    ///
+    /// [`values`]: Self::values
+    pub fn values_slice(&self) -> &[E] {


I think for this docstring we also need a clear warning note that the first & last run values may be inaccurate as they don't take into account the logical slicing. I wonder if there is a better way to handle this without too much overhead 🤔

Thanks. Added warning but not sure logical run ends API(logical_run_ends) is needed or not, but added it. Please feel free to suggest to remove it.

I quite like the logical_run_ends() API actually; it makes it harder to unintentionally misuse values_slice(). I'm thinking we remove values_slice() and expose only logical_run_ends(), since users can always derive values_slice() for themselves manually anyway.

My only comment is perhaps rename logical_run_ends() to something else, since it might be confusing with how we describe physical vs logical in the docstring for the struct. Perhaps something like sliced_values()?

Thanks. Removed values_slice and renamed logical_run_ends to sliced_values. Without values_slice, it is unclear whether adding get_physical_len would be useful for end users?

Without values_slice, it is unclear whether adding get_physical_len would be useful for end users?

Could you elaborate on this? I don't recall discussing adding a get_physical_len() API 🤔

It's my suggestion. I was thinking, would a get_physical_len API be useful for end users?

It's probably fine to omit for now, I don't see a use for it

manishkr · 2026-01-05T06:13:52Z

@Jefffrey Please help to close this. Thanks

Jefffrey · 2026-01-05T06:24:14Z

I'll try take another look soon.

Jefffrey

Apologies it took me so long to re-review, I'll try to prioritise this so we can get it over the line 👍

Jefffrey · 2026-01-10T11:30:01Z

arrow-buffer/src/buffer/run.rs

+    /// Similar to [`values`] but accounts for logical slicing, returning only the values
+    /// that are part of the logical slice of this buffer.
+    ///
+    /// [`values`]: Self::values
+    pub fn values_slice(&self) -> &[E] {


I quite like the logical_run_ends() API actually; it makes it harder to unintentionally misuse values_slice(). I'm thinking we remove values_slice() and expose only logical_run_ends(), since users can always derive values_slice() for themselves manually anyway.

My only comment is perhaps rename logical_run_ends() to something else, since it might be confusing with how we describe physical vs logical in the docstring for the struct. Perhaps something like sliced_values()?

arrow-select/src/concat.rs

Jefffrey · 2026-01-11T11:06:49Z

arrow-buffer/src/buffer/run.rs

        assert_eq!(buffer.get_end_physical_index(), 0);
    }
+    #[test]
+    fn test_sliced_buffer_access() {


This test probably can be removed now that we don't have values_slice()

Done. Thanks

Jefffrey · 2026-01-11T11:10:29Z

arrow-buffer/src/buffer/run.rs

+    /// Similar to [`values`] but accounts for logical slicing, returning only the values
+    /// that are part of the logical slice of this buffer.
+    ///
+    /// [`values`]: Self::values
+    pub fn values_slice(&self) -> &[E] {


Without values_slice, it is unclear whether adding get_physical_len would be useful for end users?

Could you elaborate on this? I don't recall discussing adding a get_physical_len() API 🤔

manishkr · 2026-01-11T11:50:23Z

@Jefffrey Thank you for approving this request. I appreciate your feedback, which helps improve it with each iteration.

Jefffrey · 2026-01-13T05:18:26Z

Thanks @manishkr

# Which issue does this PR close?  - Part of apache#9018 . # Rationale for this change  To consider offset in slicing of RunArray. # What changes are included in this PR?  1. Considered offset in slicing of RunArray. 2. Enhanced RunArray slice API. # Are these changes tested?  yes # Are there any user-facing changes?  Yes, extended API to access RunArray slices directly than getting it from index.

github-actions bot added the arrow Changes to the arrow crate label Dec 23, 2025

manishkr force-pushed the 9018-fix-runarray-kernel branch from b17d380 to d9e54fb Compare December 24, 2025 12:31

Jefffrey reviewed Dec 26, 2025

View reviewed changes

manishkr force-pushed the 9018-fix-runarray-kernel branch from d9e54fb to 6fb02cd Compare December 27, 2025 06:55

Jefffrey reviewed Dec 30, 2025

View reviewed changes

manishkr force-pushed the 9018-fix-runarray-kernel branch from 6fb02cd to 57e6a3c Compare December 30, 2025 07:36

Jefffrey reviewed Jan 10, 2026

View reviewed changes

manishkr force-pushed the 9018-fix-runarray-kernel branch from 57e6a3c to a2b3af9 Compare January 11, 2026 09:49

Jefffrey approved these changes Jan 11, 2026

View reviewed changes

fix:[9018]Fixed RunArray slice offsets

bfbe554

manishkr force-pushed the 9018-fix-runarray-kernel branch from a2b3af9 to bfbe554 Compare January 11, 2026 11:44

Jefffrey merged commit b904318 into apache:main Jan 13, 2026
26 checks passed

manishkr deleted the 9018-fix-runarray-kernel branch January 13, 2026 05:23

Jefffrey mentioned this pull request Jan 16, 2026

fix: avoid panic in RunEndBuffer::sliced_values for empty run arrays #9200

Closed

Jefffrey mentioned this pull request Feb 13, 2026

Implement min, max, sum for run-end-encoded arrays. #9409

Merged

Conversation

manishkr commented Dec 23, 2025 • edited by Jefffrey Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

manishkr commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manishkr commented Dec 24, 2025

Uh oh!

Jefffrey commented Dec 24, 2025

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

manishkr commented Dec 27, 2025

Uh oh!

Uh oh!

Uh oh!

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!

manishkr commented Jan 5, 2026

Uh oh!

Jefffrey commented Jan 5, 2026

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

manishkr commented Jan 11, 2026

Uh oh!

Uh oh!

Jefffrey commented Jan 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

manishkr commented Dec 23, 2025 •

edited by Jefffrey

Loading

manishkr commented Dec 23, 2025 •

edited

Loading