[Variant] remove BorrowedShreddingState - #9791
Conversation
|
I also removed |
scovich
left a comment
scovich
left a comment
There was a problem hiding this comment.
Maybe I don't understand the change, but it seems like it introduces a lot of cloning of concrete array types? Not sure why that's needed when the [Borrowed]ShreddingState is only concerned about ArrayRef vs. &ArrayRef?
| mod variant_get; | ||
| mod variant_to_arrow; | ||
|
|
||
| pub use variant_array::{BorrowedShreddingState, ShreddingState, VariantArray, VariantType}; |
There was a problem hiding this comment.
aside: Was this already an unused import? I wonder why clippy didn't flag it when it became unused?
There was a problem hiding this comment.
pub use makes it visible to all outside crates and clippy can't know if someone else uses it.
There was a problem hiding this comment.
so this is technically a breaking API change
| Self::$enum_variant(UnshredPrimitiveRowBuilder::new( | ||
| value, | ||
| typed_value.$cast_fn(), | ||
| typed_value.$cast_fn().clone(), |
There was a problem hiding this comment.
Not sure I understand -- why would a cast return a borrowed value that needs cloning?
| Self::Decimal32(DecimalUnshredRowBuilder::new(value, typed_value, *s as _)) | ||
| Self::Decimal32(DecimalUnshredRowBuilder::new( | ||
| value, | ||
| typed_value.as_primitive().clone(), |
There was a problem hiding this comment.
How expensive is it to clone a PrimitiveArray? It's not directly an Arc?
There was a problem hiding this comment.
We're cloning a &PrimitiveArray<T>
pub struct PrimitiveArray<T: ArrowPrimitiveType> {
data_type: DataType, // 24 bytes
/// Values data
values: ScalarBuffer<T::Native>, // 24 bytes
nulls: Option<NullBuffer>, // 48 bytes if Some
}| DataType::List(_) => Self::List(ListUnshredVariantBuilder::try_new( | ||
| value, | ||
| typed_value.as_list(), | ||
| typed_value.as_list::<i32>().clone(), |
There was a problem hiding this comment.
Similar question for these -- cost of cloning struct and list arrays?
| struct UnshredPrimitiveRowBuilder<'a, T> { | ||
| value: Option<&'a ArrayRef>, | ||
| typed_value: &'a T, | ||
| struct UnshredPrimitiveRowBuilder<T> { | ||
| value: Option<ArrayRef>, | ||
| typed_value: T, |
There was a problem hiding this comment.
Changes like this one seem unrelated to shared shredding state that needed an Option<&ArrayRef>? Even if value needs to be cloned, we could still keep a borrowed reference to typed_value which is anyway a bare type?
There was a problem hiding this comment.
Can't keep the reference without using lifetimes in builders. It seems like removing them was a bad idea. I'll switch it back.
| /// Creates a new UnshredVariantRowBuilder from the `(value, typed_value)` pair of a shredded | ||
| /// variant struct. Returns None for the None/None case - caller decides how to handle based on | ||
| /// context. | ||
| fn try_new_opt(inner_struct: &'a StructArray) -> Result<Option<Self>> { |
There was a problem hiding this comment.
This is the place where BorrowedShreddingState was so useful.
For nested builders (List/Struct) we used to recursively call try_new_opt(field_array.try_into()?): the try_into produced a BorrowedShreddingState<'a> whose ArrayRefs were borrowed from the source field_array. The wrapper got consumed, but the &'a ArrayRefs inside it carried 'a along and could be stored in the builder.
In ShreddingState the ArrayRefs are owned, so once we call the function - the ShreddingState goes out of scope and the references point to free memory.
A workaround is using &StructArray since it's a reference to the outer Array data.
There was a problem hiding this comment.
Hmm. I wonder if we're going about this wrong.
First question: What problem are we actually trying to solve by eliminating BorrowedShreddingState? Is it just annoying to have two similar types? Or something else more serious?
Second question: What if (thought experiment) we standardized on BorrowedShreddingState everywhere instead?
- Only use
ShreddingStateas an internal helper member ofVariantArray,ShreddedVariantFieldArray, etc? (its job is to centralize the name-based lookup and validation code; we should probably pushinnerinside as well, since that's always there) VariantArray::shredding_state()then returnsself.shredding_state.borrow()(BorrowedShreddingState<'_>return type)- All functions that currently expect
ShreddingStatechange to expectBorrowedShreddingStateinstead (I think this is already the case)
Third question: Where are we actually cloning StructArray, PrimitiveArray, etc today? Does the proposed change improve the situation, make it worse, or leave it unchanged? For example, VariantArray and ShreddedVariantFieldArray constructors both clone their input struct array today, and I don't think the current PR changes that.
Fourth question: Now that we know VariantArray is only a temporary helper that cannot actually impl Array, should we revisit the decision to make it an owned type? If VariantArray maintained references internally instead of owned values, then we could just use borrowed types everywhere and be done with it. Would the benefits be worth the headaches it causes code that uses VariantArray?
There was a problem hiding this comment.
- Nothing serious. I thought the
BorrowedShreddingStateredundant and wanted to remove it as dead code. - We tried this in Support Shredded Lists/Array in
variant_get#8354. There we follow thevariant_getpath and extract the shredding state. For Struct arrays we can useBorrowedShreddingState, but for List arrays, we write a new array usingtakekernel. We can't useBorrowedShreddingStatehere as reference outlives the value. PrimitiveArrayis only cloned inShreddingStateandunshred_variantbuilder.StructArrayis cloned in:VariantArray::try_new,ShreddedVariantFieldArray::try_newandcanonicalize_shredded_types. We make the situation a little worse forShreddingStateby introducing one extraArrayRefArc bump per present field. Same forPrimitiveArrays for unshred builders.- I think a good alternative is Arc'ing the inner
StructArray.
There was a problem hiding this comment.
@scovich Upon further reflection, the only performance diff, that this PR introduces is Arc bumps per Some(ArrayRef) ShreddingState depending on how it's shredded(perfectly - 1 or not -2) per variant_get path step.
The perf difference should be very minimal.
(I can still try to Cow ShreddingState to keep the Arc bumps to a minimum on the paths where we can use lifetimes)
There was a problem hiding this comment.
No arc bumps on the variant_get Field path 😪
There was a problem hiding this comment.
Sorry I was too slow -- I agree with your assessment (from five hours ago) that 1-2 arc bumps per path step should be acceptable. But it looks like you already implemented the Cow version (two hours ago)?
There was a problem hiding this comment.
No worries, I was just playing around with the idea. We can always revert the commit and proceed with the previous version. I too think that it's not worth the added complexity.
…-borrowedshreddingstate
scovich
left a comment
scovich
left a comment
There was a problem hiding this comment.
Looking at the latest Cow change, I'm not convinced the extra complexity is worth the tiny performance benefit it might bring (avoiding 1-2 arc clones per path step). The lifetimes are just too messy (the unreachable! line in into_borrowed_fields for owned shredding state is a big red flag, and a panic footgun).
| let value = if let Some(value_col) = inner_struct.column_by_name("value") { | ||
| validate_binary_array(value_col.as_ref(), "value")?; | ||
| Some(value_col) | ||
| Some(Cow::Borrowed(value_col)) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
aside: isn't this just a transpose?
let value = inner_struct
.column_by_name("value")
.map(|value| {
validate_binary_array(...)?;
Ok(Cow::Borrowed(value))
}
.transpose()?; (tho it's not shorter, so meh)
| // Cache the (value, typed_value) lookup. try_from validates the value column type. | ||
| let shredding_state = ShreddingState::try_from(inner_struct)?.into_static(); | ||
|
|
||
| // Note this clone is cheap, it just bumps the ref count |
There was a problem hiding this comment.
aside: inner_struct is a StructArray, not an ArrayRef, so clone has to round trip through ArrayData, no?
| /// The `typed_value` column the step descended through. Returned so the caller can union | ||
| /// its nulls into the accumulated null buffer without needing to re-borrow from the | ||
| /// (now-consumed) input state. | ||
| consumed_typed_value: Cow<'a, ArrayRef>, |
There was a problem hiding this comment.
We can't just return a reference to the null buffer because of lifetime issues, I guess?
There was a problem hiding this comment.
We'd clone otherwise, which is just an Arc Bump
| return Ok(None); | ||
| }; | ||
| let nested_struct = field.as_struct_opt().ok_or_else(|| { | ||
| ArrowError::InvalidArgumentError(format!( |
There was a problem hiding this comment.
This would mean the Array doesn't follow the shredding spec? Because the field should be a struct with value/typed_value children?
| // `typed_value_arc` is the owned input and is about to move into the returned | ||
| // `consumed_typed_value`. We cannot also borrow into it (self-referential), so we | ||
| // clone `nested`'s columns to produce a `ShreddingState<'static>` (coerces to 'a). |
There was a problem hiding this comment.
I don't understand how this could work? 'a is the lifetime of the root, and some cloned Arc will have a "local" lifetime instead. How can that owned value coerce to 'a lifetime? Or is 'a actually something shorter-lived than the root?
There was a problem hiding this comment.
The ShreddingState::new returns a ShreddingState<'static> and it auto coerces into a <'a>. The values we pass in it are owned so it satisfies the <'a>.
| /// Creates a new UnshredVariantRowBuilder from the `(value, typed_value)` pair of a shredded | ||
| /// variant struct. Returns None for the None/None case - caller decides how to handle based on | ||
| /// context. | ||
| fn try_new_opt(inner_struct: &'a StructArray) -> Result<Option<Self>> { |
There was a problem hiding this comment.
Sorry I was too slow -- I agree with your assessment (from five hours ago) that 1-2 arc bumps per path step should be acceptable. But it looks like you already implemented the Cow version (two hours ago)?
sdf-jkl
left a comment
sdf-jkl
left a comment
There was a problem hiding this comment.
I'll revert the commit
| /// The `typed_value` column the step descended through. Returned so the caller can union | ||
| /// its nulls into the accumulated null buffer without needing to re-borrow from the | ||
| /// (now-consumed) input state. | ||
| consumed_typed_value: Cow<'a, ArrayRef>, |
There was a problem hiding this comment.
We'd clone otherwise, which is just an Arc Bump
| return Ok(None); | ||
| }; | ||
| let nested_struct = field.as_struct_opt().ok_or_else(|| { | ||
| ArrowError::InvalidArgumentError(format!( |
| // `typed_value_arc` is the owned input and is about to move into the returned | ||
| // `consumed_typed_value`. We cannot also borrow into it (self-referential), so we | ||
| // clone `nested`'s columns to produce a `ShreddingState<'static>` (coerces to 'a). |
There was a problem hiding this comment.
The ShreddingState::new returns a ShreddingState<'static> and it auto coerces into a <'a>. The values we pass in it are owned so it satisfies the <'a>.
This reverts commit 8aadf40.
|
@scovich I've reverted the commit. |
scovich
left a comment
scovich
left a comment
There was a problem hiding this comment.
Looks like a great simplification, now that cloning shredding state is just a 2x Arc bump instead of having to make an ArrayData round tripl
| mod variant_get; | ||
| mod variant_to_arrow; | ||
|
|
||
| pub use variant_array::{BorrowedShreddingState, ShreddingState, VariantArray, VariantType}; |
|
BTW do we have any benchmarks that could sanity check our belief that this change does not hurt performance? |
Not yet. We could make some for |
|
run benchmarks variant_builder variant_kernels variant_validation |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing remove-borrowedshreddingstate (cde17de) to fd1c5b3 (merge-base) diff File an issue against this benchmark runner |
|
The high level idea seems reasonable to me -- I kicked off the benchmarks that we do have -- let's see if they tell us anything |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing remove-borrowedshreddingstate (cde17de) to fd1c5b3 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing remove-borrowedshreddingstate (cde17de) to fd1c5b3 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
| mod variant_get; | ||
| mod variant_to_arrow; | ||
|
|
||
| pub use variant_array::{BorrowedShreddingState, ShreddingState, VariantArray, VariantType}; |
There was a problem hiding this comment.
so this is technically a breaking API change
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes apache#9790. # Rationale for this change Check issue <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> # What changes are included in this PR? - Drop `BorrowedShreddingState` - Replace it with `ShreddingState` - ~~Removed the lifetimes in `unshred_variant` as they required helpers to cover recursive `ShreddingState` handling.~~ - ~~Lifetimes removal introduces clone on `NullBuffer`. Extra 3 usize (24 bytes) per `Array`. Only used in `NullUnshredVariantBuilder`~~ Removed the only place where `NullBuffer` was stored. No regression. <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> # Are these changes tested? Yes, unit tests. <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> # Are there any user-facing changes? No. <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
4 participants
Which issue does this PR close?
BorrowedShreddingState#9790.Rationale for this change
Check issue
What changes are included in this PR?
BorrowedShreddingStateShreddingStateRemoved the lifetimes inunshred_variantas they required helpers to cover recursiveShreddingStatehandling.Lifetimes removal introduces clone onRemoved the only place whereNullBuffer. Extra 3 usize (24 bytes) perArray. Only used inNullUnshredVariantBuilderNullBufferwas stored. No regression.Are these changes tested?
Yes, unit tests.
Are there any user-facing changes?
No.