Support list in the fuzzer (only for implemented actions) #1735
Conversation
joseph-isaacs
force-pushed
the
ji/list-arb-slice-check
branch
from
ab596a4 to
2cf4f98
Compare
# Conflicts: # fuzz/src/slice.rs # fuzz/src/take.rs
joseph-isaacs
changed the title
vortex-dtype/src/arbitrary.rs
Outdated
| const BASE_TYPE_COUNT: i32 = 4; | ||
| const CONTAINER_TYPE_COUNT: i32 = 2; | ||
| let max_dtype_kind = if depth == 0 { | ||
| BASE_TYPE_COUNT | ||
| } else { | ||
| CONTAINER_TYPE_COUNT + BASE_TYPE_COUNT | ||
| }; | ||
| Ok(match u.int_in_range(0..=(max_dtype_kind - 1))? { | ||
| 0 => DType::Bool(u.arbitrary()?), | ||
| 1 => DType::Primitive(u.arbitrary()?, u.arbitrary()?), | ||
| 2 => DType::Utf8(u.arbitrary()?), | ||
| 3 => DType::Binary(u.arbitrary()?), | ||
| 4 => DType::Struct(random_struct_dtype(u, depth - 1)?, u.arbitrary()?), | ||
| 5 => DType::List(Arc::new(random_dtype(u, depth - 1)?), u.arbitrary()?), |
There was a problem hiding this comment.
Would it be simpler if we kept struct as the last number? The logic became very complicated
There was a problem hiding this comment.
I can fold in the numbers, I preferred the explicitness. But the order wouldnt make anything simpler.
There was a problem hiding this comment.
I see, list is like struct. I was confused by seeing 4 since that includes Struct but then we do a range for the type - 1. I think it would be simpler to change BASE_TYPE_COUNT to 3 and remove -1 from the int_in_range
There was a problem hiding this comment.
I can do that, ill rename BASE_TYPE_COUNT to base type offset
There was a problem hiding this comment.
I moved the range to count from 1 to n (instead of 0).
vortex-array/src/children.rs
Outdated
| @@ -25,6 +30,18 @@ impl NamedChildrenCollector { | |||
| } | |||
| } | |||
|
|
|||
| impl NamedTreeCollector { | |||
There was a problem hiding this comment.
we never use the names from this collector. We just collect all the children
There was a problem hiding this comment.
Not now, but since its is off the critical path I thought it was best to implement this collector
There was a problem hiding this comment.
I'm just confused how it's related to this pr. I know we want it for some display and transformations but it's a bit out of place in this pr
There was a problem hiding this comment.
Its gone, I needed to get the encoding of all arrays and I didn't see the other iterator
|
The regular collector is only the children, not the whole tree. I could instead create a collector of all arrays |
|
You have |
fuzz/src/lib.rs
Outdated
| ArrayChildrenIterator::new(array.to_array()) | ||
| .into_iter() |
There was a problem hiding this comment.
this can be just array.depth_first_traversal()
This allows the fuzzer to support array where only some of the compute functions are implemented