Draft RFC for support nullable_number_type. #4158

wangzhen11aaa · 2022-02-15T07:43:28Z

Signed-off-by: wang.zhen wangzhenaaa7@gmail.com

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Draft RFC for support nullable_number_type.

Changelog

Bug Fix

Related Issues

#4079

Fixes #issue

Test Plan

Unit Tests

Stateless Tests

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

databend-bot · 2022-02-15T07:43:32Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

vercel · 2022-02-15T07:43:33Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/FvAHo2NUBKXEvqtaEnFAZjpFfEcR
✅ Preview: https://databend-git-fork-wangzhen11aaa-fsupportnullabl-9f5602-databend.vercel.app

[Deployment for 581e339 canceled]

wangzhen11aaa · 2022-02-15T07:45:16Z

@sundy-li I have wrote part of the process codes, is this the idea right?

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

sundy-li · 2022-02-15T08:19:48Z

common/datavalues2/src/columns/series.rs

+    /// Type promote to the nearest higher type.
+    /// UInt8 -> UInt16 -> UInt32 -> UInt64 -> bitmap<128>
+    /// Int8 -> Int16 -> Int32 -> Int64 -> bitmap<128>
+    pub(crate) fn _type_promotion(column: &ColumnRef, _datatype: TypeID) -> Option<TypeID> {


We already have construct_numeric_type && numeric_byte_size.

We already have construct_numeric_type && numeric_byte_size.
oops, duplicated ..
But, I have a question. IMHO,
numeric_byte_size is some method bound with concrete type, But this method returns a dyn trait. The number_byte_size method will be used via a type downcast, which I think is not a good solution. what?do you think？
@sundy-li

mergify · 2022-02-15T12:53:16Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

codecov-commenter · 2022-02-20T16:01:03Z

Codecov Report

Merging #4158 (581e339) into main (8d7e856) will decrease coverage by 0%.
The diff coverage is 16%.

@@          Coverage Diff           @@
##            main   #4158    +/-   ##
======================================
- Coverage     60%     60%    -1%     
======================================
  Files        804     804            
  Lines      40918   41062   +144     
======================================
+ Hits       24887   24908    +21     
- Misses     16031   16154   +123

Impacted Files	Coverage Δ
common/datavalues/src/columns/group_hash.rs	`36% <0%> (-24%)`	⬇️
common/datavalues/src/columns/series.rs	`67% <0%> (-8%)`	⬇️
common/datavalues/src/macros.rs	`68% <ø> (ø)`
...ommon/datavalues/src/types/deserializations/mod.rs	`0% <0%> (ø)`
.../datavalues/src/types/deserializations/nullable.rs	`0% <0%> (ø)`
...ry/src/pipelines/transforms/group_by/aggregator.rs	`83% <ø> (ø)`
...datablocks/src/kernels/data_block_group_by_hash.rs	`58% <25%> (-24%)`	⬇️
...mmon/datablocks/src/kernels/data_block_group_by.rs	`46% <50%> (-1%)`	⬇️
common/management/src/cluster/cluster_mgr.rs	`77% <0%> (-2%)`	⬇️
metasrv/src/network.rs	`97% <0%> (-2%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d7e856...581e339. Read the comment docs.

sundy-li · 2022-02-21T00:49:06Z

common/datablocks/src/kernels/data_block_group_by.rs

+            } else {
+                column.data_type()
+            };
+            if _typ.data_type_id().is_integer() {


Better to rename _typ

sundy-li · 2022-02-21T01:09:41Z

common/datablocks/src/kernels/data_block_group_by_hash.rs

+
+/// Init the nullable part's offset in bytes. It follows the values part.
+#[inline]
+fn init_nullable_offset_via_fields(


why not make this function return Result<usize>

why not make this function return Result<usize>

I see some codes use this kind of initialization as c style. Using Result makes codes more healthy, it is a better way.

sundy-li · 2022-02-21T01:15:52Z

common/datavalues/src/types/deserializations/mod.rs

@@ -36,6 +37,17 @@ pub trait TypeDeserializer: Send + Sync {
    fn de(&mut self, reader: &mut &[u8]) -> Result<()>;
    fn de_default(&mut self);
    fn de_batch(&mut self, reader: &[u8], step: usize, rows: usize) -> Result<()>;
+    fn de_batch_with_nullable(


It's only for nullable, so it is not must to put this into trait.

It's only for nullable, so it should not to put this into trait.
Yes, your idea is valuable.

sundy-li · 2022-02-21T01:16:29Z

common/datavalues/src/types/deserializations/nullable.rs

+            // todo, We should get the length of missed bytes which caused
+            // by self.inner.de method.
+            if reader[null_offset - 1] & 1 == 1 {
+                self.bitmap.push(false);


self.bitmap.push(reader[null_offset - 1] & 1 != 1)

self.bitmap.push(reader[null_offset - 1] & 1 != 1)

That's cool.

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

sundy-li · 2022-02-21T13:54:47Z

common/datablocks/src/kernels/data_block_group_by.rs

@@ -31,9 +32,17 @@ impl DataBlock {
        let mut group_key_len = 0;
        for col in column_names {
            let column = block.try_column_by_name(col)?;
-            let typ = column.data_type();
+            let typ = if column.is_nullable() {


use remove_nullable directly.

use remove_nullable directly.

remove_nullable is c~.

sundy-li · 2022-02-21T13:55:32Z

common/datablocks/src/kernels/data_block_group_by.rs

+                remove_nullable(&column.data_type())
+            } else {
+                column.data_type()
+            };
            if typ.data_type_id().is_integer() {


can support is_numeric.

sundy-li · 2022-02-21T13:58:38Z

common/datablocks/src/kernels/data_block_group_by_hash.rs

+}
+
+#[inline]
+fn check_group_columns_has_nullable(group_columns: &[&ColumnRef]) -> bool {


group_columns.any(|c| c.is_nullable())

sundy-li · 2022-02-21T13:59:59Z

common/datablocks/src/kernels/data_block_group_by_hash.rs

+
+/// Init the nullable part's offset in bytes. It follows the values part.
+#[inline]
+fn init_nullable_offset(group_keys: &[&ColumnRef]) -> Result<usize> {


group_keys.iter().map(|c| c.data_type().numeric_byte_size().unwrap()).sum()

sundy-li · 2022-02-21T14:02:37Z

common/datablocks/src/kernels/data_block_group_by_hash.rs

@@ -305,3 +427,43 @@ fn build(
    }
    Ok(())
 }
+
+#[inline]
+fn build_keys_with_nullable_column(


build_keys_with_nullable_column build merge into one method.

sundy-li · 2022-02-21T14:03:21Z

common/datablocks/src/kernels/data_block_group_by_hash.rs

        }
        Ok(group_keys)
    }
+
+    fn group_by_get_indices(


Remove group_by && group_by_get_indices

sundy-li · 2022-02-21T14:06:37Z

common/datablocks/src/kernels/data_block_group_by_hash.rs

+        if size == mem_size {
+            if col.is_nullable() {
+                let writer = unsafe { writer.add(*offsize) };
+                Series::fixed_hash_with_nullable(col, writer, step, *null_offset)?;


dowcast to Nullablecolumn and apply this method.

sundy-li · 2022-02-21T14:07:20Z

common/datavalues/src/columns/group_hash.rs

@@ -36,6 +59,27 @@ impl Series {
        })
    }

+    pub fn fixed_hash_with_nullable(


sundy-li · 2022-02-21T14:07:55Z

common/datavalues/src/columns/group_hash.rs

@@ -138,3 +193,30 @@ impl GroupHash for StringColumn {
        Ok(())
    }
 }
+
+impl GroupHash for NullableColumn {
+    fn fixed_hash_with_nullable(


sundy-li · 2022-02-21T14:09:47Z

common/datavalues/src/columns/group_hash.rs

+        column: &Arc<dyn Column>,
+        bitmap: &Bitmap,
+        step: usize,
+        null_offset: usize,


Merge into fixed_hash.

Option<(&Bitmap, usize)>

sundy-li · 2022-02-21T14:17:20Z

common/datavalues/src/columns/group_hash.rs

+        }
+        Ok(())
+    }
+
    pub fn fixed_hash(column: &ColumnRef, ptr: *mut u8, step: usize) -> Result<()> {


Add Option<&Bitmap, usize>

sundy-li · 2022-02-21T14:19:58Z

common/datavalues/src/types/deserializations/nullable.rs

@@ -46,6 +47,23 @@ impl TypeDeserializer for NullableDeserializer {
        Ok(())
    }

+    fn de_batch_with_nullable(


Donot put it into trait.

sundy-li · 2022-02-21T14:20:14Z

common/datavalues/src/types/deserializations/nullable.rs

+        for row in 0..rows {
+            let mut reader = &reader[step * row..];
+            self.inner.de(&mut reader)?;
+            self.bitmap.push(reader[*null_offset - type_size] & 1 != 1);


!=1 ---> = 0

sundy-li · 2022-03-05T23:42:36Z

Continue in #4340

Draft RFC

7da4751

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

databend-bot added the pr-bugfix this PR patches a bug in codebase label Feb 15, 2022

databend-bot added the need-review label Feb 15, 2022

vercel bot temporarily deployed to Preview February 15, 2022 07:43 Inactive

Fix bug in match arm

6b4c1a0

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 15, 2022 07:48 Inactive

wangzhen11aaa added 2 commits February 15, 2022 15:53

Get rid of warning

ad32db7

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

Get rid of warning

b6c3236

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 15, 2022 07:54 Inactive

sundy-li reviewed Feb 15, 2022

View reviewed changes

use datatype_bytes_size to promote type

45fa6ac

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 17, 2022 14:22 Inactive

wangzhen11aaa added 2 commits February 17, 2022 23:16

make series.rs untouched

e07f654

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

Merge branch 'master' into f_support_nullable_number_type_in_group_hash

ed82093

vercel bot deployed to Preview February 17, 2022 15:18 View deployment

The architecture of this task

6d4f70f

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 18, 2022 03:01 Inactive

fix logical bug in select the fix_hash_xx method

ccc7184

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 18, 2022 03:25 Inactive

cargo fmt

d5b7080

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 18, 2022 03:35 Inactive

wangzhen11aaa added 2 commits February 18, 2022 13:07

make group_hash.rs in datavalues mod untouched

c5c0c6f

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

hash works

3b63562

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 18, 2022 06:07 Inactive

add dereference to a reference variable

5b2a254

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 18, 2022 06:29 Inactive

make fmt calm down

952f912

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 20, 2022 14:23 Inactive

make clippy calm down

9f5971c

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 20, 2022 14:39 Inactive

make clippy calm down

5c86e89

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 20, 2022 14:48 Inactive

sundy-li reviewed Feb 21, 2022

View reviewed changes

wangzhen11aaa added 2 commits February 21, 2022 18:36

Merge branch 'master' into f_support_nullable_number_type_in_group_hash

bebe5be

Support group by more than one keys

581e339

Signed-off-by: wang.zhen <wangzhenaaa7@gmail.com>

vercel bot temporarily deployed to Preview February 21, 2022 12:07 Inactive

sundy-li reviewed Feb 21, 2022

View reviewed changes

sundy-li mentioned this pull request Mar 5, 2022

Support nullable group by #4340

Merged

sundy-li closed this Mar 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft RFC for support nullable_number_type. #4158

Draft RFC for support nullable_number_type. #4158

wangzhen11aaa commented Feb 15, 2022

databend-bot commented Feb 15, 2022

vercel bot commented Feb 15, 2022 •

edited

Loading

wangzhen11aaa commented Feb 15, 2022

sundy-li Feb 15, 2022

wangzhen11aaa Feb 15, 2022 •

edited

Loading

mergify bot commented Feb 15, 2022

codecov-commenter commented Feb 20, 2022 •

edited

Loading

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

wangzhen11aaa Feb 21, 2022 •

edited

Loading

sundy-li Feb 21, 2022

wangzhen11aaa Feb 21, 2022 •

edited

Loading

sundy-li Feb 21, 2022

wangzhen11aaa Feb 21, 2022

sundy-li Feb 21, 2022

wangzhen11aaa Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li Feb 21, 2022

sundy-li commented Mar 5, 2022

Draft RFC for support nullable_number_type. #4158

Draft RFC for support nullable_number_type. #4158

Conversation

wangzhen11aaa commented Feb 15, 2022

Summary

Changelog

Related Issues

Test Plan

databend-bot commented Feb 15, 2022

vercel bot commented Feb 15, 2022 • edited Loading

wangzhen11aaa commented Feb 15, 2022

Choose a reason for hiding this comment

wangzhen11aaa Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

mergify bot commented Feb 15, 2022

codecov-commenter commented Feb 20, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangzhen11aaa Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangzhen11aaa Feb 21, 2022 • 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

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

Choose a reason for hiding this comment

sundy-li commented Mar 5, 2022

vercel bot commented Feb 15, 2022 •

edited

Loading

wangzhen11aaa Feb 15, 2022 •

edited

Loading

codecov-commenter commented Feb 20, 2022 •

edited

Loading

wangzhen11aaa Feb 21, 2022 •

edited

Loading

wangzhen11aaa Feb 21, 2022 •

edited

Loading