Add StructArray Constructors (#3879) #4064
Conversation
tustvold
force-pushed
the
struct-array-constructors
branch
2 times, most recently
from
60086c7 to
ad76714
Compare
tustvold
force-pushed
the
struct-array-constructors
branch
from
ad76714 to
92e9f8a
Compare
| /// * `arrays[i].len() != arrays[j].len()` | ||
| /// * `arrays[i].len() != nulls.len()` | ||
| /// * `!fields[i].is_nullable() && !nulls.contains(arrays[i].nulls())` | ||
| pub fn try_new( |
There was a problem hiding this comment.
As in the similar PRs, I think we should add test coverage for these new constructors, especially the error conditions
There was a problem hiding this comment.
I verified that the error conditions are now covered (via should_panic messages). 👍
| @@ -359,37 +485,10 @@ impl std::fmt::Debug for StructArray { | |||
|
|
|||
| impl From<(Vec<(Field, ArrayRef)>, Buffer)> for StructArray { | |||
| fn from(pair: (Vec<(Field, ArrayRef)>, Buffer)) -> Self { | |||
| let capacity = pair.0.len(); | |||
There was a problem hiding this comment.
it is really nice that you are making the construction (and validation) of the various array types consistent ❤️
|
I've added tests, well re-used the existing tests 😄 |
tustvold
force-pushed
the
struct-array-constructors
branch
from
3b852da to
b66a8c7
Compare
| @@ -309,66 +309,18 @@ impl TryFrom<Vec<(&str, ArrayRef)>> for StructArray { | |||
| type Error = ArrowError; | |||
|
|
|||
| /// builds a StructArray from a vector of names and arrays. | |||
| /// This errors if the values have a different length. | |||
| /// An entry is set to Null when all values are null. | |||
There was a problem hiding this comment.
This doc comment isn't rendered in docs.rs so this somewhat peculiar behaviour was effectively not documented
| /// * `arrays[i].len() != arrays[j].len()` | ||
| /// * `arrays[i].len() != nulls.len()` | ||
| /// * `!fields[i].is_nullable() && !nulls.contains(arrays[i].nulls())` | ||
| pub fn try_new( |
There was a problem hiding this comment.
I verified that the error conditions are now covered (via should_panic messages). 👍
Which issue does this PR close?
Part of #3879
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
The TryFrom constructor no longer creates a null buffer if all children are null, this should not only be faster, but is also significantly less confusing - I had no idea it did this 😅