feat: add dense union encoding in vortex-spatial - #9367
Conversation
|
It seems like this actually has a similar issue to And if someone inevitably wants that behavior, they actually have no way to express it? I guess technically we can express it by canonicalizing into a sparse encoding by literally rebuilding the whole thing from scratch, but that is very inefficient. Maybe its time we add that garbage collection array that we've talked about for almost a year? |
| let type_ids = array.type_ids().take(indices.clone())?; | ||
| let fill_scalar = Scalar::zero_value(&indices.dtype().as_nonnullable()); | ||
| let offset_indices = indices.clone().fill_null(fill_scalar)?; | ||
| let offsets = array.offsets().take(offset_indices)?; |
There was a problem hiding this comment.
I think this does not actually work? If we look at https://arrow.apache.org/docs/format/Columnar.html#dense-union it says this:
The respective offsets for each child value array must be in order / increasing.
So if we take on the offsets we can reorder these which breaks sortedness per child.
So "not correct" here just means not zero-copyable to Arrow for this encoding. But maybe we don't care? It seems generally useful to have things out of order for more efficient take?
@robert3005 do you have any thoughts?
There was a problem hiding this comment.
Maybe in vortex you don't need a dense union and you only ever produce it on export to arrow? Is there a particular case where you find DenseUnion to be necessary?
There was a problem hiding this comment.
Yes, Vortex can represent this using canonical sparse unions, but DenseUnion is intended as a performance encoding for GeoArrow Geometry.
|
the AI review comments were verbose so I deleted them (claude cant edit review comments seemingly). I will ask it to post again, but with more concise explanations |
connortsui20
left a comment
There was a problem hiding this comment.
Reviewed 568bf2a. Differential tests against the sparse UnionArray across slice, filter, take, mask, and canonicalize agreed on every case, including nullable variants, null take indices, empty children, and zero-length results, so no correctness bug turned up in the data paths.
Two findings worth blocking on. The canonicalize hot loop does a per-row linear tag scan and allocates len * variants zeroed codes, which is where the 2.1x goes. DenseUnion also cannot be written to a file, because no edition declares vortex.dense_union. The benchmark behind the performance table never runs in CI either.
The Arrow offset-ordering question on take is still open. The differential tests would not have caught it, because they compare against Vortex sparse unions rather than Arrow.
Generated by Claude Code
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
6c45c9a to
7ad1027
Compare
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| 🆕 | Simulation | dense_take |
N/A | 3.5 ms | N/A |
| 🆕 | Simulation | sparse_take |
N/A | 2 ms | N/A |
Comparing nemo/dense-union (7459934) with develop (b363fb7)
Footnotes
-
89 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Rationale for this change
GeoArrow mixed-geometry arrays use dense unions, while the Vortex canonical union representation is sparse. This PR adds a generic external dense physical encoding without moving encoding policy into
vortex-array.What changes are included in this PR?
vortex-dense-unionencoding for logicalDType::Unionvalues.UnionArrayusing dictionary-backed children without copying payload values.What APIs are changed? Are there any user-facing changes?
vortex-dense-unioncrate and itsDenseUnionconstruction/accessor APIs.DType::Unionand canonical sparseUnionArrayremain invortex-array.Performance
takeUnionArray