Compact run buffer slice values

[Jefffrey]

Which issue does this PR close?

• Closes RunArray::slice() should align run_ends and values with the logical slice #10017

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[Jefffrey]

cc @Rich-T-kid this is what i was thinking on my previous comments on your PR

essentially we can still reduce how many physical values we hold on to, without needing to readjust all of the values (still keeping it zero-copy)

we'd need to extend it to runarray too since need to ensure the values array is similarly compacted (might need a bit of plumbing to make it work) and as noted in the issue callsites may need to be adjusted

lemme know your thoughts

[Rich-T-kid]

will take a look this afternoon!

[Jefffrey]

i realized my initial version of the PR incorrectly adjusted the offset when that wasnt needed; by removing the first physical value its as if the first run was consumed by the following run since we always list run lengths; that is

[4, 8] -> AAAABBBB
[8] -> {BBBB}BBBB

• where {BBBB} used to be a separate run but is now folded into the first run; this shouldnt matter as our logical offset points beyond it anyway

[Rich-T-kid]

Going to review this PR this week 👍

[Rich-T-kid]

@Jefffrey I took a look an im a bit confused. Im going to go through RunEndbuffer to refresh my knowledge. If you get a chance could you add a test similar to what we found in #9959 (comment). This way we validate that the slice gives us what we expect.

will try to give a more insightful review tomorrow

[Jefffrey]

@Jefffrey I took a look an im a bit confused. Im going to go through RunEndbuffer to refresh my knowledge. If you get a chance could you add a test similar to what we found in #9959 (comment). This way we validate that the slice gives us what we expect.

will try to give a more insightful review tomorrow

ill look into this; i didnt touch RunArray itself since it needs a bit of plumbing to make work but will try get around to it

[Jefffrey]

another thought i had is if we enforce this strictly (e.g. in all construction & validation methods) it can lead to simpler code; for example, values_slice() would become deprecated since it would be the same as values()

arrow-rs/arrow-array/src/array/run_array.rs

Lines 269 to 280 in 707872f

	/// Similar to [`values`] but accounts for logical slicing, returning only the values
	/// that are part of the logical slice of this array.
	///
	/// [`values`]: Self::values
	pub fn values_slice(&self) -> ArrayRef {
	if self.is_empty() {
	return self.values.slice(0, 0);
	}
	let start = self.get_start_physical_index();
	let end = self.get_end_physical_index();
	self.values.slice(start, end - start + 1)
	}

and get_start_physical_index()/get_end_physical_index() can be simplified to always getting 0/len - 1 if we know the run ends buffer strictly keeps only needed physical values, essentially skipping the binary search

arrow-rs/arrow-buffer/src/buffer/run.rs

Lines 242 to 267 in 707872f

	/// Returns the physical index at which the logical array starts.
	///
	/// The same as calling `get_physical_index(0)` but with a fast path if the
	/// buffer is not logically sliced, in which case it always returns `0`.
	pub fn get_start_physical_index(&self) -> usize {
	if self.logical_offset == 0 || self.logical_length == 0 {
	return 0;
	}
	// Fallback to binary search
	self.get_physical_index(0)
	}
	/// Returns the physical index at which the logical array ends.
	///
	/// The same as calling `get_physical_index(length - 1)` but with a fast path
	/// if the buffer is not logically sliced, in which case it returns `length - 1`.
	pub fn get_end_physical_index(&self) -> usize {
	if self.logical_length == 0 {
	return 0;
	}
	if self.max_value() == self.logical_offset + self.logical_length {
	return self.values().len() - 1;
	}
	// Fallback to binary search
	self.get_physical_index(self.logical_length - 1)
	}

[Jefffrey]

can reference this test to see how the physical values change underneath as we slice