feat(types): add Fields trait and derive macro #13862

TennyZhuang · 2023-12-07T17:08:47Z

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

What's changed and what's your intention?

risingwave/src/common/src/types/fields.rs

Lines 38 to 76 in 585f637

    
           #[derive(Fields)] 
        
           struct Data { 
        
               v1: i16, 
        
               v2: std::primitive::i32, 
        
               v3: bool, 
        
               v4: f32, 
        
               v5: F32, 
        
               v6: Option<f64>, 
        
               v7: Vec<u8>, 
        
               v8: std::vec::Vec<i16>, 
        
               v9: Option<Vec<i64>>, 
        
               v10: std::option::Option<Vec<Option<f32>>>, 
        
               v11: Box<Timestamp>, 
        
               v14: Sub, 
        
           } 
        
           assert_eq!( 
        
               Data::fields(), 
        
               vec![ 
        
                   ("v1", DataType::Int16), 
        
                   ("v2", DataType::Int32), 
        
                   ("v3", DataType::Boolean), 
        
                   ("v4", DataType::Float32), 
        
                   ("v5", DataType::Float32), 
        
                   ("v6", DataType::Float64), 
        
                   ("v7", DataType::Bytea), 
        
                   ("v8", DataType::List(Box::new(DataType::Int16))), 
        
                   ("v9", DataType::List(Box::new(DataType::Int64))), 
        
                   ("v10", DataType::List(Box::new(DataType::Float32))), 
        
                   ("v11", DataType::Timestamp), 
        
                   ( 
        
                       "v14", 
        
                       DataType::Struct(StructType::new(vec![ 
        
                           ("v12", DataType::Timestamptz), 
        
                           ("v13", DataType::Bytea) 
        
                       ])) 
        
                   ) 
        
               ] 
        
           )

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
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)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: TennyZhuang <[email protected]>

github-actions

license-eye has totally checked 4559 files.

Valid	Invalid	Ignored	Fixed
2006	1	2552	0

Click to see the invalid file list

src/common/src/types/with_data_type.rs

src/common/src/types/with_data_type.rs

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang · 2023-12-07T17:20:02Z

src/common/fields-derive/src/lib.rs

+use syn::Data;
+use syn::DeriveInput;
+use syn::Field;
+use syn::Result;


I'm unsure why cargo fmt can't help me to merge them here, but I don't like to fix it manually. I guess there are many similar fmt issues in the whole codebase. Let's investigate why cargo fmt doesn't work and fix them all later.

cargo fmt works on my machine 🤡

Maybe you are not in the root dir?

brew uninstall rustfmt

xxchan

Others LGTM

xxchan · 2023-12-08T01:08:10Z

Cargo.toml

@@ -7,6 +7,7 @@ members = [
  "src/cmd_all",
  "src/common",
  "src/common/common_service",
+  "src/common/fields-derive",


prefer underscore, to be consistent with others

It’s already inconsistent now, although most of them are underscores, there are still several crates using dash.

xxchan · 2023-12-08T01:14:06Z

src/common/src/types/fields.rs

+        }
+
+        assert_eq!(
+            Data::fields(),


Just a hint. You can use expect_test::Expect::assert_debug_eq so you don’t have to manually write the RHS 🤪

xxchan · 2023-12-08T01:22:11Z

src/common/src/types/with_data_type.rs

+    Timestamp, Timestamptz, F32, F64,
+};
+
+pub trait WithDataType {


This is a little hard to understand why it’s needed. Add some comments?

Do we have something similar before? Or can we use this trait to simplify some existing manual matches?

I can’t find similar thing that can be used, but I guess we may use it in somewhere. I’ll review it later.

Currently it’s just a macro helper.

src/common/src/types/fields.rs

BugenZhao

Rest LGTM

BugenZhao · 2023-12-08T03:59:47Z

src/common/src/types/with_data_type.rs

+impl_with_data_type!(Timestamp, DataType::Timestamp);
+impl_with_data_type!(Timestamptz, DataType::Timestamptz);
+impl_with_data_type!(Interval, DataType::Interval);
+impl_with_data_type!(Vec<u8>, DataType::Bytea);


Is this okay?

Suggested change

impl_with_data_type!(Vec<u8>, DataType::Bytea);

impl_with_data_type!([u8], DataType::Bytea);

We may need both lines?

BugenZhao · 2023-12-08T04:03:38Z

src/common/src/types/with_data_type.rs

+    Timestamp, Timestamptz, F32, F64,
+};
+
+pub trait WithDataType {


Do we have something similar before? Or can we use this trait to simplify some existing manual matches?

BugenZhao · 2023-12-08T04:05:22Z

src/common/src/types/fields.rs

+/// A struct can implements `Fields` when if can be represented as a relational Row.
+pub trait Fields {
+    /// When the struct being converted to an [`Row`](crate::row::Row) or a [`DataChunk`](crate::array::DataChunk), it schema must be consistent with the `fields` call.
+    fn fields() -> Vec<(&'static str, DataType)>;


Should we return Cow here?

+1 for making the return value static. Maybe use lazy-static?

Got it, but it increases the complexity. We can do it when it’s really used in critical path.

We can use an attribute static to control whether make it lazy static.

Please note that static doesn't mean always more efficient.
If user only called once, and need the ownership, then we need to allocate once, return a static reference, and then allocate (clone) again.

And we can modify the signature to this:

fn fields() -> impl ToOwned<Owned = Vec<(&'static str, DataType)>> + AsRef<[(&'static str, DataType)]>;

wangrunji0408

Rest LGTM!

wangrunji0408 · 2023-12-08T04:53:55Z

src/common/fields-derive/src/lib.rs

+                ty,
+                ..
+            } = field_rs;
+            let name = name.map_or("".to_string(), |name| name.to_string());


Why a struct field may have no name? 🤔

In PostgreSQL the names would implicitly become f1, f2, ...

test=# select row(9, 18); row -------- (9,18) (1 row) test=# select (row(9, 18)).f1; f1 ---- 9 (1 row) test=# select to_jsonb(row(9, 18)); to_jsonb --------------------- {"f1": 9, "f2": 18} (1 row)

But in RisingWave this has not been supported consistently yet. Some places are empty while some places have these implicit names. Have not thought about the impact on backward compatibility if it was fixed to be pg-compatible.

No opinion on what the behavior of this place should be.

wangrunji0408 · 2023-12-08T04:56:27Z

src/common/src/types/fields.rs

+/// A struct can implements `Fields` when if can be represented as a relational Row.
+pub trait Fields {
+    /// When the struct being converted to an [`Row`](crate::row::Row) or a [`DataChunk`](crate::array::DataChunk), it schema must be consistent with the `fields` call.
+    fn fields() -> Vec<(&'static str, DataType)>;


+1 for making the return value static. Maybe use lazy-static?

Signed-off-by: TennyZhuang <[email protected]>

codecov · 2023-12-11T09:26:28Z

Codecov Report

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

Comparison is base (ddfa85b) 68.30% compared to head (ae981e4) 68.25%.
Report is 29 commits behind head on main.

Files	Patch %	Lines
src/common/src/types/with_data_type.rs	41.66%	21 Missing ⚠️
src/common/fields-derive/src/lib.rs	87.50%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13862      +/-   ##
==========================================
- Coverage   68.30%   68.25%   -0.06%     
==========================================
  Files        1528     1531       +3     
  Lines      262603   262735     +132     
==========================================
- Hits       179375   179320      -55     
- Misses      83228    83415     +187

Flag	Coverage Δ
rust	`68.25% <79.54%> (-0.06%)`	⬇️

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.

feat(types): add Fields trait and derive macro

585f637

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang requested review from wangrunji0408 and BugenZhao December 7, 2023 17:08

TennyZhuang requested a review from a team as a code owner December 7, 2023 17:08

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

github-actions bot reviewed Dec 7, 2023

View reviewed changes

src/common/src/types/with_data_type.rs Show resolved Hide resolved

TennyZhuang and others added 3 commits December 8, 2023 01:10

Update src/common/src/types/with_data_type.rs

0adfd13

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

clean code

d7f867e

Signed-off-by: TennyZhuang <[email protected]>

make clippy happy

6ca88be

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang commented Dec 7, 2023

View reviewed changes

xxchan reviewed Dec 8, 2023

View reviewed changes

src/common/src/types/fields.rs Outdated Show resolved Hide resolved

BugenZhao reviewed Dec 8, 2023

View reviewed changes

wangrunji0408 approved these changes Dec 8, 2023

View reviewed changes

TennyZhuang added 3 commits December 8, 2023 20:47

fmt

9d01e99

Signed-off-by: TennyZhuang <[email protected]>

add comment

74276cc

Signed-off-by: TennyZhuang <[email protected]>

pass lint

245155e

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang enabled auto-merge December 11, 2023 08:16

TennyZhuang added 2 commits December 11, 2023 16:42

fix macro

a45d09c

Signed-off-by: TennyZhuang <[email protected]>

add some comments

ae981e4

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang added this pull request to the merge queue Dec 11, 2023

Merged via the queue into main with commit 92f3b58 Dec 11, 2023
7 of 8 checks passed

TennyZhuang deleted the feat/with-data-type branch December 11, 2023 10:01

TennyZhuang mentioned this pull request Dec 11, 2023

fix(types): fix Serial in Fields derive and add tests #13926

Merged

9 tasks

lmatz mentioned this pull request Dec 12, 2023

14 nexmark benchmark skus performance degradation #13931

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(types): add Fields trait and derive macro #13862

feat(types): add Fields trait and derive macro #13862

TennyZhuang commented Dec 7, 2023 •

edited

Loading

github-actions bot left a comment

TennyZhuang Dec 7, 2023 •

edited

Loading

xxchan Dec 8, 2023

xxchan Dec 8, 2023

TennyZhuang Dec 8, 2023

xxchan left a comment

xxchan Dec 8, 2023

TennyZhuang Dec 8, 2023

xxchan Dec 8, 2023

xxchan Dec 8, 2023

BugenZhao Dec 8, 2023

TennyZhuang Dec 8, 2023

BugenZhao left a comment

BugenZhao Dec 8, 2023

TennyZhuang Dec 8, 2023

BugenZhao Dec 8, 2023

BugenZhao Dec 8, 2023

wangrunji0408 Dec 8, 2023

TennyZhuang Dec 8, 2023

TennyZhuang Dec 8, 2023

wangrunji0408 left a comment

wangrunji0408 Dec 8, 2023

xxchan Dec 8, 2023

xiangjinwu Dec 8, 2023

wangrunji0408 Dec 8, 2023

codecov bot commented Dec 11, 2023 •

edited

Loading

	#[derive(Fields)]
	struct Data {
	v1: i16,
	v2: std::primitive::i32,
	v3: bool,
	v4: f32,
	v5: F32,
	v6: Option<f64>,
	v7: Vec<u8>,
	v8: std::vec::Vec<i16>,
	v9: Option<Vec<i64>>,
	v10: std::option::Option<Vec<Option<f32>>>,
	v11: Box<Timestamp>,
	v14: Sub,
	}

	assert_eq!(
	Data::fields(),
	vec![
	("v1", DataType::Int16),
	("v2", DataType::Int32),
	("v3", DataType::Boolean),
	("v4", DataType::Float32),
	("v5", DataType::Float32),
	("v6", DataType::Float64),
	("v7", DataType::Bytea),
	("v8", DataType::List(Box::new(DataType::Int16))),
	("v9", DataType::List(Box::new(DataType::Int64))),
	("v10", DataType::List(Box::new(DataType::Float32))),
	("v11", DataType::Timestamp),
	(
	"v14",
	DataType::Struct(StructType::new(vec![
	("v12", DataType::Timestamptz),
	("v13", DataType::Bytea)
	]))
	)
	]
	)

	impl_with_data_type!(Vec<u8>, DataType::Bytea);
	impl_with_data_type!([u8], DataType::Bytea);

feat(types): add Fields trait and derive macro #13862

feat(types): add Fields trait and derive macro #13862

Conversation

TennyZhuang commented Dec 7, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

github-actions bot left a comment

Choose a reason for hiding this comment

TennyZhuang Dec 7, 2023 • 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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangrunji0408 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

codecov bot commented Dec 11, 2023 • edited Loading

Codecov Report

TennyZhuang commented Dec 7, 2023 •

edited

Loading

TennyZhuang Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 11, 2023 •

edited

Loading