feat: impl canonicalize_into for SparseArray #2420

a10y · 2025-02-19T03:50:19Z

Timer precision: 41 ns
canonical             fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ canonicalize_into                │               │               │               │         │
│  ├─ 0.001           8.207 µs      │ 553 µs        │ 11.77 µs      │ 12.59 µs      │ 1000    │ 1000
│  ├─ 0.01            12.04 µs      │ 121.5 µs      │ 13.2 µs       │ 13.42 µs      │ 1000    │ 1000
│  ├─ 0.05            15.99 µs      │ 38.08 µs      │ 18.41 µs      │ 18.43 µs      │ 1000    │ 1000
│  ╰─ 0.1             20.41 µs      │ 140.4 µs      │ 23.29 µs      │ 23.35 µs      │ 1000    │ 1000
╰─ into_canonical                   │               │               │               │         │
   ├─ 0.001           8.874 µs      │ 154.9 µs      │ 12.24 µs      │ 12.88 µs      │ 1000    │ 1000
   ├─ 0.01            11.91 µs      │ 129.2 µs      │ 13.12 µs      │ 13.38 µs      │ 1000    │ 1000
   ├─ 0.05            13.74 µs      │ 117.8 µs      │ 18.24 µs      │ 17.31 µs      │ 1000    │ 1000
   ╰─ 0.1             17.08 µs      │ 118.9 µs      │ 20.22 µs      │ 20.3 µs       │ 1000    │ 1000

gatesn · 2025-02-19T09:19:01Z

encodings/sparse/src/canonical.rs

+                    canonicalize_primitive_into(&array, builder)?;
+                });
+            }
+            _ => unreachable!("unsupported SparseArray dtype {}", array.dtype()),


We should probably error here instead of panic, just because there's no compile-time safety that Sparse arrays don't one day start supporting other DTypes. I don't see why they couldn't, e.g. support ExtDType?

Could even fall back to builder.extend(array.into_canonical())

encodings/sparse/src/canonical/builder.rs

gatesn · 2025-02-19T09:20:48Z

encodings/sparse/src/canonical/builder.rs

+        .fill_scalar()
+        .as_primitive()
+        .typed_value()
+        .vortex_expect("fill value");


This is optional when the fill value is null. I don't think you can expect this?

you are correct 🤦

gatesn · 2025-02-19T09:21:31Z

encodings/sparse/src/canonical/builder.rs

+        .fill(MaybeUninit::new(fill_value));
+    // SAFETY: we just filled the buffer with the fill value.
+    unsafe {
+        builder.values.set_len(sparse.len());


Style nit, but putting the semi-colon after the } keeps it on one line.

gatesn · 2025-02-19T09:21:55Z

encodings/sparse/src/canonical/builder.rs

+        .vortex_expect("fill value");
+
+    builder
+        .values


It's odd to me that this is public?

Yes it is a bit odd, I think Dan exposed it as part of his bitpacking canonicalize_into. I can change PrimitiveBuilder to expose a spare_capacity_mut instead

gatesn · 2025-02-19T09:22:36Z

encodings/sparse/src/canonical/builder.rs

+        }
+    });
+
+    // Set the validity from the sparse array.


Seems a shame after specifically patching values, to then canonicalize the validity into a BooleanBuffer when there may only be a couple of nulls.

0ax1

Primarily had a peek at the benchmarks. Looks good, some tiny style suggestions.

0ax1 · 2025-02-19T09:15:40Z

encodings/sparse/Cargo.toml

@@ -28,5 +28,11 @@ vortex-mask = { workspace = true }
 vortex-scalar = { workspace = true }

 [dev-dependencies]
+divan = { workspace = true }


0ax1 · 2025-02-19T09:22:51Z

encodings/sparse/benches/canonical.rs

+use vortex_scalar::Scalar;
+use vortex_sparse::SparseArray;
+
+fn generate_sparse_array(len: usize, sparsity: f64) -> SparseArray {


tiny style nits:

could consider going ordering functions by call order (been told by @gatesn that's how we do it :)
=> main first, then the benchmarks, then the helper. I primarily care about the benchmarks being above the helpers

factoring out shared bench parametrization into const BENCH_ARGS, e.g. see encodings/fsst/benches/fsst_compress.rs

Not really crucial, more like to get the benchmarks into similar shape and easily human parseable due to consistency.

0ax1 · 2025-02-19T09:25:15Z

encodings/sparse/benches/canonical.rs

+fn into_canonical(bencher: Bencher, sparsity: f64) {
+    bencher
+        .with_inputs(|| generate_sparse_array(64_000, sparsity))
+        .bench_local_values(|sparse_array| sparse_array.into_canonical().unwrap())


Curious on your take about bench_local_values vs bench_values.

It was sort of an arbitrary choice, I think it's probably fine to parallelize so can switch to bench_values

joseph-isaacs · 2025-02-19T09:44:57Z

encodings/sparse/src/canonical/builder.rs

+    match_each_integer_ptype!(indices.ptype(), |$I| {
+        let indices = indices.as_slice::<$I>();
+        for (&index, value) in indices.iter().zip(values.boolean_buffer().into_iter()) {
+            builder.inner.set_bit(index as usize, value);
+        }
+    });
+
+    // Set the validity from the sparse array.
+    builder.nulls.append_validity_mask(sparse.validity_mask()?);
+


shall we move this into the builder?

joseph-isaacs · 2025-02-19T09:47:22Z

encodings/sparse/src/canonical/builder.rs

+    builder
+        .values
+        .spare_capacity_mut()
+        .fill(MaybeUninit::new(fill_value));


Will this not fill past the arr len? Is that performance or correct?

Ah I see, I can just trim this to [..len] and that's probably better

joseph-isaacs · 2025-02-19T09:48:01Z

Did sparse show up as slow to canonicalize?

a10y · 2025-02-19T14:34:52Z

Moving to draft to clean things up.

@joseph-isaacs We use Sparse as a standalone compressor in vortex-btrblocks so I'd expect it to be more frequent in our encoding trees

codspeed-hq · 2025-02-20T23:18:08Z

CodSpeed Performance Report

Merging #2420 will degrade performances by 23.33%

_{Comparing aduffy/sparse-canonicalize-into (ad9eb84) with develop (174e5b5)}

Summary

❌ 1 regressions
✅ 764 untouched benchmarks
🆕 8 new benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
🆕	`canonicalize_into[0.001]`	N/A	309.9 µs	N/A
🆕	`canonicalize_into[0.01]`	N/A	329 µs	N/A
🆕	`canonicalize_into[0.05]`	N/A	410.8 µs	N/A
🆕	`canonicalize_into[0.1]`	N/A	509.7 µs	N/A
🆕	`into_canonical[0.001]`	N/A	310.4 µs	N/A
🆕	`into_canonical[0.01]`	N/A	325.2 µs	N/A
🆕	`into_canonical[0.05]`	N/A	387.4 µs	N/A
🆕	`into_canonical[0.1]`	N/A	474.3 µs	N/A
❌	`compress[(ALPRDCompressor, F32)]`	35.4 ms	46.2 ms	-23.33%

gatesn reviewed Feb 19, 2025

View reviewed changes

0ax1 reviewed Feb 19, 2025

View reviewed changes

joseph-isaacs reviewed Feb 19, 2025

View reviewed changes

a10y marked this pull request as draft February 19, 2025 14:33

a10y added 3 commits February 20, 2025 14:16

feat: impl canonical_into for SparseArray

06e6c1f

save

9fc8483

better sparse bools

ad9eb84

a10y force-pushed the aduffy/sparse-canonicalize-into branch from 4ef8d81 to ad9eb84 Compare February 20, 2025 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: impl canonicalize_into for SparseArray #2420

feat: impl canonicalize_into for SparseArray #2420

a10y commented Feb 19, 2025 •

edited

Loading

gatesn Feb 19, 2025

gatesn Feb 19, 2025

a10y Feb 19, 2025

gatesn Feb 19, 2025

gatesn Feb 19, 2025

a10y Feb 19, 2025

gatesn Feb 19, 2025

0ax1 left a comment

0ax1 Feb 19, 2025

0ax1 Feb 19, 2025

0ax1 Feb 19, 2025

a10y Feb 20, 2025

joseph-isaacs Feb 19, 2025

joseph-isaacs Feb 19, 2025

a10y Feb 19, 2025

joseph-isaacs commented Feb 19, 2025

a10y commented Feb 19, 2025

codspeed-hq bot commented Feb 20, 2025

feat: impl canonicalize_into for SparseArray #2420

Are you sure you want to change the base?

feat: impl canonicalize_into for SparseArray #2420

Conversation

a10y commented Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0ax1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseph-isaacs commented Feb 19, 2025

a10y commented Feb 19, 2025

codspeed-hq bot commented Feb 20, 2025

Merging #2420 will degrade performances by 23.33%

Summary

Benchmarks breakdown

a10y commented Feb 19, 2025 •

edited

Loading