Add support `StringView` / `BinaryView` in `interleave` kernel #6779

onursatici · 2024-11-23T12:09:02Z

Which issue does this PR close?

Closes StringView: Using the Interleave kernel (and potentially others) results in many repeated buffers in variadic_buffers #6780

Rationale for this change

Currently interleaving ByteViewArrays are done with the fallback implementation, which uses a MutableArrayBuilder. The extend method on this builder is copying over all variadic buffers because it doesn't know if there are buffers not referenced by any views in the array. Especially on datafusion's TopK implementation, which uses an heap that interleaves arrow arrays to produce the top k rows, current interleave implementation results in an explosion of variadic buffer count for byte view arrays, adding the same set of buffers over and over again. Where this becomes really problematic is when sending such arrays over flight, current encoder materialises all variadic buffers.

What changes are included in this PR?

Add a ByteViewArray specific interleave implementation that does not add a previously referenced variadic buffer when building the interleaved array

Are there any user-facing changes?

onursatici · 2024-11-23T12:09:56Z

arrow-select/src/interleave.rs

+    }
+
+    let array =
+        GenericByteViewArray::<T>::try_new(views_builder.into(), buffers, interleaved.nulls)?;


should we do new_unchecked here instead similar to other specific interleave methods?

Yes probably

alamb · 2024-11-24T21:56:43Z

Thanks @onursatici -- this looks very cool. I hope to review it in the next few days

tustvold

I've not been following the StringView work very closely but this appears to be a similar approach to #6154 and have the same broad issue.

I think we probably need a more holistic think about how we should be handling deduplicating array views, that can then be applied across the selection kernels, much like we have for dictionaries.

There are lots of ways this could be done, and it is certainly possible that no general purpose solution is possible and we need a more sophisticated API for this #6692

tustvold · 2024-11-26T10:02:09Z

arrow-select/src/interleave.rs

+) -> Result<ArrayRef, ArrowError> {
+    let interleaved = Interleave::<'_, GenericByteViewArray<T>>::new(values, indices);
+    let mut views_builder = BufferBuilder::new(indices.len());
+    let mut buffers = Vec::with_capacity(values[0].len());


Why is this the capacity?

tustvold · 2024-11-26T10:02:34Z

arrow-select/src/interleave.rs

+    }
+
+    let array =
+        GenericByteViewArray::<T>::try_new(views_builder.into(), buffers, interleaved.nulls)?;


Yes probably

tustvold · 2024-11-26T10:03:52Z

arrow-select/src/interleave.rs

+        let fallback_result = fallback.as_string_view();
+        // as of commit 97055631, assertion below, commented out to not block future improvements, passes:
+        // note that fallback_result has 2 buffers, but only one long enough string to warrant a buffer
+        // assert_eq!(fallback_result.data_buffers().len(), 2);


Suggested change

// assert_eq!(fallback_result.data_buffers().len(), 2);

assert_eq!(fallback_result.data_buffers().len(), 2);

Ultimately if this starts failing it indicates improvements have been made that might make the specialized impl redundant

tustvold · 2024-11-26T10:11:11Z

arrow-select/src/interleave.rs

+    let mut views_builder = BufferBuilder::new(indices.len());
+    let mut buffers = Vec::with_capacity(values[0].len());
+
+    let mut buffer_lookup = HashMap::new();


This misunderstands #6780, the issue isn't not skipping buffers that aren't referenced, it is that the arrays being interleaved may contain the same actual buffers, e.g. sourced from the same parquet dictionary page. This needs to deduplicate based on the underlying Buffer pointers.

I see, I have filed #6808 to deduplicate the buffers while building the interleaved array in the fallback implementation.

I am not sure if that would completely remove the need for this PR though, it should guarantee that no duplicate buffers should exist on the interleaved array, but the fallback implementation would still have 2 buffers in the test below, because those two buffers are unique. It feels like this PR and #6808 are complementary

Can you also please add some comments about what the buffer lookup represents? I think it is

// (input array_index, input buffer_index) --> output array buffer_index

alamb

Thank you @onursatici -- I apologize for the delay in reviewing.

I have reviewed the code and I think it looks quite good. I left some comments on this PR about some additional testing I think is needed.

If you are willing to implement that additional testing, I think this PR will be good to go (I agree it is potentially complimentary with #6808)

alamb · 2024-12-18T13:13:03Z

arrow-select/src/interleave.rs

+        let result = values.as_string_view();
+        assert_eq!(result.data_buffers().len(), 1);
+
+        // Test fallback implementation


Since interleave_fallback isn't pub I don't think we should be testing it directly. Intead you should arrange the paramters to call it directly

I removed the comment, it was a bit misleading. I was not trying to test the fallback implementation here, but was trying to use it to show that it does produce the same results with the new implementation, but with more buffers.

With the new implementation I don't think it is straightforward to call the fallback implementation using the public methods now

alamb · 2024-12-18T13:15:43Z

arrow-select/src/interleave.rs

+            (1, 0), // "test"
+            (0, 4), // "baz"
+            (1, 3), // "views"
+            (0, 1), // "world_long_string_not_inlined"


since the test only picks one long string, only one buffer is going to be copied (the other strings are inlined)

To provoke the issue described in the ticket, I think you need to interleave that same long string from the two different buffers. Something like

let indices = &[ (0, 1), // "world_long_string_not_inlined" (view_a) (1, 2), // "world_long_string_not_inlined" (view_b) (0, 1), // "world_long_string_not_inlined" (view_a) (1, 2), // "world_long_string_not_inlined" (view_b) (0, 1), // "world_long_string_not_inlined" (view_a) (1, 2), // "world_long_string_not_inlined" (view_b) (0, 1), // "world_long_string_not_inlined" (view_a) (1, 2), // "world_long_string_not_inlined" (view_b) (0, 1), // "world_long_string_not_inlined" (view_a) (1, 2), // "world_long_string_not_inlined" (view_b)

And make sure the output view only has 2 buffers (the one from view_a and the one from view_b)

I think the issue with the fallback implementation was that it did copy all buffers from all inputs, even if no views on the output referred them. I believe this test shows that issue well, when we run the fallback implementation, it does end up having two buffers although only one is referenced in the output array. When run with the new implementation specific to views, it does correctly prune the unused buffer from the second input array.

arrow-select/src/interleave.rs

alamb · 2024-12-18T13:22:24Z

arrow-select/src/interleave.rs

+    let mut views_builder = BufferBuilder::new(indices.len());
+    let mut buffers = Vec::with_capacity(values[0].len());
+
+    let mut buffer_lookup = HashMap::new();


Can you also please add some comments about what the buffer lookup represents? I think it is

// (input array_index, input buffer_index) --> output array buffer_index

arrow-select/src/interleave.rs

add byteview specific interleave

27ac654

onursatici commented Nov 23, 2024

View reviewed changes

github-actions bot added the arrow Changes to the arrow crate label Nov 23, 2024

alamb mentioned this pull request Nov 23, 2024

StringView: Using the Interleave kernel (and potentially others) results in many repeated buffers in variadic_buffers #6780

Open

onursatici added 2 commits November 24, 2024 10:08

clippy

2052cb8

test

8b11191

onursatici marked this pull request as ready for review November 24, 2024 10:22

more clippy

d5daedc

more test coverage

6ff5c0a

tustvold reviewed Nov 26, 2024

View reviewed changes

enable assertion, remove explicit vector capacity

763a792

onursatici mentioned this pull request Nov 27, 2024

deduplicate variadic buffers in MutableArrayData::extend for ByteView arrays #6808

Open

alamb reviewed Dec 18, 2024

View reviewed changes

alamb changed the title ~~add byteview specific interleave~~ Add support StringView / BinaryView in interleave kernel Dec 18, 2024

add new test, address comments

7aa95d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support `StringView` / `BinaryView` in `interleave` kernel #6779

Add support `StringView` / `BinaryView` in `interleave` kernel #6779

onursatici commented Nov 23, 2024 •

edited

Loading

onursatici Nov 23, 2024

tustvold Nov 26, 2024

alamb commented Nov 24, 2024

tustvold left a comment

tustvold Nov 26, 2024

tustvold Nov 26, 2024

tustvold Nov 26, 2024

tustvold Nov 26, 2024 •

edited

Loading

onursatici Nov 27, 2024

alamb Dec 18, 2024

alamb left a comment

alamb Dec 18, 2024

onursatici Dec 21, 2024

alamb Dec 18, 2024

onursatici Dec 21, 2024

alamb Dec 18, 2024

	// assert_eq!(fallback_result.data_buffers().len(), 2);
	assert_eq!(fallback_result.data_buffers().len(), 2);

Add support StringView / BinaryView in interleave kernel #6779

Are you sure you want to change the base?

Add support StringView / BinaryView in interleave kernel #6779

Conversation

onursatici commented Nov 23, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 24, 2024

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add support `StringView` / `BinaryView` in `interleave` kernel #6779

Add support `StringView` / `BinaryView` in `interleave` kernel #6779

onursatici commented Nov 23, 2024 •

edited

Loading

tustvold Nov 26, 2024 •

edited

Loading