Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor datum from Option<ScalarImpl> to a struct #477

Open
BugenZhao opened this issue Feb 22, 2022 · 3 comments
Open

refactor datum from Option<ScalarImpl> to a struct #477

BugenZhao opened this issue Feb 22, 2022 · 3 comments
Labels
component/common Common components, such as array, data chunk, expression. difficulty/medium Issues that need some knowledge of the whole system good first issue Good for newcomers type/refactor

Comments

@BugenZhao
Copy link
Member

BugenZhao commented Feb 22, 2022

Currently, we use Option<ScalarImpl> in RisingWave to represent a nullable value. Using type alias for Datum indeed caused a lot of problems for us. For example, people would easily get confused when they see Option<Datum> or Option<Option<ScalarImpl>>, where the outer option means existence and the inner option means null.

We should refactor Datum to a struct, e.g. struct Datum(pub Option<ScalarImpl>).

Note this issue requires significant refactor work. Please contact maintainers before proceeding.

https://github.com/singularity-data/risingwave-dev/blob/7c6fb9b42846d9e0414d420e2e21df327352251a/rust/common/src/types/mod.rs#L276-L277

https://github.com/singularity-data/risingwave-dev/blob/7c6fb9b42846d9e0414d420e2e21df327352251a/rust/common/src/types/mod.rs#L303-L316

@fuyufjh
Copy link
Member

fuyufjh commented Mar 2, 2022

What's the benefit?

@BugenZhao
Copy link
Member Author

What's the benefit?

There're lots of stuff related to datum ser/de implemented as standalone functions, since we cannot implement methods on a type alias Option<ScalarImpl>.

image

image

https://github.com/singularity-data/risingwave-dev/blob/ed6330a950732d83a2b348e087b9f6933c8153b5/rust/common/src/types/mod.rs#L355-L366

@fuyufjh
Copy link
Member

fuyufjh commented Mar 2, 2022

However, as we discussed offline just now, this will cause plenty of changes.

BTW, functions like serialize_datum_into also looks okay to me :D

@fuyufjh fuyufjh added good first issue Good for newcomers and removed ramp up labels Mar 18, 2022
@skyzh skyzh added type/refactor component/common Common components, such as array, data chunk, expression. event/OSD labels Sep 2, 2022
@skyzh skyzh added difficulty/medium Issues that need some knowledge of the whole system and removed event/OSD labels Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/common Common components, such as array, data chunk, expression. difficulty/medium Issues that need some knowledge of the whole system good first issue Good for newcomers type/refactor
Projects
None yet
Development

No branches or pull requests

3 participants