Skip to content

Commit 78096e0

Browse files
dfa1claude
andcommitted
docs(adr): split read and write runtimes out of core
The 33 `unwrapForSubParser` audit-trail sites in PR #27 are a strong signal that the current module layout is wrong: `core` hosts the read runtime, not just the data model. `reader` is a thin orchestration shell around `core`; cross-module byte hand-offs force public escape-hatch APIs (`VortexHandle.slice`, `Registry.decodeAsSegment`) that no architectural wrapper can really cure. Proposes splitting `Encoding` into separate read and write interfaces and moving the corresponding runtime into `reader` / `writer`, leaving `core` as the model-only module it was originally intended to be. PR #27's escape hatches collapse as a side effect of co-locating the decoder runtime with the byte source. Includes: - Root-cause analysis of the current `core` layout (5 concrete smells) - Target module split with the `Array` hierarchy moving to `reader` (writer only touches `NullableData`, confirmed by audit) — only `NullableData` and `BoundedSegment` stay in `core` proper - Registry split into distinct `ReadRegistry` and `WriteRegistry` types passed alongside the entry points (not folded into options records) — read-only callers never pull the `writer` module onto their classpath - 6-phase migration plan with effort estimates (~9 person-days plus CI fallout) - Consequences (positive and negative) and risks-to-manage - Rejected alternatives (JPMS hide, move FlatSegmentDecoder alone, adopt Arrow codec SPI, status-quo + docs) Status: Proposed. No code changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 060e1db commit 78096e0

1 file changed

Lines changed: 358 additions & 0 deletions

File tree

Lines changed: 358 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,358 @@
1+
# ADR 0001: Split read and write runtimes out of `core`
2+
3+
- **Status:** Proposed
4+
- **Date:** 2026-06-11
5+
- **Deciders:** project maintainer
6+
- **Supersedes:**
7+
- **Superseded by:**
8+
9+
## Context
10+
11+
The current module layout collapses the file-format model, the read runtime,
12+
and the write runtime into a single `core` module. The other modules
13+
(`reader`, `writer`, `inspector`, `cli`, `csv`, `jdbc`, `parquet`) are thin
14+
orchestration layers that call back into `core` for every meaningful operation.
15+
16+
### What `core` currently contains
17+
18+
```
19+
io.github.dfa1.vortex.core — DType, PType, Footer, Layout,
20+
VortexException, VortexFormat,
21+
Array hierarchy, ArrayStats
22+
io.github.dfa1.vortex.encoding — Encoding (encode + decode on one type),
23+
Registry (read + write dispatch),
24+
DecodeContext, EncodeContext,
25+
FlatSegmentDecoder, ArrayNode,
26+
30+ concrete *Encoding.java classes
27+
io.github.dfa1.vortex.extension — Extension interface, ExtensionId,
28+
4 spec extension impls
29+
io.github.dfa1.vortex.proto — generated proto records (in-tree codec)
30+
io.github.dfa1.vortex.fbs — generated flatbuffer types
31+
```
32+
33+
### The smell
34+
35+
1. **`Encoding` is bifunctional.** Every encoding implements both
36+
`encode(DType, Object, EncodeContext)` and `decode(DecodeContext)`. A
37+
read-only consumer (the most common deployment shape — analytical engines
38+
reading columnar files) pulls in the full write path, including Zstd
39+
compression libraries, dictionary builders, and stats sketchers.
40+
41+
2. **`Registry` is a dual dispatcher.** It maps `EncodingId` to a single
42+
`Encoding` instance and exposes both read (`decode`, `decodeAsSegment`) and
43+
write surfaces. Read-only callers carry write-side identity even when no
44+
write code path will ever fire.
45+
46+
3. **`reader` is a shell, not a runtime.** `VortexReader` memory-maps the
47+
file, parses the trailer/postscript/footer/layout, then hands off to
48+
`FlatSegmentDecoder` — which lives in `core`. From that point on every
49+
meaningful operation (per-buffer slicing, `Registry.decode` dispatch,
50+
`Encoding.decode` call) happens inside `core`. The `reader` module
51+
contributes ~3 KLOC of orchestration around ~30 KLOC of decode runtime that
52+
lives in `core`.
53+
54+
4. **The `slice()` escape hatch is forced by this layout.**
55+
[PR #27](https://github.com/dfa1/vortex-java/pull/27) wraps untrusted
56+
`MemorySegment.asSlice` calls in a `BoundedSegment` type, but ended up
57+
shipping 33 `unwrapForSubParser(...)` sites because the consumers
58+
(`Encoding.decode`, `FlatSegmentDecoder`, `Registry`) live in `core` while
59+
the byte-producing handle (`VortexHandle.slice`) lives in `reader`. They
60+
cannot share package-private access; the cross-module API must be public;
61+
the public API must expose raw segments because that is what the consumers
62+
take. Every typed wrapper added to plug the gap (`BoundedSegment`,
63+
`unwrapForSubParser`, `MemorySegments.slice`) is a workaround for the fact
64+
that the read runtime should not be in a different module than the byte
65+
source it consumes.
66+
67+
5. **Concrete consequences of (1)–(4):**
68+
- **`Registry.decodeAsSegment`** exists purely so `ScanIterator` (in
69+
`reader`) can decode a child node back into a raw `MemorySegment`
70+
an inversion of the natural data-flow direction.
71+
- **`DecodeContext.segmentBuffers`** had to become `BoundedSegment[]` so
72+
the security contract survives the cross-module hand-off; if decoders
73+
lived alongside `FlatSegmentDecoder`, package-private `MemorySegment[]`
74+
would have sufficed.
75+
- **Read-only jars cannot be smaller than the full reactor.** The CLI
76+
uber-jar pulls every encoder, every writer dependency (`zstd-jni`,
77+
`air-compressor`, etc.) even if the binary only reads files.
78+
- **`Extension`** mirrors the same problem at a smaller scale —
79+
`encodeAll` and decode helpers live on one interface, pulling
80+
write-side dependencies into read-only consumers.
81+
82+
## Decision
83+
84+
Split `core` into three logical surfaces along the read/write axis:
85+
86+
```
87+
core/ — only the model
88+
Encoding — id() + accepts(); no encode/decode methods
89+
EncodingId
90+
DType, PType, ArrayStats, Footer, Layout
91+
NullableData — the one Array-shaped type both sides need
92+
BoundedSegment — the byte-access primitive
93+
proto, fbs — generated code (already pure data)
94+
95+
reader/ — read runtime
96+
VortexReader, VortexHandle, VortexHttpReader, ScanIterator
97+
Array hierarchy — BoolArray, IntArray, LongArray, VarBinArray,
98+
StructArray, ... (the read-only data exchange
99+
format; writer never touches these)
100+
ReadRegistry — Map<EncodingId, EncodingDecoder>
101+
EncodingDecoder — Array decode(DecodeContext)
102+
DecodeContext, FlatSegmentDecoder
103+
BitpackedDecoder, AlpDecoder, PcoDecoder, ... (30+ files)
104+
105+
writer/ — write runtime
106+
VortexWriter
107+
WriteRegistry — Map<EncodingId, EncodingEncoder>
108+
EncodingEncoder — EncodeResult encode(DType, Object, EncodeContext)
109+
EncodeContext
110+
BitpackedEncoder, AlpEncoder, PcoEncoder, ... (30+ files)
111+
```
112+
113+
### How the registry attaches at the API surface
114+
115+
`Registry` splits into two distinct types — `ReadRegistry` and
116+
`WriteRegistry` — not a generic `Registry<T>`. Each is passed alongside
117+
its corresponding entry point, **not** folded into an options record:
118+
119+
```java
120+
ReadRegistry rr = ReadRegistry.builder().registerServiceLoaded().build();
121+
VortexReader.open(path, rr); // ReadRegistry directly
122+
123+
WriteRegistry wr = WriteRegistry.builder().registerServiceLoaded().build();
124+
VortexWriter.builder(path, schema)
125+
.registry(wr) // WriteRegistry directly
126+
.options(WriteOptions.defaults()) // tuning knobs only
127+
.build();
128+
```
129+
130+
Two design choices feed this shape.
131+
132+
**Distinct types, not `Registry<T>`.** Reasons:
133+
- Different builder ergonomics: the read side has no cascade chain to
134+
configure; the write side does (`cascadeCodecs`, allowed-cascading depth).
135+
A generic type would carry irrelevant builder methods on both sides.
136+
- `ServiceLoader` manifests are already separate
137+
(`META-INF/services/...EncodingDecoder` vs `...EncodingEncoder`), so
138+
type-level separation matches the runtime story.
139+
- Mistakes like passing a write registry to `VortexReader.open` become
140+
compile errors, not runtime errors.
141+
142+
**Alongside the options, not inside them.** Reasons:
143+
- `WriteOptions` is a record (an immutable value). `WriteRegistry` is a
144+
configured map with lifecycle: typically built once at app startup and
145+
reused across many file writes. Mixing forces re-creating options every
146+
time you want a new file with the same registry.
147+
- Records work badly for fields with non-trivial equality semantics
148+
(`Registry.equals`?).
149+
- Today the registry already lives on the method signature; keeping that
150+
split is the lowest-migration shape.
151+
152+
The same applies to read-side configuration. There is no `ReadOptions`
153+
record today (the reader takes `ScanOptions` per-scan instead); the
154+
proposal keeps that as-is. `ReadRegistry` is the file-open parameter;
155+
`ScanOptions` is the per-scan parameter.
156+
157+
Effect on caller code:
158+
- Read-only callers (analytics engines, inspector, CLI inspector)
159+
construct only `ReadRegistry`. No transitive dependency on writer
160+
encoders — the `writer` module isn't on their classpath at all.
161+
- Write-only callers (CSV importer, JDBC importer) construct only
162+
`WriteRegistry`.
163+
- Tools that do both (integration tests, parquet bridge) construct both.
164+
165+
### What changes structurally
166+
167+
- `Encoding` becomes a small metadata-only interface in `core`. It carries
168+
`EncodingId` and `accepts(DType)` and nothing else. No bifunctional decode
169+
+ encode methods.
170+
- Each encoding's `Decoder` static inner class becomes a top-level
171+
`EncodingDecoder` implementation in `reader`. The `Encoder` inner class
172+
becomes `EncodingEncoder` in `writer`. CLAUDE.md already documents this
173+
split via private inner classes; the migration largely lifts those into
174+
separate compilation units across modules.
175+
- `Registry` splits into `ReadRegistry` and `WriteRegistry`. Each registry
176+
exposes only the dispatch surface its side needs. The `decodeAsSegment`
177+
escape hatch is deleted; the corresponding adapter logic lives in
178+
`FlatSegmentDecoder` (in `reader`) instead.
179+
- `DecodeContext` moves to `reader`; `EncodeContext` moves to `writer`.
180+
- `FlatSegmentDecoder` moves to `reader`, into the same package as
181+
`VortexReader`. The `slice()` method on `VortexHandle` becomes
182+
package-private — `FlatSegmentDecoder`, `Trailer`, and `PostscriptParser`
183+
are its only callers, all co-resident in `reader/io`.
184+
- `unwrapForSubParser` and the corresponding audit trail collapse to the
185+
minority of decoders that genuinely call into a sub-parser (`ProtoReader`)
186+
with a raw `MemorySegment`. Cross-module byte hand-offs disappear.
187+
- `Extension` similarly splits into `ExtensionDecoder` + `ExtensionEncoder`,
188+
or keeps a single interface with read-only and write-only sub-types.
189+
190+
### Effect on the `slice()` problem
191+
192+
The motivating problem disappears as a side effect:
193+
194+
- `VortexHandle.slice(long, long)` → package-private. External callers cannot
195+
see it; cross-module consumers (`ScanIterator`, `InspectorTree`) move into
196+
the same module so the package-private access works.
197+
- `BoundedSegment` stays in `core` as the primitive, but no longer needs to
198+
travel through public API surfaces. Most internal uses can drop back to
199+
raw `MemorySegment` because they live in the same package as the byte
200+
source and the trust boundary is now spatially local.
201+
- The 33 `unwrapForSubParser` sites from PR #27 are mostly eliminated —
202+
not because we wrote more wrappers, but because the wrappers are no longer
203+
needed once read code stops crossing module boundaries to reach its bytes.
204+
205+
## Migration phases
206+
207+
Each phase is a separate PR, lands independently green, and keeps the old
208+
shape running side-by-side until cut-over.
209+
210+
**Phase 0 — preparation (≈0.5 day)**
211+
- Land this ADR.
212+
- Add `Encoding` metadata-only interface in `core` (extends the existing
213+
one for now). Verify all current `Encoding` impls already implement
214+
`id()` and `accepts(DType)`.
215+
- Introduce `ReadRegistry` and `WriteRegistry` skeletons that for now
216+
delegate to the existing `Registry`. No call-site changes yet.
217+
218+
**Phase 1 — split `DecodeContext` and the read registry (≈1 day)**
219+
- Move `DecodeContext`, `ArrayNode`, `FlatSegmentDecoder` to `reader`.
220+
- `ReadRegistry` becomes the canonical read dispatcher; `Registry.decode`
221+
forwards to it during transition.
222+
- `ScanIterator` uses `ReadRegistry` directly.
223+
- `decodeAsSegment` deleted; `FlatSegmentDecoder` gains the equivalent
224+
package-private helper.
225+
226+
**Phase 2 — lift `*Decoder` impls into `reader` (≈1 day per family, ≈3 days)**
227+
- Pick one encoding family at a time (Fastlanes, ALP, Pco, …).
228+
- For each: extract the `Decoder` inner class into a new
229+
`*EncodingDecoder` in `reader/encoding`; register via
230+
`META-INF/services/...EncodingDecoder`; delete the `decode(...)` method
231+
from the old `*Encoding` in `core`.
232+
- After all families lifted, `Encoding` in `core` no longer has a `decode`
233+
method. `Registry` (the old dual) no longer has a read surface.
234+
235+
**Phase 3 — repeat for the write side (≈3 days)**
236+
- Mirror Phase 2 for writers. `Encoding` in `core` becomes the
237+
metadata-only shape promised in the Decision section.
238+
239+
**Phase 4 — `VortexHandle.slice` to package-private (≈0.5 day)**
240+
- Drop `slice()` from the public `VortexHandle` interface. All remaining
241+
callers are now in `reader` and use a package-private accessor on the
242+
concrete `VortexReader` / `VortexHttpReader` types.
243+
- Inspector and CLI inspector code that today calls `handle.slice(...)`
244+
receives a new typed accessor instead (e.g.
245+
`FlatSegmentInspector.peek(handle, spec)`).
246+
- The 33 `unwrapForSubParser` sites from PR #27 are deleted at the same
247+
time; the corresponding decoders take raw `MemorySegment` again because
248+
they live in the same package as the byte source.
249+
250+
**Phase 5 — `Extension` split (≈0.5 day)**
251+
- `ExtensionDecoder` and `ExtensionEncoder` in their respective modules.
252+
- Confirm the four spec extensions (`Date`, `Time`, `Timestamp`, `Uuid`)
253+
ride through the split cleanly.
254+
255+
**Phase 6 — read-only jar artifact (≈0.5 day)**
256+
- Verify the CLI's "read-only" personality (the inspector) can be built
257+
without the writer module on the classpath. Document in `compatibility.md`.
258+
259+
Cumulative effort estimate: ~9 person-days of focused work, plus ~3 days
260+
of CI / integration-test fallout, plus reviewer time. Not a weekend.
261+
262+
## Consequences
263+
264+
### Positive
265+
266+
- **Public API never exposes raw `MemorySegment`** for the read path.
267+
`VortexHandle.slice` disappears from the public surface. The SECURITY.md
268+
contract is enforced architecturally, not by audit-trail convention.
269+
- **PR #27's 33 `unwrapForSubParser` sites collapse to a handful**
270+
only the decoders that genuinely call a sub-parser
271+
(`ProtoReader`-bound decoders: Constant, Pco, Sparse, plus Zstd's
272+
native lib hand-off) retain a documented trust transfer.
273+
- **`Registry.decodeAsSegment` deleted.** The current adapter exists only
274+
because cross-module dispatch needs a raw-segment escape; once decoders
275+
are co-resident with the byte source, the adapter is no longer needed.
276+
- **Read-only deployments shrink.** No transitive pull on `zstd-jni`
277+
encode paths, FSST dictionary builders, ALP encoders, etc. The CLI
278+
inspector becomes a true read-only artifact.
279+
- **Dependency direction matches data flow.** `reader` depends on `core`;
280+
`writer` depends on `core`; neither depends on the other. Today both
281+
live inside `core` and the dependency direction is invisible.
282+
283+
### Negative
284+
285+
- **Multi-day refactor.** ~9 person-days plus CI iteration. Cannot land in
286+
a single PR; must be staged carefully so each phase runs green.
287+
- **Encoding impls double in file count** during transition. `BitpackedEncoding`
288+
becomes `BitpackedDecoder` (in `reader`) + `BitpackedEncoder` (in `writer`).
289+
Test files split similarly.
290+
- **CLAUDE.md updates** — the "three touch-points for adding an encoding"
291+
rule becomes "decoder side + encoder side + EncodingId enum constant",
292+
each in its own module.
293+
- **CHANGELOG breaking-changes section grows.** External users (none today,
294+
but any future ones) see `Encoding`, `Registry`, and `DecodeContext`
295+
moved. Probably worth bundling under a 0.7.0 release boundary.
296+
- **Two `ServiceLoader` manifests per encoding** instead of one.
297+
- **Integration tests need re-routing.** Tests that today construct a
298+
`Registry` and call `decode` directly will need to construct a
299+
`ReadRegistry` instead — mechanical but pervasive.
300+
301+
### Risks to manage
302+
303+
- **Side-by-side period drift.** Phases 1–3 leave both the old `Registry`
304+
and the new `ReadRegistry`/`WriteRegistry` registered for each encoding
305+
during transition. Risk: divergent behaviour if a bug fix lands on one
306+
side and not the other. Mitigation: integration tests run against both
307+
paths during the transition; the old `Registry` becomes a thin forwarder
308+
early in Phase 1.
309+
- **Extension split.** `Extension` carries the same encode/decode tension
310+
as `Encoding`; the migration plan assumes a parallel split. If the
311+
extension API has tighter user-facing constraints (it does — see
312+
`DateExtension.decodeAll`), Phase 5 may need a separate ADR.
313+
- **JMH benchmarks.** `RustVsJavaReadBenchmark` and friends construct
314+
`Registry` + `DecodeContext` directly. They live in the `performance`
315+
module, which depends on `reader`. The benchmarks need re-wiring at the
316+
end of Phase 2.
317+
318+
## Alternatives considered
319+
320+
- **Keep `core` as-is, hide `slice()` via Java modules (JPMS).** Drops
321+
PR #27's escape-hatch noise but does not address the underlying smell —
322+
`core` still hosts the read runtime, `reader` still calls back into
323+
`core` for every operation, `Registry` still dispatches both sides,
324+
and read-only deployments still pull the writer surface. Rejected as
325+
cosmetic.
326+
- **Move `FlatSegmentDecoder` alone into `reader`, leave everything else.**
327+
Solves the immediate `slice()` problem at the cost of a circular module
328+
dependency: `Encoding.decode` (in `core`) would call
329+
`Registry.decode` (in `core`) which would route into
330+
`FlatSegmentDecoder` (in `reader`). Rejected as architecturally worse
331+
than the current state.
332+
- **Adopt an existing pluggable codec framework (e.g. Arrow's
333+
`CompressionCodec` SPI shape).** Considered briefly. Vortex's
334+
cascading-encoding model has tighter requirements than Arrow's flat
335+
codec model; an external SPI does not fit. Rejected.
336+
- **Status quo + documentation.** Document that `core` is the read runtime
337+
and `reader` is a shell. Cheapest. Rejected because every future
338+
feature that needs cross-module byte access reintroduces the same
339+
escape-hatch problem.
340+
341+
## Decision drivers
342+
343+
- The 33 `unwrapForSubParser` sites in PR #27 are a strong proxy signal:
344+
every one of them documents a place where read code needs bytes that
345+
live in another module.
346+
- A genuinely read-only deployment (inspector + scan) should be possible
347+
without pulling Zstd encoders or FSST builders. Today it is not.
348+
- The `Encoding` interface bifunctional shape blocks ahead-of-time
349+
pruning of the write surface; the refactor is the only path to a
350+
smaller read-only artifact.
351+
352+
## References
353+
354+
- [PR #27`sec(parser): BoundedSegment + audit trail for untrusted asSlice`](https://github.com/dfa1/vortex-java/pull/27)
355+
- [Phase 1–4 commits — BoundedSegment introduction and migration](https://github.com/dfa1/vortex-java/pull/27/commits)
356+
- [SECURITY.md — the contract this work hardens](../../SECURITY.md)
357+
- [CLAUDE.md — current "three touch-points" rule for adding an encoding](../../CLAUDE.md)
358+
- [TODO.md — parser hardening backlog](../../TODO.md)

0 commit comments

Comments
 (0)