refactor(type): refactor `ListValue` using array #13402

wangrunji0408 · 2023-11-14T03:06:37Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR changes the inner structure of ListValue from Box<[Datum]> to ArrayImpl. Now creating a ListValue requires its type in advance, just like array.

Pros:

Elements in the list are forced to be the same type. And we can get the type.
ListRef can be unified to a slice of an array.
The unnest operation naturally becomes a slice of underlying array.
Memory allocations for varchar[] values have been significantly reduced. (n -> 3)

Cons:

It needs more memory allocations for primitive type lists. (1 -> 3 if has null, -> 2 if non null)
It consumes more memory for small lists. (roughly when n <= 4, see the chart below)

For example, {1, null} :: bigint[] currently takes 1 malloc + 48 bytes, and will take 3 mallocs + 96 bytes after this PR.

// before: heap size = 48
Box<[Datum]>            // 24 * 2

// after:  heap size = 96
Box<ArrayImpl::Int64 {  // 72
    bitmap: Bitmap,     // 8
    values: Box<[i64]>, // 8 * 2
}>

I'm not 100% sure if this refactoring is a positive one. It seems worthwhile if we assume the list is not too small, and there are considerable uses for string lists. 🤔

This PR also:

reduces the size of ArrayImpl from 88 bytes to 72 bytes.
adds a lot of ListValue::from_iter functions to simplify list construction in unit tests.
rewrites array_agg in stateful style, because ListValue becomes immutable.
implements Clone and EstimateSize for array builders, as required by 3.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Signed-off-by: Runji Wang <[email protected]>

liurenjie1024 · 2023-11-14T10:24:08Z

I think most operations are operated on ListArray rather ListValue? Is there any special case you want to improve?

wangrunji0408 · 2023-11-14T10:36:20Z

I think most operations are operated on ListArray rather ListValue? Is there any special case you want to improve?

Yes. The main purpose of this change is to ensure the same type for all elements. And it seems natural to reuse arrays in this case. There are 2 expressions (some-all and array-transform) that may benefit from the union on ListValue and array. Except them, there seems to be no other cases that can be improved in performance.

liurenjie1024 · 2023-11-14T11:15:32Z

The main purpose of this change is to ensure the same type for all elements. And it seems natural to reuse arrays in this case.

This sounds reasonable to me.

I think it's ok to move on.

TennyZhuang

Too many changes. Generally LGTM

src/common/src/array/arrow.rs

src/common/src/array/list_array.rs

BugenZhao · 2023-11-15T06:12:54Z

reduces the size of ArrayImpl from 88 bytes to 72 bytes.

Instead of Box<ArrayImpl>, what about boxing every variant of ArrayImpl? 🥵

It needs more memory allocations for primitive type lists. (1 -> 3 if has null, -> 2 if non null)

It consumes more memory for small lists. (roughly when n <= 4, see the chart below)

Array is not optimized for low cardinality. Is it possible to leverage something like smallvec as the small "list" optimization?

wangrunji0408 · 2023-11-15T06:30:52Z

Instead of Box<ArrayImpl>, what about boxing every variant of ArrayImpl? 🥵

Do you mean Box<dyn Array>? 🤔

Array is not optimized for low cardinality. Is it possible to leverage something like smallvec as the small "list" optimization?

It's possible. But it adds complexity and I'm not sure if it is worthy. 🥵

Anyway, considering that "most operations are operated on ListArray rather ListValue", I think a little performance degradation on small list is acceptable.

BugenZhao

Look great to me!!

src/common/src/array/list_array.rs

BugenZhao · 2023-11-15T06:23:09Z

src/common/src/array/list_array.rs

+impl FromIterator<bool> for ListValue {
+    fn from_iter<I: IntoIterator<Item = bool>>(iter: I) -> Self {
+        Self::new(iter.into_iter().collect::<BoolArray>().into())
+    }
+}


Is it possible to implement on generic scalar types?

Maybe. But I have a feeling that it requires an advanced type exercise. 🤪

BugenZhao · 2023-11-15T06:23:59Z

src/common/src/array/list_array.rs

+impl FromIterator<ListValue> for ListValue {
+    fn from_iter<I: IntoIterator<Item = ListValue>>(iter: I) -> Self {
+        Self::new(iter.into_iter().collect::<ListArray>().into())
+    }
+}


Will this be possibly misused?

Humm, I think it's clear that collecting a list of lists produces a list of lists. 😇

src/common/src/array/list_array.rs

BugenZhao · 2023-11-15T06:48:30Z

Do you mean Box<dyn Array>? 🤔

Nope. I mean

enum ArrayImpl {
  I16(Box<I16Array>)
  ...
}

It's possible. But it adds complexity and I'm not sure if it is worthy. 🥵

Anyway, considering that "most operations are operated on ListArray rather ListValue", I think a little performance degradation on small list is acceptable.

That makes sense. 👍 But it seems like we're heavily relying on ListValue and ArrayBuilder in array expressions. It will be better if we can...

use smallvec for builder,
or directly write to the list array builder.

wangrunji0408 · 2023-11-15T08:35:03Z

enum ArrayImpl {
  I16(Box<I16Array>)
  ...
}

Not recommended as it brings another indirection and malloc to other ArrayImpl usages.

or directly write to the list array builder.

Yes I'll do that in #13263.

Signed-off-by: Runji Wang <[email protected]>

stdrc

Rest looks great to me!

src/expr/impl/src/scalar/array_remove.rs

Signed-off-by: Runji Wang <[email protected]>

codecov · 2023-11-29T07:52:07Z

Codecov Report

Attention: 250 lines in your changes are missing coverage. Please review.

Comparison is base (ff90f8c) 68.31% compared to head (55369df) 68.28%.
Report is 5 commits behind head on main.

Files	Patch %	Lines
src/expr/core/src/expr/expr_some_all.rs	0.00%	43 Missing ⚠️
src/common/src/array/list_array.rs	85.66%	41 Missing ⚠️
src/common/src/field_generator/mod.rs	0.00%	24 Missing ⚠️
src/connector/src/parser/unified/avro.rs	0.00%	12 Missing ⚠️
src/expr/impl/src/scalar/regexp.rs	0.00%	10 Missing ⚠️
src/common/src/array/struct_array.rs	40.00%	9 Missing ⚠️
src/connector/src/parser/unified/json.rs	0.00%	8 Missing ⚠️
src/expr/impl/src/aggregate/bit_and.rs	0.00%	8 Missing ⚠️
src/expr/impl/src/aggregate/bit_or.rs	0.00%	8 Missing ⚠️
...pr/impl/src/aggregate/approx_count_distinct/mod.rs	0.00%	7 Missing ⚠️
... and 26 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13402      +/-   ##
==========================================
- Coverage   68.31%   68.28%   -0.04%     
==========================================
  Files        1523     1523              
  Lines      261834   261481     -353     
==========================================
- Hits       178879   178555     -324     
+ Misses      82955    82926      -29

Flag	Coverage Δ
rust	`68.28% <63.07%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xxchan · 2024-07-21T09:33:13Z

src/expr/impl/src/table_function/unnest.rs

 fn unnest(list: ListRef<'_>) -> impl Iterator<Item = Option<ScalarRefImpl<'_>>> {
-    list.flatten().into_iter()
+    list.flatten().iter()


Is this still suboptimal? We are still iterating over the list slices. Can we directly reinterpret the whole ListArray to the underlying ArrayImpl without iteration? 🤔

(BTW, I come here when I was thinking about expressions for the map type and found we don't seem to have vectorized exprs for list also. sth like https://github.com/apache/datafusion/pull/7825/files#diff-1cddf6dfddea9d7c2e299e973bf915176256222281028f2571b1662ed92cd58dR190-R195, other examples include our array_positions, which can be vectorized but not

Oh, ListArray::map_inner (which is only used to implement ArrayTransformExpression) looks like what I mean.

We are still iterating over the list slices. Can we directly reinterpret the whole ListArray to the underlying ArrayImpl without iteration? 🤔

I don't quite get it. Can you elaborate more?

Oh, you're referring to vectorized execution of unnest. The optimization seems too specific because it requires unnest to be the only expression in the ProjectSet operator.

Did you see map_inner?

map_inner manipulates the underlying entires child array of the list array directly. Generally, I feel we haven't utilize such optimization a lot. Not specific to unnest.

Not specific to unnest.

I wasn't specifically talking about unnest. I'm referring to the vectorized execution of any set-returning functions for list type. Not manipulating the offsets array only works if we don't need to know the output length of each list scalar, which requires that there's no other set-returning or scalar functions in the same ProjectSet.

Hmmm, actually I was only thinking about scalar functions.

But I'm not sure I understand what you mean. I feel it also work for ProjectSet:

Specifically, TableFunction will return a chunk with 2 columns (row_idx, and values ).

risingwave/src/expr/core/src/table_function/mod.rs

Lines 82 to 95 in dcb565e

/// ```text

/// select generate_series(1, x) from t(x);

///

/// # input chunk output chunks (TableFunction) output chunks (ProjectSet)

/// 1 --------------> 0 1 -------------------------> 0 1

/// 2 --------------> 1 1 -------------------------> 0 1

/// 3 ----┐ --- ---

/// │ 1 2 1 2

/// └---------> 2 1 -------------------------> 0 1

/// --- ---

/// 2 2 1 2

/// 2 3 2 3

/// row idx--^ ^--values projected_row_id--^ ^--values

/// ```

For unnest:

/// # input chunk output chunks /// [1,2] --------------> 0 1 /// [3,4,5] -----┐ 0 2 /// │ /// └--------> 1 3 /// 1 4 /// 1 5 /// row idx--^ ^--values

To build row_idx, we still need to iterate over list_array.iter() though, but values seems to be exactly the same as list_array.inner, so we can directly clone it. In the current implementation, we have an extra copy when building values.

Is this still suboptimal? We are still iterating over the list slices. Can we directly reinterpret the whole ListArray to the underlying ArrayImpl without iteration? 🤔

Yes, of course. However, this specialization has to be done with #[build_function]. The current implementation is more obvious.

but values seems to be exactly the same as list_array.inner, so we can directly clone it. In the current implementation, we have an extra copy when building values.

You're right. There's indeed an extra allocation. But anyhow we still have to do copying when reconstructing the output for a ProjectSet.

risingwave/src/stream/src/executor/project_set.rs

Line 186 in 0b0cb85

if let Some(chunk) = builder.append_row(op, &*row) {

On the other hand, if we can have an owned version of ListRef (like holding an ArrayRef instead of &'a ArrayImpl) and directly let eval returning an iterator of ListRef, we can also achieve zero-copy.

wangrunji0408 added 10 commits November 13, 2023 12:18

common: refactor ListValue

d369a01

Signed-off-by: Runji Wang <[email protected]>

impl Clone & EstimateSize for ArrayBuilders

476c4a1

Signed-off-by: Runji Wang <[email protected]>

fix frontend, batch and stream

46db702

Signed-off-by: Runji Wang <[email protected]>

common: fix bugs

e2b725c

Signed-off-by: Runji Wang <[email protected]>

fix ListValue in connector

9491254

Signed-off-by: Runji Wang <[email protected]>

fix flatten

b91f422

Signed-off-by: Runji Wang <[email protected]>

connector: fix protobuf parser

0f714c3

Signed-off-by: Runji Wang <[email protected]>

fix expressions

d8eba1d

Signed-off-by: Runji Wang <[email protected]>

fix clippy

7683ba6

Signed-off-by: Runji Wang <[email protected]>

reduce size of BytesArray and StructArray to 64 bytes

f55b3a1

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 requested review from liurenjie1024, TennyZhuang, xiangjinwu and BugenZhao November 14, 2023 03:06

github-actions bot added the type/refactor label Nov 14, 2023

TennyZhuang reviewed Nov 14, 2023

View reviewed changes

src/common/src/array/arrow.rs Outdated Show resolved Hide resolved

src/common/src/array/list_array.rs Outdated Show resolved Hide resolved

stdrc self-requested a review November 15, 2023 03:04

BugenZhao reviewed Nov 15, 2023

View reviewed changes

wangrunji0408 added 2 commits November 15, 2023 16:50

Merge remote-tracking branch 'origin/main' into wrj/array-as-list

11c582d

fix comment

3a9e4e1

Signed-off-by: Runji Wang <[email protected]>

stdrc reviewed Nov 20, 2023

View reviewed changes

src/expr/impl/src/scalar/array_remove.rs Outdated Show resolved Hide resolved

wangrunji0408 added 2 commits November 28, 2023 16:50

Merge remote-tracking branch 'origin/main' into wrj/array-as-list

825939c

Signed-off-by: Runji Wang <[email protected]>

fix some/all

d844418

Signed-off-by: Runji Wang <[email protected]>

introduce ListValue::from_datum_iter

ba099e3

Signed-off-by: Runji Wang <[email protected]>

TennyZhuang approved these changes Nov 29, 2023

View reviewed changes

wangrunji0408 enabled auto-merge November 29, 2023 06:22

wangrunji0408 added 2 commits November 29, 2023 14:36

Merge remote-tracking branch 'origin/main' into wrj/array-as-list

5ace87f

fix some

55369df

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 added this pull request to the merge queue Nov 29, 2023

Merged via the queue into main with commit 13ba630 Nov 29, 2023
26 of 27 checks passed

wangrunji0408 deleted the wrj/array-as-list branch November 29, 2023 08:17

wangrunji0408 mentioned this pull request Dec 26, 2023

fix(expr): fix SOME/ALL/ANY expression #14221

Merged

9 tasks

xxchan reviewed Jul 21, 2024

View reviewed changes

xxchan mentioned this pull request Jul 23, 2024

perf: vectorized execution for ListArray functions #17784

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(type): refactor `ListValue` using array #13402

refactor(type): refactor `ListValue` using array #13402

wangrunji0408 commented Nov 14, 2023 •

edited

Loading

liurenjie1024 commented Nov 14, 2023

wangrunji0408 commented Nov 14, 2023 •

edited

Loading

liurenjie1024 commented Nov 14, 2023

TennyZhuang left a comment

BugenZhao commented Nov 15, 2023

wangrunji0408 commented Nov 15, 2023 •

edited

Loading

BugenZhao left a comment

BugenZhao Nov 15, 2023

wangrunji0408 Nov 15, 2023

BugenZhao Nov 15, 2023

wangrunji0408 Nov 15, 2023

BugenZhao commented Nov 15, 2023

wangrunji0408 commented Nov 15, 2023 •

edited

Loading

stdrc left a comment

codecov bot commented Nov 29, 2023 •

edited

Loading

xxchan Jul 21, 2024 •

edited

Loading

xxchan Jul 22, 2024 •

edited

Loading

BugenZhao Jul 22, 2024 •

edited

Loading

xxchan Jul 22, 2024

xxchan Jul 22, 2024

BugenZhao Jul 22, 2024

xxchan Jul 22, 2024

wangrunji0408 Jul 22, 2024 •

edited

Loading

BugenZhao Jul 22, 2024

	/// ```text
	/// select generate_series(1, x) from t(x);
	///
	/// # input chunk output chunks (TableFunction) output chunks (ProjectSet)
	/// 1 --------------> 0 1 -------------------------> 0 1
	/// 2 --------------> 1 1 -------------------------> 0 1
	/// 3 ----┐ --- ---
	/// │ 1 2 1 2
	/// └---------> 2 1 -------------------------> 0 1
	/// --- ---
	/// 2 2 1 2
	/// 2 3 2 3
	/// row idx--^ ^--values projected_row_id--^ ^--values
	/// ```

refactor(type): refactor ListValue using array #13402

refactor(type): refactor ListValue using array #13402

Conversation

wangrunji0408 commented Nov 14, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

liurenjie1024 commented Nov 14, 2023

wangrunji0408 commented Nov 14, 2023 • edited Loading

liurenjie1024 commented Nov 14, 2023

TennyZhuang left a comment

Choose a reason for hiding this comment

BugenZhao commented Nov 15, 2023

wangrunji0408 commented Nov 15, 2023 • edited Loading

BugenZhao 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

BugenZhao commented Nov 15, 2023

wangrunji0408 commented Nov 15, 2023 • edited Loading

stdrc left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 29, 2023 • edited Loading

Codecov Report

xxchan Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

xxchan Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

BugenZhao Jul 22, 2024 • edited Loading

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

wangrunji0408 Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor(type): refactor `ListValue` using array #13402

refactor(type): refactor `ListValue` using array #13402

wangrunji0408 commented Nov 14, 2023 •

edited

Loading

wangrunji0408 commented Nov 14, 2023 •

edited

Loading

wangrunji0408 commented Nov 15, 2023 •

edited

Loading

wangrunji0408 commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading

xxchan Jul 21, 2024 •

edited

Loading

xxchan Jul 22, 2024 •

edited

Loading

BugenZhao Jul 22, 2024 •

edited

Loading

wangrunji0408 Jul 22, 2024 •

edited

Loading