Fix FOR bug, also fix bench to compile #341

a10y · 2024-06-03T17:05:32Z

@robert3005

Fix issue with FoR compression where nullability was getting stripped, causing compress benchmark to fail
Fix some random tokio bs that was also causing compress benchmark to fail due to recursive runtime creations

vortex-fastlanes/src/for/compress.rs

bench-vortex/src/reader.rs

bench-vortex/benches/random_access.rs

vortex-alp/src/compute.rs

vortex-array/src/array/datetime/localdatetime.rs

vortex-array/src/compress.rs

vortex-datetime-parts/src/compute.rs

a10y · 2024-06-05T17:08:05Z

vortex-fastlanes/src/for/compute.rs

@@ -86,4 +94,31 @@ mod test {
        assert_eq!(scalar_at(&forarr, 1).unwrap(), 15.into());
        assert_eq!(scalar_at(&forarr, 2).unwrap(), 19.into());
    }
+
+    #[test]
+    fn for_as_contiguous() {


i guess we should probably centralize 1-2 smoketests for each of the ArrayCompute functions for all of the variants.

vortex-datetime-parts/src/compute.rs

a10y · 2024-06-05T17:11:53Z

Ok, I think I fixed all of the errors in the benchmarks now.

Implemented AsContiguousFn for all of the types that get touched by the taxi dataset. I added an impl_default_as_contiguous! macro so that types can easily opt themselves in to implementing AsContiguous in terms of flattens. I wanted to do a blanket implementation but you can't override blanket implementations :(
Fixed some bugs in how StructArray as_contiguous was implemented
Fixed some bugs in how
Fix how medicare data was being written

Now the benchmarks complete without error which is nice. I can FLUP with a github action to run the benchmarks in a scheduled action to get faster feedback if things break

bench-vortex/src/public_bi_data.rs

vortex-array/src/compute/as_contiguous.rs

vortex-alp/src/compute.rs

vortex-array/src/array/struct/compute.rs

vortex-datetime-parts/src/compute.rs

gatesn · 2024-06-06T13:50:22Z

Discussed offline about removing as_contiguous and reaffirming how flatten should work

a10y · 2024-06-10T19:36:57Z

I believe this PR now addresses all of the stylistic comments.

For the AsContiguous removal: I broke that out into a separate PR stacked on this branch: #346

Would love a review there of the structure, and maybe a discussion about how best to handle List and Extension dtyped-arrays

Per the comments on #341 , this PR removes the AsContiguousFn trait and as_contiguous function entirely, and instead pushes the relevant details into the implementation of `ArrayFlatten::flatten` for the `ChunkedArray` type. A few questions I've bumped into while doing this refactor: * Can DType::List only reference primitive types? I see ListScalar, but it’s not clear to me how Vortex would encode e.g. `List<Struct>` or `List<List>` * **Answer**: Deferring for now as List DTypes not fully support yet anyway * How should this work for ExtensionArray? * **Answer**: ExtensionArray can contain a ChunkedArray for its internal storage Array

@robert3005

@robert3005 1. Fix issue with FoR compression where nullability was getting stripped, causing `compress` benchmark to fail 2. Fix some random tokio bs that was also causing `compress` benchmark to fail due to recursive runtime creations

fix compress benchmark

ea36852

a10y requested a review from robert3005 June 3, 2024 17:05

a10y commented Jun 3, 2024

View reviewed changes

vortex-fastlanes/src/for/compress.rs Show resolved Hide resolved

a10y commented Jun 3, 2024

View reviewed changes

bench-vortex/src/reader.rs Show resolved Hide resolved

robert3005 approved these changes Jun 3, 2024

View reviewed changes

a10y added 3 commits June 5, 2024 17:34

save

2651669

lord give me the strength

516bf27

change back

75b8781