Validate ArrayData type when converting to Array (#2834) #2835
Conversation
|
Interestingly this is failing on timestamp casts, which is very fishy 😅 |
tustvold
force-pushed
the
validate-array-data-type
branch
from
20f74ee
to
102f86f
Compare
arrow/src/compute/kernels/cast.rs
Outdated
| // note: (numeric_cast + SIMD multiply) is faster than (cast & multiply) | ||
| let c: Int64Array = numeric_cast(&time_array); | ||
| let from_size = time_unit_multiple(from_unit); | ||
| let to_size = time_unit_multiple(to_unit); | ||
| // from is only smaller than to if 64milli/64second don't exist | ||
| let mult = Int64Array::from(vec![to_size / from_size; array.len()]); | ||
| let converted = multiply(&c, &mult)?; | ||
| let converted = multiply_scalar(&c, to_size / from_size)?; |
github-actions
bot
added
arrow
| assert_eq!( | ||
| data.data_type(), | ||
| &DataType::Boolean, | ||
| "BooleanArray expected ArrayData with type {} got {}", |
| let array: ArrayRef = match T::get_physical_type() { | ||
| PhysicalType::BOOLEAN => Arc::new(BooleanArray::from(array_data)), | ||
| PhysicalType::INT32 => match array_data.data_type() { | ||
| ArrowType::UInt32 => Arc::new(UInt32Array::from(array_data)), |
There was a problem hiding this comment.
is this a bug fix? Seems like a good improvement to me
There was a problem hiding this comment.
Sort of, it doesn't actually matter as the PrimitiveArray is discarded by the parent StructArrayReader, which is only concerned with the ArrayData portion
There was a problem hiding this comment.
I guess I was wondering "what is the rationale for this change"
There was a problem hiding this comment.
Oh, because now it would panic complaining that it was creating an Int32Array from DataType::UInt32 ArrayData.
|
Parquet failure does not appear to relate to this PR, will fix separately |
|
Benchmark runs are scheduled for baseline = 1397fb4 and contender = 8adebca. 8adebca 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?
Closes #2834
Rationale for this change
Arrays can assume that the provided ArrayData is valid, however, what that means depends on the data type of the ArrayData. We weren't always validating that the ArrayData had the expected data type, which could lead to unsoundness.
What changes are included in this PR?
Are there any user-facing changes?