Implement `GenericByteViewArray::gc` for compacting StringViewArray and ByteViewArray #5707

ClSlaid · 2024-05-02T08:33:34Z

Which issue does this PR close?

~~Closes #5513.~~

This PR is being splited into:

Implement compaction check. Adding compact check functionality to GenericByteViewArray #5720
Gabage Collection on non-compact buffers.

Rationale for this change

What changes are included in this PR?

Adding a new gc method for StringViewArray and ByteViewArray.

Are there any user-facing changes?

None.

part 1: implement checker to check if current buffer is compact Signed-off-by: 蔡略 <cailue@bupt.edu.cn>

part2: implement actual gc algorithm Signed-off-by: 蔡略 <cailue@bupt.edu.cn>

Signed-off-by: 蔡略 <cailue@bupt.edu.cn>

ClSlaid · 2024-05-02T08:53:24Z

The naming of gc might persuade people that it should modify itself. So should I:

making gc in place and edit itself

ou or

renaming gc to compact?

alamb · 2024-05-02T19:49:18Z

Thank you -- looking forward to this one.

ClSlaid · 2024-05-03T00:57:15Z

arrow-array/src/array/byte_view_array.rs

+/// A helper struct that used to check if the array is compact view
+///
+/// The checker is lazy and will not check the array until `finish` is called.
+///
+/// This is based on the assumption that the array will most likely to be not compact,
+/// so it is likely to scan the entire array.
+///
+/// Then it is better to do the check at once, rather than doing it for each accumulate operation.
+struct CompactChecker {
+    length: usize,
+    intervals: BTreeMap<usize, usize>,
+}


I wonder if there is a better algorithm for this.
It's the most straightforward and simplest way I could come up with, though.

I recommend we pull the "compact checking" algorithm into a new PR to discuss it -- I am not sure about the assumption that StringViewArrays will mostly often be compacted (I would actually expect the opposite)

Signed-off-by: 蔡略 <cailue@bupt.edu.cn>

RinChanNOWWW · 2024-05-04T03:21:59Z

arrow-array/src/array/byte_view_array.rs

+        }
+
+        let mut new_views = Vec::with_capacity(self.views.len());
+        let mut new_bufs: Vec<Vec<u8>> = vec![vec![]; self.buffers.len()];


The number of buffers may shrink after gc. Every buffer should be filled up to block_size.

See GenericByteViewBuilder::append_value.

alamb

Thank you for this contributon @ClSlaid and for the review @RinChanNOWWW

My biggest suggestion is to split the code in this PR into two PRs -- one for the "compact_check" and one for the actual compaction.

I had some suggestions on some additional coverage needed for the gc, but I think it is looking quite close.

Thanks again!

alamb · 2024-05-04T11:53:15Z

arrow-array/src/array/byte_view_array.rs

+/// A helper struct that used to check if the array is compact view
+///
+/// The checker is lazy and will not check the array until `finish` is called.
+///
+/// This is based on the assumption that the array will most likely to be not compact,
+/// so it is likely to scan the entire array.
+///
+/// Then it is better to do the check at once, rather than doing it for each accumulate operation.
+struct CompactChecker {
+    length: usize,
+    intervals: BTreeMap<usize, usize>,
+}


I recommend we pull the "compact checking" algorithm into a new PR to discuss it -- I am not sure about the assumption that StringViewArrays will mostly often be compacted (I would actually expect the opposite)

alamb · 2024-05-04T11:58:15Z

arrow-array/src/array/byte_view_array.rs

+        let mut view = ByteView::from(array.views[1]);
+        view.length = 15;
+        let new_views = ScalarBuffer::from(vec![array.views[0], view.into()]);


I dn't undertand what this is doing

I think you can more easily create a stringview with multiple buffers like this:

arrow-rs/arrow-cast/src/pretty.rs

Lines 334 to 341 in 905c46b

// Use a small capacity so we end up with multiple views

let mut builder = StringViewBuilder::with_capacity(20);

builder.append_value("hello");

builder.append_null();

builder.append_value("longer than 12 bytes");

builder.append_value("another than 12 bytes");

builder.append_null();

builder.append_value("small");

Not familiar with the rest of the code, so I made it out brutally.

that StringViewArrays will mostly often be compacted (I would actually expect the opposite)

We assume the same idea, it's likely to be not compacted.

ClSlaid · 2024-05-04T12:07:30Z

Thanks for your review @alamb, and I'd left this PR to a draft and reopen two PRs for this.

ClSlaid added 2 commits May 2, 2024 15:01

feat: support gc for view_arrays

25197a6

part 1: implement checker to check if current buffer is compact Signed-off-by: 蔡略 <cailue@bupt.edu.cn>

feat: add gc to view-arrays

9072c20

part2: implement actual gc algorithm Signed-off-by: 蔡略 <cailue@bupt.edu.cn>

github-actions bot added the arrow Changes to the arrow crate label May 2, 2024

ClSlaid marked this pull request as draft May 2, 2024 08:35

patch: add more tests

c45acb8

Signed-off-by: 蔡略 <cailue@bupt.edu.cn>

ClSlaid force-pushed the feat/byte-view-array/gc branch from 04efa78 to c45acb8 Compare May 2, 2024 08:41

ClSlaid marked this pull request as ready for review May 2, 2024 08:41

alamb mentioned this pull request May 2, 2024

DataFusion weekly project plan (Andrew Lamb) - April 29, 2024 apache/datafusion#10283

Closed

8 tasks

ClSlaid commented May 3, 2024

View reviewed changes

ClSlaid force-pushed the feat/byte-view-array/gc branch from 7e47a1d to 73ab8a3 Compare May 3, 2024 01:01

patch: improve robustness

6d2a453

Signed-off-by: 蔡略 <cailue@bupt.edu.cn>

ClSlaid force-pushed the feat/byte-view-array/gc branch from 73ab8a3 to 6d2a453 Compare May 3, 2024 01:10

RinChanNOWWW reviewed May 4, 2024

View reviewed changes

alamb changed the title ~~feat: byte view array gc~~ Implement GenericByteViewArray::gc for compacting StringViewArray and ByteViewArray May 4, 2024

alamb reviewed May 4, 2024

View reviewed changes

ClSlaid marked this pull request as draft May 4, 2024 12:07

ClSlaid mentioned this pull request May 4, 2024

Adding compact check functionality to GenericByteViewArray #5720

Draft

This was referenced May 6, 2024

DataFusion weekly project plan (Andrew Lamb) - May 6, 2024 apache/datafusion#10395

Closed

Encapsulate View logic for GenericByteViewArray #5619

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `GenericByteViewArray::gc` for compacting StringViewArray and ByteViewArray #5707

Implement `GenericByteViewArray::gc` for compacting StringViewArray and ByteViewArray #5707

ClSlaid commented May 2, 2024 •

edited

ClSlaid commented May 2, 2024 •

edited

alamb commented May 2, 2024

ClSlaid May 3, 2024 •

edited

alamb May 4, 2024

RinChanNOWWW May 4, 2024

alamb left a comment

alamb May 4, 2024

alamb May 4, 2024

ClSlaid May 4, 2024

ClSlaid May 4, 2024

ClSlaid commented May 4, 2024

	// Use a small capacity so we end up with multiple views
	let mut builder = StringViewBuilder::with_capacity(20);
	builder.append_value("hello");
	builder.append_null();
	builder.append_value("longer than 12 bytes");
	builder.append_value("another than 12 bytes");
	builder.append_null();
	builder.append_value("small");

Implement GenericByteViewArray::gc for compacting StringViewArray and ByteViewArray #5707

Are you sure you want to change the base?

Implement GenericByteViewArray::gc for compacting StringViewArray and ByteViewArray #5707

Conversation

ClSlaid commented May 2, 2024 • edited

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

ClSlaid commented May 2, 2024 • edited

alamb commented May 2, 2024

ClSlaid May 3, 2024 • edited

Choose a reason for hiding this comment

alamb May 4, 2024

Choose a reason for hiding this comment

RinChanNOWWW May 4, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb May 4, 2024

Choose a reason for hiding this comment

alamb May 4, 2024

Choose a reason for hiding this comment

ClSlaid May 4, 2024

Choose a reason for hiding this comment

ClSlaid May 4, 2024

Choose a reason for hiding this comment

ClSlaid commented May 4, 2024

Implement `GenericByteViewArray::gc` for compacting StringViewArray and ByteViewArray #5707

Implement `GenericByteViewArray::gc` for compacting StringViewArray and ByteViewArray #5707

ClSlaid commented May 2, 2024 •

edited

ClSlaid commented May 2, 2024 •

edited

ClSlaid May 3, 2024 •

edited