Repository navigation
Write Widen arrays through an array plugin - #11002
connortsui20 wants to merge 2 commits into
Conversation
Merging this PR will degrade performance by 15.61%1 warning
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
de8a7a6 to
f3d53cc
Compare
851cd14 to
9556b96
Compare
f3d53cc to
72749e5
Compare
| fn serialize( | ||
| &self, | ||
| array: &ArrayRef, | ||
| session: &VortexSession, | ||
| ) -> VortexResult<Option<ArraySerialization>> { | ||
| // TODO(connor)[Widen]: Add `vortex.widen` to both ID lists when it gets a wire form | ||
| // (#10733), and write it only where the writer permits it. This method cannot see the | ||
| // permitted IDs today. | ||
|
|
||
| // The child validity can be lazy, so the writer needs recursive canonicalization to | ||
| // produce nodes that it can store. | ||
| let canonical = array | ||
| .clone() | ||
| .execute::<RecursiveCanonical>(&mut session.create_execution_ctx())? | ||
| .0 | ||
| .into_array(); | ||
|
|
||
| session.array_serialize(&canonical) | ||
| } |
There was a problem hiding this comment.
Here is an open question: instead of doing this, do we want to have a more generalized version of this fix?
Not saying the PR should be blocked on this (as I do need to make progress on this stack), but there exist other encodings that we only have in-memory that we would want to ensure do not get serialized to disk (either because of an edition, or because we do not want them to be serialized ever).
Some examples:
FilterSlicePatched(the LLM is even telling me that this is a bug, andPatchedneeds a similar change as this one)ScalarFn(which we serialize now but imo we might want a different name for serialized scalar functions, see Tracking Issue: Encodings vs Scalar Functions #9156)- Any encoding that might easily be created in memory but we disallow via editions
There was a problem hiding this comment.
This shouldn't be in serialisation at this point we are waaaay too late. At this point we shouldn't execute but error and force the higher level to handle it. We need to do a normalisation pass which removes all non serialisable ids from the array tree
There was a problem hiding this comment.
oh actually we could also just write the "old" decimal array here...
Not sure if that is better.
Tracking Issue: #10701 Bottom of the decimal `Widen` stack. #11002 builds on it. No release gate. ## Summary A plugin can need another encoding's wire form without taking over its reader. `serialized_ids()` currently serves both jobs, so a `Widen` writer cannot emit a narrow decimal node while preserving the decimal reader. ## Changes Reader registration now uses `read_ids()`, while serialization validates its output against `write_ids()`. A writing session does not need a reader for every ID it writes. Existing plugins keep their current IDs, and tests cover reader ownership and the two validation contracts. `LayoutPlugin` is unchanged. ## API Changes Replaces `ArrayPlugin::serialized_ids()` with required `read_ids()` and `write_ids()` methods, with no defaults. Read IDs stay ordered oldest to newest. --------- Signed-off-by: Connor Tsui <[email protected]>
Signed-off-by: Connor Tsui <[email protected]>
Signed-off-by: Connor Tsui <[email protected]>
72749e5 to
7929610
Compare
robert3005
left a comment
There was a problem hiding this comment.
We should not execute
| fn serialize( | ||
| &self, | ||
| array: &ArrayRef, | ||
| session: &VortexSession, | ||
| ) -> VortexResult<Option<ArraySerialization>> { | ||
| // TODO(connor)[Widen]: Add `vortex.widen` to both ID lists when it gets a wire form | ||
| // (#10733), and write it only where the writer permits it. This method cannot see the | ||
| // permitted IDs today. | ||
|
|
||
| // The child validity can be lazy, so the writer needs recursive canonicalization to | ||
| // produce nodes that it can store. | ||
| let canonical = array | ||
| .clone() | ||
| .execute::<RecursiveCanonical>(&mut session.create_execution_ctx())? | ||
| .0 | ||
| .into_array(); | ||
|
|
||
| session.array_serialize(&canonical) | ||
| } |
There was a problem hiding this comment.
This shouldn't be in serialisation at this point we are waaaay too late. At this point we shouldn't execute but error and force the higher level to handle it. We need to do a normalisation pass which removes all non serialisable ids from the array tree
Tracking Issue: #10701 Based on `develop`, including the writer work previously in #11002. #11003 and #10994 build on it. No release gate. ## Summary Canonical decimal width follows precision, so small values under `Decimal(38, 2)` still occupy 16 bytes each. A decimal `Widen` keeps those values in a lower-precision child while retaining the column's logical dtype. ## Changes The child precision must hold every valid value and have a narrower canonical width. Execution restores the logical width. Until the later kernels land, operations use that canonical fallback. Serialization writes a concrete decimal leaf through the existing `vortex.decimal` wire form without executing values or validity. Integer `Widen` arrays, non-decimal leaves, and nonserializable validity require explicit preparation by the caller. Its metadata stores the width, while the containing format supplies the parent precision. This keeps an `i32` child at four bytes per value, including IPC and writes without a compressor. A serialized-byte regression test checks that size independently of the eventual read encoding. <details> <summary>Historical local benchmark results</summary> These retained measurements predate this style pass and rebase. They do not measure the current head. `cargo bench -p vortex-array --bench widen_decimal`, Apple M4, medians. The numbers were measured at the top of this stack before the split, so they cover the whole stack, not this layer alone. The baseline is the canonical input, and the candidate is the same values as a `Widen` (child precision 5 under precision 10, 9 under 18, and 18 under 38). CodSpeed on this PR measures the layer itself. | bench | p10 canonical | p10 Widen | p18 canonical | p18 Widen | p38 canonical | p38 Widen | | --- | ---: | ---: | ---: | ---: | ---: | ---: | | `filter` | 22.16 µs | 5.083 µs | 13.66 µs | 4.916 µs | 10.58 µs | 24.74 µs | | `execute` | 10.59 ns | 2.832 µs | 10.26 ns | 2.926 µs | 10.34 ns | 9.895 µs | `execute` on a `Widen` is the widening copy, which the canonical input does not need. The canonical `filter` medians at p10 and p18 were bimodal across runs, and `filter` at p38 is slower on the `Widen`. </details> ## API Changes `WidenArray::try_new` accepts decimal children. Adds `WidenArray::encode_decimal` and registers a writer-only `WidenPlugin`, whose only write ID is `vortex.decimal`. ## Validation Source review and rebase range-diffs only. No local tests, builds, linting, or documentation checks were run. A commit hook flagged one formatting wrap, corrected manually without rerunning the check. --------- Signed-off-by: Connor Tsui <[email protected]>
Folded into #10946. The replacement writes a concrete decimal leaf in the existing
vortex.decimalwire form without executing arrays during serialization. UnsupportedWideninputs require explicit caller preparation. The original branch is retained for recovery.Original proposal
Tracking Issue: #10701
Stacked on #11057. #10946 builds on it. No release gate.
Summary
Widenhas no wire form yet (#10733), but IPC and writes without a compressor still need to store it. A writer plugin makes that work through the same session API used by direct metadata callers.Changes
An integer
Widenwrites its canonical primitive form. Recursive canonicalization makes lazy child validity storable, while keeping theWidenas the serialization source preserves statistics in its logical dtype. Restricted contexts need only the existing primitive wire ID. Decimal writes follow in #10946.API Changes
Adds public
vortex_array::arrays::widen::WidenPluginto the defaultArraySession. Empty array sessions can register it explicitly. The plugin registers no reader becausevortex.widenhas no wire form.