refactor(types): remove `type_name` and `field_descs` from `ColumnDesc` #20497

BugenZhao · 2025-02-14T06:38:34Z

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

What's changed and what's your intention?

The follow-up to #20496. ColumnDesc is simply Field with some extra information.

field_descs will be generated on the fly based on the StructType when it's used in DESCRIBE. Resolve #17128.

Checklist

I have written necessary rustdoc comments.
I have added necessary unit tests and integration tests.
I have added test labels as necessary.
I have added fuzzing tests or opened an issue to track them.
My PR contains breaking changes.
My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

My PR needs documentation updates.

Release note

BugenZhao · 2025-02-14T06:39:01Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

BugenZhao · 2025-02-18T07:02:54Z

src/connector/codec/src/decoder/protobuf/parser.rs

+        // TODO(struct): since we deprecated `field_descs` in `ColumnDesc`, there's no need to
+        // traverse the fields here except for maintaining the same column id (index) as the
+        // original implementation.
+        let _field_descs = if let DataType::List { .. } = field_type {


As a result, there's no information for the "column" id of the nested fields, which seems to make resolving #19755 even more challenging. I may reintroduce it in future PRs, but in a clearer manner.

Just to confirm, does it mean that we will only be able to alter subfields for newly created structs? 🤔

Not that true as if we really want this to be available on existing tables, we can introduce a "mini" multi-versioned schema for the STRUCT type when we are going to upgrade its storage representation for compatibility purposes.

xxchan · 2025-02-19T05:59:29Z

src/common/src/catalog/column.rs

@@ -128,27 +126,14 @@ pub struct ColumnDesc {

 impl ColumnDesc {
    pub fn unnamed(column_id: ColumnId, data_type: DataType) -> ColumnDesc {


Maybe we could rename it to unnamed_for_test

Or just for_test

I was confused when we have unnamed column, and found it's just in tests.

Counting on @xiangjinwu! 😁

xxchan · 2025-02-19T06:05:02Z

src/common/src/catalog/column.rs

+    /// Should only be used for observability/debugging purposes.
    pub fn flatten(&self) -> Vec<ColumnDesc> {


Should only be used for observability/debugging purposes.

I think for these we should always name it like xxx_for_debug to avoid misuse. Because developers don't always read docs. 🤣

Currently, there is only one caller in the handler for DESCRIBE table. I might update it in the future to stop returning ColumnDesc, which would prevent potential misuse.

update it in the future to stop returning ColumnDesc, which would prevent potential misuse.

Agree. Or it can be moved as part of describe's implementation, rather than a reusable method on common::catalog::column::ColumnDesc.

Makes sense. Done in https://github.com/risingwavelabs/risingwave/pull/20497/files/4ef67561ef31dfceba8a9d9a98c6f5547c6de6e2..a8a61d6da97aadcfbe3f9ec1ed590af01954e7a1

xxchan

Generally LGTM

xiangjinwu · 2025-02-19T10:11:24Z

src/common/src/catalog/column.rs

+    /// Should only be used for observability/debugging purposes.
    pub fn flatten(&self) -> Vec<ColumnDesc> {


update it in the future to stop returning ColumnDesc, which would prevent potential misuse.

Agree. Or it can be moved as part of describe's implementation, rather than a reusable method on common::catalog::column::ColumnDesc.

xiangjinwu · 2025-02-19T10:12:21Z

src/common/src/catalog/column.rs

    pub fn new_struct(
        name: &str,
        column_id: i32,
-        type_name: &str,
+        _type_name: &str,
        fields: Vec<ColumnDesc>,
    ) -> Self {
+        // TODO(struct): assign type name to the struct once supported
        let data_type =
            StructType::new(fields.iter().map(|f| (&f.name, f.data_type.clone()))).into();
-        Self {
-            data_type,
-            column_id: ColumnId::new(column_id),
-            name: name.to_owned(),
-            field_descs: fields,
-            type_name: type_name.to_owned(),
-            generated_or_default_column: None,
-            description: None,
-            additional_column: AdditionalColumn { column_type: None },
-            version: ColumnDescVersion::LATEST,
-            system_column: None,
-        }
+
+        Self::named(name, ColumnId::new(column_id), data_type)


I will deprecate this in favor of named as well.

xiangjinwu · 2025-02-19T10:13:10Z

src/common/src/catalog/column.rs

-            additional_column: c.additional_column.clone().into(),
-            version: c.version as i32,
-        }
+        c.to_protobuf()


Did not notice such duplication 🤯

xiangjinwu · 2025-02-20T06:05:37Z

src/connector/codec/src/decoder/avro/schema.rs

+            // TODO(struct): since we deprecated `field_descs` in `ColumnDesc`, there's no need to
+            // traverse the fields here except for maintaining the same column id (index) as the
+            // original implementation.


This is the immediate motivation that I restarted #19538 after putting #13104 on hold.

Actually, because we always reassign the ColumnId in the final step of creating a source or table, there's no need to allocate a column ID here (but simply specifying ColumnId::placeholder()), unless we need the uniqueness property of ColumnId before that step.

graphite-app · 2025-02-20T09:44:46Z

Merge activity

Feb 20, 4:44 AM EST: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.

Signed-off-by: Bugen Zhao <[email protected]>

github-actions bot added the type/refactor label Feb 14, 2025

This was referenced Feb 14, 2025

refactor(types): assign name f{} to unnamed fields of struct (record) #20490

Merged

refactor(types): remove type_name and sub_fields from Field #20496

Merged

BugenZhao force-pushed the bz/struct-part-2 branch from 18b4236 to 27c218b Compare February 14, 2025 06:57

BugenZhao force-pushed the bz/struct-part-3 branch from 0f64462 to feb234b Compare February 14, 2025 06:57

BugenZhao mentioned this pull request Feb 14, 2025

refactor(common): (WIP/Preview) optional name for struct data type #19538

Draft

9 tasks

BugenZhao force-pushed the bz/struct-part-2 branch from 27c218b to 0165303 Compare February 17, 2025 07:11

BugenZhao force-pushed the bz/struct-part-3 branch from a2384d4 to 71e92b9 Compare February 17, 2025 07:11

BugenZhao force-pushed the bz/struct-part-2 branch from 0165303 to 6d40b4b Compare February 17, 2025 07:30

BugenZhao force-pushed the bz/struct-part-3 branch from 71e92b9 to a95541c Compare February 17, 2025 07:30

BugenZhao force-pushed the bz/struct-part-2 branch from 6d40b4b to 81b85ca Compare February 17, 2025 10:13

BugenZhao force-pushed the bz/struct-part-3 branch 2 times, most recently from 961e7ed to 10e177a Compare February 18, 2025 06:49

BugenZhao marked this pull request as ready for review February 18, 2025 06:55

BugenZhao commented Feb 18, 2025

View reviewed changes

BugenZhao requested review from xiangjinwu and xxchan February 18, 2025 07:03

xxchan reviewed Feb 19, 2025

View reviewed changes

xxchan approved these changes Feb 19, 2025

View reviewed changes

BugenZhao force-pushed the bz/struct-part-2 branch from 145a308 to 144ede2 Compare February 19, 2025 09:01

BugenZhao force-pushed the bz/struct-part-3 branch from 10e177a to 9fabe05 Compare February 19, 2025 09:01

xiangjinwu approved these changes Feb 20, 2025

View reviewed changes

BugenZhao force-pushed the bz/struct-part-2 branch from 144ede2 to 46a2007 Compare February 20, 2025 07:46

BugenZhao force-pushed the bz/struct-part-3 branch from 9fabe05 to a8a61d6 Compare February 20, 2025 07:47

Base automatically changed from bz/struct-part-2 to main February 20, 2025 09:42

BugenZhao force-pushed the bz/struct-part-3 branch from 285bc59 to 08fa368 Compare February 20, 2025 09:44

refactor(types): remove type_name and field_descs from ColumnDesc

74824bd

BugenZhao added 9 commits February 20, 2025 17:45

cleanup source column desc

33f3430

Signed-off-by: Bugen Zhao <[email protected]>

deprecate pb column desc

ac3f632

Signed-off-by: Bugen Zhao <[email protected]>

remove deprecated fields

fde3826

Signed-off-by: Bugen Zhao <[email protected]>

update tests

2265633

Signed-off-by: Bugen Zhao <[email protected]>

inline flatten for show columns only

f0c8937

Signed-off-by: Bugen Zhao <[email protected]>

add tests

36a7b28

Signed-off-by: Bugen Zhao <[email protected]>

change list to struct[]

ecd9dc5

Signed-off-by: Bugen Zhao <[email protected]>

only show array element if it's struct

4ac4626

Signed-off-by: Bugen Zhao <[email protected]>

fix describe e2e test

d2bddd4

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao force-pushed the bz/struct-part-3 branch from 08fa368 to d2bddd4 Compare February 20, 2025 09:45

BugenZhao added this pull request to the merge queue Feb 20, 2025

Merged via the queue into main with commit c632835 Feb 20, 2025
31 checks passed

BugenZhao deleted the bz/struct-part-3 branch February 20, 2025 10:33

xiangjinwu mentioned this pull request Feb 21, 2025

refactor(source): simplify Avro field to SQL column mapping #20559

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(types): remove `type_name` and `field_descs` from `ColumnDesc` #20497

refactor(types): remove `type_name` and `field_descs` from `ColumnDesc` #20497

BugenZhao commented Feb 14, 2025 •

edited

Loading

BugenZhao commented Feb 14, 2025 •

edited

Loading

BugenZhao Feb 18, 2025

xxchan Feb 19, 2025

BugenZhao Feb 20, 2025 •

edited

Loading

xxchan Feb 19, 2025

xxchan Feb 19, 2025

xxchan Feb 19, 2025

BugenZhao Feb 20, 2025

xxchan Feb 19, 2025

BugenZhao Feb 19, 2025

xiangjinwu Feb 19, 2025

BugenZhao Feb 20, 2025 •

edited

Loading

xxchan left a comment

xiangjinwu Feb 19, 2025

xiangjinwu Feb 19, 2025

xiangjinwu Feb 19, 2025

xiangjinwu Feb 20, 2025

BugenZhao Feb 20, 2025

graphite-app bot commented Feb 20, 2025

		@@ -128,27 +126,14 @@ pub struct ColumnDesc {

		impl ColumnDesc {
		pub fn unnamed(column_id: ColumnId, data_type: DataType) -> ColumnDesc {

		/// Should only be used for observability/debugging purposes.
		pub fn flatten(&self) -> Vec<ColumnDesc> {

refactor(types): remove type_name and field_descs from ColumnDesc #20497

refactor(types): remove type_name and field_descs from ColumnDesc #20497

Conversation

BugenZhao commented Feb 14, 2025 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

BugenZhao commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao Feb 20, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

xxchan 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

graphite-app bot commented Feb 20, 2025

Merge activity

refactor(types): remove `type_name` and `field_descs` from `ColumnDesc` #20497

refactor(types): remove `type_name` and `field_descs` from `ColumnDesc` #20497

BugenZhao commented Feb 14, 2025 •

edited

Loading

BugenZhao commented Feb 14, 2025 •

edited

Loading

BugenZhao Feb 20, 2025 •

edited

Loading

BugenZhao Feb 20, 2025 •

edited

Loading