feat: apply column-aware row encoding & enable schema change #8394

BugenZhao · 2023-03-07T09:37:40Z

Signed-off-by: Bugen Zhao i@bugenzhao.comI hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Enable column-aware row encoding for MaterializeExecutor and all StorageTable usages.

StateTable will decide which encoding format is used according to whether version field is set. Note that this is only set for regular tables.
Add a versioned field to TableDesc. StorageTable will similarly decide the format by this.
Enable ALTER TABLE [ADD|DROP] COLUMN in production.

Checklist For Contributors

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).
I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

SQL commands, functions, and operators
- ALTER TABLE [ADD|DROP] COLUMN

Release note

Initial support for ALTER TABLE [ADD|DROP] COLUMN on regular tables (without connectors).

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

…ncoding-for-table Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao · 2023-03-07T10:37:06Z

src/stream/src/executor/mview/materialize.rs

+        // TODO: If we do some `Delete` after schema change, we cannot ensure the encoded value
+        // with the new version of serializer is the same as the old one, even if they can be
+        // decoded into the same value. The table is now performing consistency check on the raw
+        // bytes, so we need to turn off the check here. We may turn it on if we can compare the
+        // decoded row.
+        let state_table =
+            StateTableInner::from_table_catalog_inconsistent_op(table_catalog, store, vnodes).await;


cc @hzxa21 😰

cc. @hzxa21 Sanity check is disabled here

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

src/common/src/util/value_encoding/column_aware_row_encoding.rs

y-wei · 2023-03-07T14:57:50Z

Do existing e2e/unit tests cover schema change cases?

BugenZhao · 2023-03-08T03:38:06Z

Do existing e2e/unit tests cover schema change cases?

Oops, I forgot. I'll add some.

…ncoding-for-table

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

codecov · 2023-03-08T07:56:44Z

Codecov Report

Merging #8394 (5924af6) into main (e6f2561) will decrease coverage by 0.02%.
The diff coverage is 56.63%.

@@            Coverage Diff             @@
##             main    #8394      +/-   ##
==========================================
- Coverage   71.62%   71.61%   -0.02%     
==========================================
  Files        1135     1135              
  Lines      185458   185541      +83     
==========================================
+ Hits       132835   132875      +40     
- Misses      52623    52666      +43

Flag	Coverage Δ
rust	`71.61% <56.63%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
...batch/src/executor/join/distributed_lookup_join.rs	`0.00% <0.00%> (ø)`
src/batch/src/executor/row_seq_scan.rs	`21.80% <0.00%> (-0.21%)`	⬇️
src/common/src/row/compacted_row.rs	`64.28% <0.00%> (-35.72%)`	⬇️
...c/util/value_encoding/column_aware_row_encoding.rs	`78.02% <0.00%> (-3.29%)`	⬇️
src/ctl/src/cmd_impl/table/scan.rs	`0.00% <0.00%> (ø)`
src/java_binding/src/iterator.rs	`0.00% <0.00%> (ø)`
src/meta/src/manager/streaming_job.rs	`20.40% <0.00%> (-1.10%)`	⬇️
src/stream/src/from_proto/batch_query.rs	`0.00% <0.00%> (ø)`
src/stream/src/from_proto/chain.rs	`0.00% <0.00%> (ø)`
src/stream/src/from_proto/lookup.rs	`0.00% <0.00%> (ø)`
... and 18 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

BugenZhao · 2023-03-08T08:30:03Z

Do existing e2e/unit tests cover schema change cases?

Done. PTAL 🥺

st1page · 2023-03-08T09:12:42Z

src/stream/src/executor/mview/materialize.rs

    }

    pub async fn handlle_conflict<'a, S: StateStore>(
        &mut self,
        buffer: MaterializeBuffer,
-        table: &StateTable<S>,
+        table: &StateTableInner<S, SD>,


can we extract a trait for the state table such as StateTableRead and change the type to impl StateTableRead to erase the S and SD here 🤔

Might be a good idea. We can do this refactor in the future.

src/stream/src/common/table/state_table.rs

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

fuyufjh

Good job!

fuyufjh · 2023-03-08T09:43:04Z

src/common/src/util/value_encoding/mod.rs

 pub mod column_aware_row_encoding;

 pub type Result<T> = std::result::Result<T, ValueEncodingError>;

+/// The kind of all possible `ValueRowSerde`.


If we run fast enough, perhaps we could remove the Basic encoding in 0.18. (Because the storage's side work has break the compatibility anyway. Of course just for this time 🥵)

I'm thinking of still using the basic value-encoding for the tables that (temporarily) not supports schema change, like internal tables or normal materialized views (except for Index). The reason is that "column ID" does not make much sense for these tables with immutable schema, and we can save more storage costs.

fuyufjh · 2023-03-08T09:44:25Z

proto/plan_common.proto

@@ -41,6 +41,9 @@ message StorageTableDesc {
  uint32 retention_seconds = 5;
  repeated uint32 value_indices = 6;
  uint32 read_prefix_len_hint = 7;
+  // Whether the table is versioned. If `true`, column-aware row encoding will be used
+  // to be compatible with schema changes.
+  bool versioned = 8;


Is it possible that a table contains both basic encoded and column-aware encoded rows?

If we don't consider backward compatibility, then nope. Currently, we follow the version field of TableCatalog to decide the encoding.

y-wei

I don't have any concerns, but I may be better to have another senior's approval 😀

fuyufjh

LGTM

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao · 2023-03-09T06:09:49Z

src/java_binding/src/iterator.rs

+        // Decide which serializer to use based on whether the table is versioned or not.
+        let row_serde = if versioned {
+            ColumnAwareSerde::new(&column_ids, schema.into()).into()
+        } else {
+            BasicSerde::new(&column_ids, schema.into()).into()
+        };


I just found that there's something else we need to update in the storage java binding. cc @wenym1 Is it possible to reuse some code of StorageTable here?

BugenZhao added 3 commits March 7, 2023 17:21

version field, either serde, integrate with materialize

c0ccdcf

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

add generic to materialize

24d256f

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

turn off sanity check

4599da5

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

github-actions bot added the type/feature label Mar 7, 2023

BugenZhao added 2 commits March 7, 2023 18:03

add fake generic

245b567

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

disable schema change for table with connector

7f56be9

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao added the breaking-change label Mar 7, 2023

BugenZhao added 2 commits March 7, 2023 18:30

fill default id

b355c8c

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

remove notice and enable in production

8f94036

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao changed the title ~~feat: apply column-aware row encoding for table~~ feat: apply column-aware row encoding for table & enable schema change Mar 7, 2023

BugenZhao changed the title ~~feat: apply column-aware row encoding for table & enable schema change~~ feat: apply column-aware row encoding & enable schema change Mar 7, 2023

BugenZhao added the user-facing-changes Contains changes that are visible to users label Mar 7, 2023

Merge remote-tracking branch 'origin/main' into bz/column-aware-row-e…

6b788b8

…ncoding-for-table Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao requested review from wcy-fdu, y-wei and st1page March 7, 2023 10:36

BugenZhao commented Mar 7, 2023

View reviewed changes

BugenZhao added 2 commits March 7, 2023 19:18

enable table version for table with connector

966d290

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

fix encoding

7fcfeb3

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao commented Mar 7, 2023

View reviewed changes

src/common/src/util/value_encoding/column_aware_row_encoding.rs Show resolved Hide resolved

y-wei mentioned this pull request Mar 7, 2023

test: harder unit tests for ColumnAwareSerde #8408

Merged

6 tasks

BugenZhao added 2 commits March 8, 2023 14:49

Merge remote-tracking branch 'origin/main' into bz/column-aware-row-e…

0ba116e

…ncoding-for-table

extend e2e tests

82a4133

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao requested a review from chenzl25 March 8, 2023 08:29

st1page reviewed Mar 8, 2023

View reviewed changes

refine comments

cae0e81

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

fuyufjh reviewed Mar 8, 2023

View reviewed changes

y-wei approved these changes Mar 8, 2023

View reviewed changes

fuyufjh approved these changes Mar 9, 2023

View reviewed changes

github-actions bot removed the user-facing-changes Contains changes that are visible to users label Mar 9, 2023

BugenZhao added this pull request to the merge queue Mar 9, 2023

github-actions bot added the user-facing-changes Contains changes that are visible to users label Mar 9, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 9, 2023

support column-aware serde

5924af6

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao commented Mar 9, 2023

View reviewed changes

BugenZhao requested a review from wenym1 March 9, 2023 06:10

BugenZhao added this pull request to the merge queue Mar 9, 2023

Merged via the queue into main with commit 630685f Mar 9, 2023

BugenZhao deleted the bz/column-aware-row-encoding-for-table branch March 9, 2023 08:32

BugenZhao mentioned this pull request Mar 9, 2023

Integrate column-aware row encoding for tables. #7906

Closed

BugenZhao mentioned this pull request Apr 11, 2023

fix(streaming): only disable sanity check for schema changable table #9095

Merged

7 tasks

CharlieSYH added 📖✓ Covered or will be covered in the user docs. 📖✗ No user documentation is needed. and removed 📖✓ Covered or will be covered in the user docs. labels Apr 17, 2023

CharlieSYH added 📖✓ Covered or will be covered in the user docs. and removed 📖✗ No user documentation is needed. labels May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: apply column-aware row encoding & enable schema change #8394

feat: apply column-aware row encoding & enable schema change #8394

BugenZhao commented Mar 7, 2023 •

edited

Loading

BugenZhao Mar 7, 2023

fuyufjh Mar 8, 2023

y-wei commented Mar 7, 2023

BugenZhao commented Mar 8, 2023

codecov bot commented Mar 8, 2023 •

edited

Loading

BugenZhao commented Mar 8, 2023

st1page Mar 8, 2023

BugenZhao Mar 8, 2023

fuyufjh left a comment

fuyufjh Mar 8, 2023

BugenZhao Mar 8, 2023

fuyufjh Mar 8, 2023

BugenZhao Mar 8, 2023

y-wei left a comment

fuyufjh left a comment

BugenZhao Mar 9, 2023

feat: apply column-aware row encoding & enable schema change #8394

feat: apply column-aware row encoding & enable schema change #8394

Conversation

BugenZhao commented Mar 7, 2023 • edited Loading

What's changed and what's your intention?

Checklist For Contributors

Checklist For Reviewers

Documentation

Types of user-facing changes

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-wei commented Mar 7, 2023

BugenZhao commented Mar 8, 2023

codecov bot commented Mar 8, 2023 • edited Loading

Codecov Report

BugenZhao commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuyufjh 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

y-wei left a comment

Choose a reason for hiding this comment

fuyufjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao commented Mar 7, 2023 •

edited

Loading

codecov bot commented Mar 8, 2023 •

edited

Loading