Support Semi-structured variant data type #4348
Conversation
|
Thanks for the contribution! Please review the labels and make any necessary changes. |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/databend/databend/92h8fFjQ7FjufYvb7JTnufDbbMPT |
| @@ -150,6 +151,9 @@ pub trait Column: Send + Sync { | |||
| pub trait IntoColumn { | |||
| fn into_column(self) -> ColumnRef; | |||
| fn into_nullable_column(self) -> ColumnRef; | |||
|
|
|||
| fn into_column_with_field(self, f: &ArrowField) -> ColumnRef; | |||
There was a problem hiding this comment.
So into_column & into_nullable_column can be removed because they are covered by new methods?
There was a problem hiding this comment.
into_column & into_nullable_column need to be kept, because in some cases we don't have a field
| let size = column.len(); | ||
| let mut builder = ColumnBuilder::<JsonValue>::with_capacity(size); | ||
|
|
||
| with_match_physical_primitive_type!(from_type.data_type_id().to_physical_type(), |$T| { |
There was a problem hiding this comment.
StringColumn cast to variant column is not covered.
There was a problem hiding this comment.
String can't be automatically casted to Variants, it must be explicitly cast using the to_variant function
| Some(data_type) => { | ||
| match data_type.data_type_id() { | ||
| // arrow type has no nullable type | ||
| Nullable => unimplemented!(), |
There was a problem hiding this comment.
Maybe Err(ErrorCode::LogicaError) is better ?
There was a problem hiding this comment.
I think unimplemented is ok, this is not a logical error, nullable is defined by ourselves, arrow doesn't have a corresponding type.
what is your opinion? @sundy-li
There was a problem hiding this comment.
unimplemented!() will panic, please use the error instead.
There was a problem hiding this comment.
We can use ErrorCode::UnImplement instead. There are other places to be improved...
There was a problem hiding this comment.
ok, we can modify it in other issues, as the implementation of nullable is not related to varint
b41sh
force-pushed
the
variant
branch
2 times, most recently
from
edb1c38
to
4e86354
Compare
I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/
Summary
Support Semi-structured variant data type
Changelog
Related Issues
#3916
Test Plan
Unit Tests
Stateless Tests