Support StructArray in Row Format (#3159) #3212
Conversation
There was a problem hiding this comment.
I do not fully understand how this encoding will avoid conflicts with other fields potentially as the length of an encoded Struct can be variable length as well
Otherwise I had some other comments, but I don't think any would prevent this PR from being merged
| @@ -307,6 +308,13 @@ mod variable; | |||
| /// | |||
| /// Input Row Format | |||
| /// ``` | |||
| /// | |||
| /// ## Struct Encoding | |||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| enum Codec { |
There was a problem hiding this comment.
extracting this into Codec` is a nice generalization
| Ok(Self::Struct(converter, owned)) | ||
| } | ||
| _ => Err(ArrowError::NotYetImplemented(format!( | ||
| "not yet implemented: {:?}", |
There was a problem hiding this comment.
| "not yet implemented: {:?}", | |
| "Row format support not yet implemented for: {:?}", |
| .iter() | ||
| .zip(lengths.iter_mut()) | ||
| .for_each(|(slice, length)| *length += variable::encoded_len(slice)), | ||
| DataType::Utf8 => as_string_array(array) |
There was a problem hiding this comment.
Seeing this list of types I think it is going to conflict with #3182
There was a problem hiding this comment.
Oh yes, most definitely 😁
| } | ||
| Codec::Struct(converter, null) => { | ||
| let v = as_struct_array(array); | ||
| let rows = converter.convert_columns(v.columns())?; |
There was a problem hiding this comment.
Is it true that this code converts the struct array to row format now and then the individual rows are copied over one by one into the resulting format?
In other words, the row format form of the struct array is copied?
There was a problem hiding this comment.
Yup - we just flatten the schema, so a Struct{Int32, Int32}, Struct{Float32} is encoded similarly to Int32,Int32,Float32 albeit with additional nullability for the structs
| }; | ||
| let end_offset = *offset + 1 + row.as_ref().len(); | ||
| out.buffer[*offset] = sentinel; | ||
| out.buffer[*offset + 1..end_offset].copy_from_slice(row.as_ref()); |
There was a problem hiding this comment.
Since the row format of Struct arrays can be variable length, how are we ensuring that the contents of one struct array aren't accidentally compared with the contents of another field (e.g. the COBs encoding approach for variable length strings).
Also I wonder why not encode the struct array directly into out rather than copy it from another Rows?
There was a problem hiding this comment.
aren't accidentally compared with the contents of another field
The same way as normal, effectively this approach flattens the schema in a depth first manner. Lists will need encoding in this way, that's still to come 😄
Also I wonder why not encode the struct array directly into out rather than copy it from another Rows?
Nulls, we need to not encode the values if the StructArray contains a null. Otherwise you would potentially establish an ordering between nulls, based on the "masked" values.
There was a problem hiding this comment.
I'm working on adding a diagram to show this more clearly
There was a problem hiding this comment.
Nulls, we need to not encode the values if the StructArray contains a null. Otherwise you would potentially establish an ordering between nulls, based on the "masked" values.
This explanation makes sense -- I suggest you put it as a comment somewhere as it was not obvious to me while reading this PR.
| @@ -1329,6 +1502,54 @@ mod tests { | |||
| assert_eq!(&cols[0], &a); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_struct() { | |||
There was a problem hiding this comment.
Would it be valuable to more than one level (aka a struct array with a struct array child)? Also, how about a struct array with another type?
|
Benchmark runs are scheduled for baseline = b2bfe9c and contender = 733d32e. 733d32e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #3159
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?