Add support `StringView` / `BinaryView` in `interleave` kernel by onursatici · Pull Request #6779 · apache/arrow-rs

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

+    }
+
+    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

+) -> 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

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


Yes probably

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

+        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

+    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

+        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

+            (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.

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

+    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

alamb

Thank you @onursatici -- I reviewed this PR again and I think it looks quite nice.

Sorry for the very long review cycle

…e#6779) * add byteview specific interleave * clippy * test * more clippy * more test coverage * enable assertion, remove explicit vector capacity * add new test, address comments

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

Closed

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

Closed

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

alamb mentioned this pull request Jan 11, 2025

Release arrow-rs / parquet minor version 53.4.0 (Jan 2025) apache/arrow-rs-object-store#30

Closed

14 tasks

alamb approved these changes Jan 17, 2025

View reviewed changes

alamb merged commit af777cd into apache:main Jan 17, 2025

Kontinuation mentioned this pull request Feb 13, 2025

External sorting not working for (maybe only for string columns??) apache/datafusion#12136

Closed

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

Conversation

onursatici commented Nov 23, 2024 • edited 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 there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 24, 2024

Uh oh!

tustvold left a comment

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!

tustvold Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

alamb left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

onursatici commented Nov 23, 2024 •

edited

Loading

tustvold Nov 26, 2024 •

edited

Loading