feat: binding struct values when they are parsed as row #2371
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2371 +/- ##
==========================================
+ Coverage 71.31% 71.35% +0.03%
==========================================
Files 688 688
Lines 88254 88392 +138
==========================================
+ Hits 62936 63069 +133
- Misses 25318 25323 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
…/risingwave into feat/insert_struct_data
…/risingwave into feat/insert_struct_data
| select * from s where s.v3 = (1,2,(1,2,3)); | ||
| logical_plan: | | ||
| LogicalProject { exprs: [$1, $2, $3], expr_alias: [v1, v2, v3] } | ||
| LogicalFilter { predicate: ($3 = {Some(Int32(1)), Some(Int32(2)), Some(Struct(StructValue { fields: [Some(Int32(1)), Some(Int32(2)), Some(Int32(3))] }))}:Struct { fields: [Int32, Int32, Struct { fields: [Int32, Int32, Int32] }] }) } |
There was a problem hiding this comment.
I wonder why the test format of StructValue remains the verbose form even after you impl fmt::Display in concise form?
There was a problem hiding this comment.
Maybe because the display of Datum is like these Some(...), so this looks like complex.
| @@ -322,6 +322,15 @@ for_all_scalar_variants! { scalar_impl_enum } | |||
| pub type Datum = Option<ScalarImpl>; | |||
| pub type DatumRef<'a> = Option<ScalarRefImpl<'a>>; | |||
|
|
|||
| pub fn get_data_type_from_datum(datum: &Datum) -> Result<DataType> { | |||
| match datum { | |||
| None => Err(internal_error( | |||
There was a problem hiding this comment.
I think the data type of a literal should be unknown until it's inferred by some means, think of such case:
postgres=# create type foo as (v1 int, v2 int); CREATE TYPE
postgres=# create table t (v1 foo);
CREATE TABLE
postgres=# SELECT * FROM t WHERE v1 > (1, null);
v1
----
(0 rows)
How can we determine the type of (1, null) until it's inferred from v1? (given the rule that v1 and (1, null) must be the same type, so the null's type must be int).
There was a problem hiding this comment.
I see.., will fix in follow-up issue.
| let data_type = match self { | ||
| ScalarImpl::Int16(_) => DataType::Int16, | ||
| ScalarImpl::Int32(_) => DataType::Int32, | ||
| ScalarImpl::Int64(_) => DataType::Int64, |
There was a problem hiding this comment.
IIRC timestamp with time zone is also stored as ScalarImpl::Int64 but has DataType::Timestampz.
There was a problem hiding this comment.
Thanks for mention, I will try to fix in follow pr.
There was a problem hiding this comment.
Just bringing it up. So that we are aware of this tricky issue in type system and can see which design is better. I do need see an immediate way to "fix" - in the current design, mapping from ScalarImpl to DataType is not unique; alternatively, we may add a chrono wrapper for timestampz.
There was a problem hiding this comment.
Let me see, I found that acutally I don't need this function, I refactor this part #2508.
What's changed and what's your intention?
PLEASE DO NOT LEAVE THIS EMPTY !!!
Checklist
Refer to a related PR or issue link (optional)
#581