diff --git a/adr/0025-centralize-zone-map-stats-computation.md b/adr/0025-centralize-zone-map-stats-computation.md new file mode 100644 index 00000000..be10b0e8 --- /dev/null +++ b/adr/0025-centralize-zone-map-stats-computation.md @@ -0,0 +1,139 @@ +# ADR 0025: Centralize zone-map MIN/MAX stats computation in the writer + +- **Status:** Accepted +- **Date:** 2026-09-13 +- **Deciders:** project maintainer +- **Supersedes:** — +- **Superseded by:** — + +## Context + +Between #382 and #387, thirteen `EncodingEncoder` implementations (`AlpRd`, `Constant`, `RunEnd`, +`ZigZag`, `Sequence`, `Pco`, `Rle`, `Sparse`, `Patched`, `Zstd`, `Fsst`, `VarBinView`, +`Ext`'s cascade path, `DateTimeParts`) were found hardcoding `null, null` for zone-map `MIN`/`MAX` +stats in their `EncodeResult`/`CascadeStep`, regardless of input. Each bug was independent — +different encoder, different author intent, no shared cause beyond "an encoder is responsible for +computing and reporting its own stats, and this one didn't." Two things made this bug class +dangerous in this specific way: + +1. **No test could catch it structurally.** The entire existing test suite (`RoundTripPropertyTest`, + the Rust-interop integration tests) asserts decoded *values* are correct. Broken pruning changes + nothing about what a scan returns — only how much it reads — so a test suite built around value + correctness is blind to it by construction. The eventual fix + (`ZoneMapStatsCoverageTest`) had to explicitly assert stats *presence* per encoder to close this, + which is itself evidence the underlying design had no structural guarantee. +2. **It kept recurring.** Thirteen instances across two PRs is not "one encoder had a bug" — it is + an architecture where the correct behavior is a convention every encoder must independently + remember, with nothing enforcing it. + +**How Rust avoids this.** The reference implementation (`spiraldb/vortex`) has no per-encoding +stats special-casing at all for most encodings. `StatsSet::compute_stat` computes `MIN`/`MAX` via +one generic aggregate reduction (`min_max`) over the array's canonical/decoded form — checked +directly against `encodings/datetime-parts/src/` (no stats code in `array.rs`, no aggregate kernel +in `compute/`) and `encodings/zigzag/src/array.rs`'s `test_compute_statistics`, which asserts a +zigzag-encoded array's computed stats equal the underlying array's. A few encodings (e.g. constant) +*do* provide a cheaper override, but the generic path is what makes stats a guarantee rather than a +convention — an encoding that provides no override still gets correct stats for free. + +## Decision + +Move the required computation to the one choke point every written segment already passes +through — `VortexWriter#writeSegment` (private) — instead of trusting each encoder's own +`EncodeResult`/`CascadeStep`: + +```java +if (result.hasStats()) { + lastStatsMin = result.statsMin(); + lastStatsMax = result.statsMax(); +} else { + byte[][] fallback = ZoneMapStatCodec.columnMinMax(dtype, data); + lastStatsMin = fallback != null ? fallback[0] : null; + lastStatsMax = fallback != null ? fallback[1] : null; +} +``` + +`ZoneMapStatCodec.columnMinMax(DType, Object)` is the new generic fallback, computed from the +segment's original `(dtype, data)` — untouched by whichever encoding wins — mirroring +`columnSum`, which already worked this way and, not coincidentally, never had this bug. It dispatches +on the same dtype shapes `zoneMinMaxDtype` already recognizes (`Primitive`, `Extension` with +`Primitive` storage, `Utf8`), compacting a nullable primitive column to its valid-only elements +first via `PrimitiveArrays#compact` (an invalid slot's placeholder, commonly `0`, is not a min/max +identity the way it is a sum identity — the #381 bug this generalizes) and skipping `null` entries +directly for a nullable Utf8 column, matching `VarBinEncodingEncoder#minMaxStats`'s own null-safe +loop. + +This makes stats coverage an **encoder-independent guarantee**: an encoder MAY still report a +cheaper override when it can (kept for `Constant`/`Sequence`, whose extremes are O(1) known from +what they already validated, and `RunEnd`/`ZigZag`, whose stats are folded into a single-pass loop +they must run anyway), but never MUST. The nine other fixed encoders (`Pco`, `Rle`, `Sparse`, +`Patched`, `Zstd`, `Fsst`, `VarBinView`, `Ext`'s cascade path, `DateTimeParts`) had their explicit +stats computation *removed*, not kept: each was a second, unfused pass over the same input with no +efficiency benefit over the one generic pass the fallback already does. + +**The `ComparableValues` escape hatch.** Most encoders receive a plain typed array +(`long[]`/`String[]`/...) as `data`, but `DateTimePartsData` is a carrier holding pre-split +day/second/subsecond values — its *comparable* form (the original combined timestamp) isn't its +literal shape. `ComparableValues` (`ptype()` + `values()`) lets such a carrier expose its +comparable form; `columnMinMax` checks for it before falling back to dtype-shape dispatch. This is +the write-side mirror of what Rust's own generic fallback does for the exact same encoding: Rust +has no `DateTimeParts`-specific stats code either — it decodes to canonical form +(`canonical.rs`'s `decode_to_temporal`, recombining `days * 86400 * divisor + seconds * divisor + +subseconds`) and runs the generic reduction over that. Java's writer never needs to decode to get +there; the pre-split values are already sitting in the carrier. + +## Consequences + +### Positive + +- A future encoder that forgets to compute stats now gets them anyway, for free, correctly. The + bug class this ADR responds to cannot recur for any dtype shape `columnMinMax` already handles. +- Nine encoders got simpler (explicit stats computation deleted), not more complex — the + centralization was a net code reduction, not a new abstraction layered on top of the old one. +- `ZoneMapStatsCoverageTest`'s guarantee moved from "every encoder reports its own stats" (a + per-encoder enumeration that grows forever and is easy to under-cover) to "`columnMinMax` handles + every dtype shape it's supposed to" (`ZoneMapStatCodecTest`, a fixed, small surface: `Primitive` + signed/unsigned, `Extension`+`Primitive` storage, `Extension`+`ComparableValues`, `Utf8`, each + nullable, plus the excluded shapes — `Decimal`, `Bool`, `Binary`, structural types). + +### Negative + +- Two sources of truth for stats now exist in principle (an encoder's own override vs. the + fallback), even though only four encoders use the override today. A reviewer adding stats logic + to a new encoder must know the fallback exists and ask whether the override is actually cheaper + before adding one — a `Sonar`/review-time judgment call, not something enforced by a type. +- `columnMinMax`'s dispatch (`Primitive`, `Extension`+`Primitive` storage, `Utf8`, + `ComparableValues`) is a second place (alongside `zoneMinMaxDtype`) that must stay in sync with + which dtype shapes are stats-eligible. They already agree today; nothing forces them to keep + agreeing if `zoneMinMaxDtype` gains a new case later. + +### Risks to manage + +- If a future encoding needs a *cheaper-than-generic* override (mirroring `Constant`/`Sequence`), + the reviewer must resist the temptation to skip it "since the fallback handles it anyway" when + the fallback would in fact cost a real second pass over large data — the fallback removes the + *correctness* risk, not the *performance* one. + +## Alternatives considered + +- **Keep per-encoder stats mandatory, close the gap with only a coverage test.** This is what + #387 shipped first (`ZoneMapStatsCoverageTest` asserting every registry-selectable encoder has a + case). It works, but only as long as every future PR remembers to add a case — a test that must + itself be remembered is a weaker guarantee than a fallback that runs unconditionally. Superseded + by this ADR once the central fallback existed to make the coverage test's job largely moot for + new encoders. +- **Fully centralize: remove the override capability entirely, no encoder ever reports its own + stats.** Rejected — `Constant`/`Sequence`'s O(1) shortcuts and `RunEnd`/`ZigZag`'s single-pass + fusion are real, free efficiency wins the generic fallback cannot replicate (it would need a + second full-array pass). Matches Rust's own shape: a required generic path plus optional + cheaper overrides, not an all-or-nothing choice. +- **Have `columnMinMax` itself decode `DateTimePartsData` (or any future composite carrier) via + its own split-specific logic**, instead of a `ComparableValues` marker interface. Rejected: it + would make the generic writer-level function aware of one specific encoding's internal + representation, coupling in the wrong direction. The marker interface lets the carrier own the + knowledge of its own comparable form, matching how Rust's `canonical.rs` lives inside the + `datetime-parts` encoding crate, not in the generic stats module. + +## References + +- [#382](https://github.com/dfa1/vortex-java/issues/382), [#384](https://github.com/dfa1/vortex-java/issues/384), [#385](https://github.com/dfa1/vortex-java/issues/385), [#386](https://github.com/dfa1/vortex-java/issues/386), [#387](https://github.com/dfa1/vortex-java/pull/387) — the thirteen encoders this generalizes. +- `spiraldb/vortex`: `vortex-array/src/stats/array.rs` (`StatsSet::compute_stat`, `min_max` generic reduction), `encodings/datetime-parts/src/canonical.rs` (`decode_to_temporal`), `encodings/zigzag/src/array.rs` (`test_compute_statistics`). diff --git a/adr/ADR.md b/adr/ADR.md index f9571e24..577af09f 100644 --- a/adr/ADR.md +++ b/adr/ADR.md @@ -39,3 +39,4 @@ the decision shipped in (blank = not yet shipped). | 0022 | Extract FSST into a standalone module, ported faithfully from the paper | Accepted | | | 0023 | Adopt the Vortex editions model as a client-side write/read policy | Accepted | | | 0024 | JPMS adoption for core/reader/writer | Proposed | | +| 0025 | Centralize zone-map MIN/MAX stats computation in the writer | Accepted | | diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/VortexWriter.java b/writer/src/main/java/io/github/dfa1/vortex/writer/VortexWriter.java index e5c0681b..cc69826e 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/VortexWriter.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/VortexWriter.java @@ -660,8 +660,19 @@ private int writeSegment(DType dtype, Object data, EncodingEncoder encodingOverr bytesWritten += 4; segs.add(new SegRef(offset, bytesWritten - offset)); - lastStatsMin = result.statsMin(); - lastStatsMax = result.statsMax(); + // The winning encoder's own stats win when present (a cheaper override, e.g. + // vortex.constant already knows its value is both extremes) -- otherwise the generic + // fallback computes them from the untouched input, independent of which encoder ran. + // This is what makes stats coverage an encoder-independent guarantee rather than a + // per-encoder convention every new encoder has to remember (ADR 0025). + if (result.hasStats()) { + lastStatsMin = result.statsMin(); + lastStatsMax = result.statsMax(); + } else { + byte[][] fallback = ZoneMapStatCodec.columnMinMax(dtype, data); + lastStatsMin = fallback != null ? fallback[0] : null; + lastStatsMax = fallback != null ? fallback[1] : null; + } lastStatsSum = ZoneMapStatCodec.columnSum(dtype, data); lastNullCount = segNullCount; return segIdx; diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/ZoneMapStatCodec.java b/writer/src/main/java/io/github/dfa1/vortex/writer/ZoneMapStatCodec.java index 898a6a4d..16f60f6d 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/ZoneMapStatCodec.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/ZoneMapStatCodec.java @@ -1,10 +1,13 @@ package io.github.dfa1.vortex.writer; +import io.github.dfa1.vortex.core.compute.PrimitiveArrays; import io.github.dfa1.vortex.core.model.DType; import io.github.dfa1.vortex.core.model.PType; import io.github.dfa1.vortex.core.proto.ProtoScalarValue; +import io.github.dfa1.vortex.writer.encode.ComparableValues; import io.github.dfa1.vortex.writer.encode.NullableData; import io.github.dfa1.vortex.writer.encode.PrimitiveEncodingEncoder; +import io.github.dfa1.vortex.writer.encode.VarBinEncodingEncoder; import java.io.IOException; import java.lang.foreign.MemorySegment; @@ -84,6 +87,50 @@ static byte[] columnSum(DType dtype, Object data) { return PrimitiveEncodingEncoder.sumStat(p.ptype(), values); } + /// The serialized `{min, max}` pair for `data` of logical type `dtype`, or `null` when the + /// column has no recordable min/max ([#zoneMinMaxDtype] returns `null` for the same `dtype` + /// shapes). This is the generic fallback every column gets regardless of which + /// [io.github.dfa1.vortex.writer.encode.EncodingEncoder] wrote it — mirroring the Rust + /// reference, which computes stats through one generic reduction for every encoding rather + /// than trusting each encoding to report its own (see [ComparableValues]'s Rust-parity note). + /// `VortexWriter#writeSegment` (private) only calls this when the winning encoder's own + /// [io.github.dfa1.vortex.writer.encode.EncodeResult] didn't already supply stats -- an + /// encoder MAY still report a cheaper override (e.g. `vortex.constant` already knows its one + /// value is both the min and the max, no scan needed), but never MUST. + /// + /// Unlike [#columnSum] (sum-neutral placeholders make raw dense values safe to sum directly), + /// a nullable primitive column is first compacted to its valid-only elements via + /// [PrimitiveArrays#compact] -- an invalid slot's placeholder (commonly `0`) is not a min/max + /// identity the way it is a sum identity, and would otherwise corrupt the reported extremes + /// (the #381 bug this fix generalizes). A nullable Utf8 column needs no such compaction: + /// [VarBinEncodingEncoder#minMaxStats] already skips `null` array entries itself. + /// + /// @param dtype the segment's logical type + /// @param data the segment's input data, possibly [NullableData]- or [ComparableValues]-wrapped + /// @return a `{min, max}` pair, or `null` when `dtype` has no recordable min/max + static byte[][] columnMinMax(DType dtype, Object data) { + if (data instanceof ComparableValues cv) { + return PrimitiveEncodingEncoder.minMaxStats(cv.ptype(), cv.values()); + } + return switch (dtype) { + case DType.Primitive p -> PrimitiveEncodingEncoder.minMaxStats(p.ptype(), compactIfNullable(p.ptype(), data)); + case DType.Extension ext when ext.storageDType() instanceof DType.Primitive p -> + PrimitiveEncodingEncoder.minMaxStats(p.ptype(), compactIfNullable(p.ptype(), data)); + case DType.Utf8 _ -> { + Object values = data instanceof NullableData nd ? nd.values() : data; + yield values instanceof String[] strings ? VarBinEncodingEncoder.minMaxStats(strings) : null; + } + default -> null; + }; + } + + private static Object compactIfNullable(PType ptype, Object data) { + if (!(data instanceof NullableData nd)) { + return data; + } + return PrimitiveArrays.compact(ptype, nd.values(), nd.validity()); + } + /// Builds the per-zone min (or max) values array for the resolved min/max `dtype`, decoding each /// zone's serialized [ProtoScalarValue] stat into the array shape its encoder expects. A `null` /// entry in `statBytes` (the chunk's encoder did not surface a min/max, e.g. an all-null chunk, diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/CascadeStep.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/CascadeStep.java index 1e10f0f5..fea4a1d5 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/CascadeStep.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/CascadeStep.java @@ -46,21 +46,6 @@ public static CascadeStep notApplicable() { return new CascadeStep(null, List.of(), List.of(), null, null, false); } - /// Convenience: applicable step with open children, taking a `{min, max}`-or-`null` stats pair - /// (as returned by e.g. [PrimitiveEncodingEncoder#minMaxStats] / [VarBinEncodingEncoder#minMaxStats]) - /// instead of two independently-nullable arguments. - /// - /// @param partialRoot partially-assembled root encode node - /// @param ownedBuffers buffers owned directly by the root node - /// @param openChildren child slots to be filled recursively by the cascading compressor - /// @param stats a `{min, max}` pair, or `null` when neither stat is available - /// @return an applicable [CascadeStep] with `stats` unpacked into `statsMin`/`statsMax` - public static CascadeStep open(EncodeNode partialRoot, List ownedBuffers, - List openChildren, byte[][] stats) { - return new CascadeStep(partialRoot, ownedBuffers, openChildren, - stats != null ? stats[0] : null, stats != null ? stats[1] : null, true); - } - /// Returns `true` if this step has no open child slots. /// /// @return `true` if the step is terminal (no open children) diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ComparableValues.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ComparableValues.java new file mode 100644 index 00000000..0d3647eb --- /dev/null +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ComparableValues.java @@ -0,0 +1,31 @@ +package io.github.dfa1.vortex.writer.encode; + +import io.github.dfa1.vortex.core.model.PType; + +/// Implemented by a write-side data carrier whose logical comparable value differs from the +/// literal `Object data` an [EncodingEncoder] receives — e.g. [DateTimePartsData] splits a +/// timestamp into day/second/subsecond parts, so its comparable form for zone-map min/max is the +/// original combined timestamp, not the carrier itself. +/// +/// Mirrors how the Rust reference computes stats for `vortex.datetimeparts`: it has no +/// per-encoding stats override at all, so a generic min/max reduction runs after +/// `canonical.rs`'s `decode_to_temporal` recombines the parts back into a plain `i64` array — +/// the exact same `day * ticksPerDay + seconds * divisor + subseconds` arithmetic this +/// encoding's own writer already does in reverse. On the write side there's no need to +/// decode anything to get there: the pre-split values are already sitting in the carrier. +/// +/// `ZoneMapStatCodec#columnMinMax` (writer package) checks for this before falling back to +/// dtype-shape dispatch. +public interface ComparableValues { + + /// The primitive type of {@link #values()}. + /// + /// @return the comparable primitive type + PType ptype(); + + /// The original, pre-transform primitive array (`long[]`, `int[]`, ...) suitable for + /// [PrimitiveEncodingEncoder#minMaxStats]. + /// + /// @return the comparable values, in `ptype()`'s array shape + Object values(); +} diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/DateTimePartsData.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/DateTimePartsData.java index 8911ff8f..c4a5e5b1 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/DateTimePartsData.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/DateTimePartsData.java @@ -1,9 +1,25 @@ package io.github.dfa1.vortex.writer.encode; +import io.github.dfa1.vortex.core.model.PType; + /// Input data for DateTimePartsEncodingEncoder input data. /// +/// Implements [ComparableValues]: the carrier's comparable form for zone-map min/max is the raw +/// pre-split timestamp, not the carrier itself -- signed order matches chronological order +/// regardless of how the encoding later splits it into days/seconds/subseconds. +/// /// @param timestamps raw i64 timestamps (number of time units since Unix epoch) /// @param nullable whether the array has a validity (null) dimension @SuppressWarnings("java:S6218") // internal data carrier; record components are arrays of immutable primitives or refs that flow through pipelines without ever being compared. -public record DateTimePartsData(long[] timestamps, boolean nullable) { +public record DateTimePartsData(long[] timestamps, boolean nullable) implements ComparableValues { + + @Override + public PType ptype() { + return PType.I64; + } + + @Override + public Object values() { + return timestamps; + } } diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/DateTimePartsEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/DateTimePartsEncodingEncoder.java index fc18572d..cf0c506d 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/DateTimePartsEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/DateTimePartsEncodingEncoder.java @@ -88,10 +88,7 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { MemorySegment.ofArray(metaBytes), new EncodeNode[]{daysNode, secondsNode, subsecondsNode}, new int[]{}); - // The extension's zone-map min/max is its storage primitive's, unwrapped -- here the raw - // i64 timestamp before it's split into days/seconds/subseconds, not any of the three parts - // individually (signed i64 order matches chronological order regardless of the split). - return EncodeResult.of(root, List.copyOf(allBuffers), PrimitiveEncodingEncoder.minMaxStats(PType.I64, d.timestamps())); + return new EncodeResult(root, List.copyOf(allBuffers), null, null); } @Override @@ -139,10 +136,6 @@ public CascadeStep encodeCascade(DType dtype, Object data, EncodeContext encodeC new ChildSlot(DType.I64, seconds, 1), new ChildSlot(DType.I64, subseconds, 2)); - // See #encode -- same open-children stats requirement as ExtEncodingEncoder#encodeCascade: - // CascadingCompressor#spliceResult takes the step's own stats verbatim, never deriving them - // from a resolved child, so the raw pre-split timestamp's stats must be computed here. - return CascadeStep.open(partialRoot, List.of(), children, - PrimitiveEncodingEncoder.minMaxStats(PType.I64, d.timestamps())); + return new CascadeStep(partialRoot, List.of(), children, null, null, true); } } diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/EncodeResult.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/EncodeResult.java index e5684585..e0c8ab62 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/EncodeResult.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/EncodeResult.java @@ -44,16 +44,4 @@ public static EncodeResult simple(EncodingId encodingId, MemorySegment data) { public boolean hasStats() { return statsMin != null && statsMax != null; } - - /// Convenience factory taking a `{min, max}`-or-`null` stats pair (as returned by e.g. - /// [PrimitiveEncodingEncoder#minMaxStats] / [VarBinEncodingEncoder#minMaxStats]) instead of two - /// independently-nullable arguments. - /// - /// @param rootNode the root encode node describing the encoding tree structure - /// @param buffers flat list of data buffers in the order referenced by `rootNode` - /// @param stats a `{min, max}` pair, or `null` when neither stat is available - /// @return an [EncodeResult] with `stats` unpacked into `statsMin`/`statsMax` - public static EncodeResult of(EncodeNode rootNode, List buffers, byte[][] stats) { - return new EncodeResult(rootNode, buffers, stats != null ? stats[0] : null, stats != null ? stats[1] : null); - } } diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ExtEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ExtEncodingEncoder.java index 666d0508..bfcdb7e7 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ExtEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ExtEncodingEncoder.java @@ -46,7 +46,7 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { childResult = storageEncoder.encode(storage, data, ctx); } EncodeNode root = new EncodeNode(EncodingId.VORTEX_EXT, null, new EncodeNode[]{childResult.rootNode()}, new int[0]); - return new EncodeResult(root, childResult.buffers(), childResult.statsMin(), childResult.statsMax()); + return new EncodeResult(root, childResult.buffers(), null, null); } @Override @@ -59,13 +59,6 @@ public CascadeStep encodeCascade(DType dtype, Object data, EncodeContext ctx) { } EncodeNode partialRoot = new EncodeNode(EncodingId.VORTEX_EXT, null, new EncodeNode[1], new int[0]); ChildSlot slot = new ChildSlot(ext.storageDType(), data, 0); - // CascadingCompressor#spliceResult takes a step's stats verbatim -- it never derives them - // from a resolved open child -- so an open storage slot needs its stats computed here, - // independently of whatever encoding the cascade eventually picks for it (matching - // ZoneMapStatCodec#zoneMinMaxDtype: an Extension's zone-map min/max is its storage - // primitive's, unwrapped). - byte[][] stats = ext.storageDType() instanceof DType.Primitive p - ? PrimitiveEncodingEncoder.minMaxStats(p.ptype(), data) : null; - return CascadeStep.open(partialRoot, List.of(), List.of(slot), stats); + return new CascadeStep(partialRoot, List.of(), List.of(slot), null, null, true); } } diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/FsstEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/FsstEncodingEncoder.java index 5fbaf396..5b41bcdb 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/FsstEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/FsstEncodingEncoder.java @@ -79,11 +79,8 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { new EncodeNode[]{uncompLensNode, codesOffNode}, new int[]{0, 1, 2}); - // Zone-map min/max is lexicographic-string-only, matching VarBinEncodingEncoder: a - // Binary blob isn't usefully zone-mapped. - byte[][] stats = data instanceof String[] strings ? VarBinEncodingEncoder.minMaxStats(strings) : null; - return EncodeResult.of(root, - List.of(c.symBuf(), c.symLenBuf(), c.compBuf(), uncompLenBuf, codesOffBuf), stats); + return new EncodeResult(root, + List.of(c.symBuf(), c.symLenBuf(), c.compBuf(), uncompLenBuf, codesOffBuf), null, null); } /// Cascading FSST: expose the per-row uncompressed-length and code-offset children as open @@ -111,12 +108,11 @@ public CascadeStep encodeCascade(DType dtype, Object data, EncodeContext ctx) { MemorySegment.ofArray(c.metaBytes()), new EncodeNode[]{null, null}, new int[]{0, 1, 2}); - byte[][] stats = data instanceof String[] strings ? VarBinEncodingEncoder.minMaxStats(strings) : null; - return CascadeStep.open(partialRoot, + return new CascadeStep(partialRoot, List.of(c.symBuf(), c.symLenBuf(), c.compBuf()), List.of(new ChildSlot(new DType.Primitive(c.uncompLenPType(), false), uncompLens, 0), new ChildSlot(new DType.Primitive(c.codesOffPType(), false), codesOffsets, 1)), - stats); + null, null, true); } /// The FSST-specific product of compression: the symbol-table buffers, the wire code stream, the diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/PatchedEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/PatchedEncodingEncoder.java index 058173f6..72927873 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/PatchedEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/PatchedEncodingEncoder.java @@ -78,14 +78,14 @@ static CascadeStep encodeCascade(DType dtype, Object data) { DType u32Dtype = DType.U32; DType u16Dtype = DType.U16; - return CascadeStep.open(partialRoot, List.of(), + return new CascadeStep(partialRoot, List.of(), List.of( new ChildSlot(dtype, fromLongs(pd.inner, ptype), 0), new ChildSlot(u32Dtype, pd.laneOffsets, 1), new ChildSlot(u16Dtype, pd.patchIndices, 2), new ChildSlot(dtype, fromLongs(pd.patchValues, ptype), 3) ), - PrimitiveEncodingEncoder.minMaxStats(ptype, data)); + null, null, true); } static EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { @@ -133,8 +133,7 @@ static EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { EncodeNode root = new EncodeNode(EncodingId.VORTEX_PATCHED, MemorySegment.ofArray(metaBytes), new EncodeNode[]{innerNode, laneNode, idxNode, valNode}, new int[]{}); - return EncodeResult.of(root, List.of(innerBuf, laneOffsBuf, patchIdxBuf, patchValBuf), - PrimitiveEncodingEncoder.minMaxStats(ptype, data)); + return new EncodeResult(root, List.of(innerBuf, laneOffsBuf, patchIdxBuf, patchValBuf), null, null); } private static PatchedData computePatchedData(long[] longs, PType ptype, int n) { diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/PcoEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/PcoEncodingEncoder.java index 83460550..703934ea 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/PcoEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/PcoEncodingEncoder.java @@ -102,7 +102,7 @@ static EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { int[] allBufIdxs = IntStream.range(0, buffers.size()).toArray(); MemorySegment metaBuf = buildMetadata(chunks); EncodeNode node = new EncodeNode(EncodingId.VORTEX_PCO, metaBuf, new EncodeNode[0], allBufIdxs); - return EncodeResult.of(node, buffers, PrimitiveEncodingEncoder.minMaxStats(ptype, data)); + return new EncodeResult(node, buffers, null, null); } private static ChunkResult encodeChunk(long[] latents, PType ptype, int dtypeSize, Arena arena) { diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/RleEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/RleEncodingEncoder.java index 95e535e4..911381c5 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/RleEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/RleEncodingEncoder.java @@ -183,8 +183,7 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { MemorySegment.ofArray(metaBytes), new EncodeNode[]{valuesNode, indicesNode, offsetsNode}, new int[0]); - return EncodeResult.of(root, List.of(valuesSeg, indicesSeg, offsetsSeg), - PrimitiveEncodingEncoder.minMaxStats(ptype, data)); + return new EncodeResult(root, List.of(valuesSeg, indicesSeg, offsetsSeg), null, null); } private static int rleEncode(long[] input, long[] chunkValues, short[] chunkIndices) { diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/SparseEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/SparseEncodingEncoder.java index a0000865..7cf3d14b 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/SparseEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/SparseEncodingEncoder.java @@ -117,8 +117,7 @@ public CascadeStep encodeCascade(DType dtype, Object data, EncodeContext ctx) { DType idxDtype = new DType.Primitive(idxPtype, false); ChildSlot idxSlot = new ChildSlot(idxDtype, idxArr, 0); ChildSlot valSlot = new ChildSlot(dtype, valArr, 1); - return CascadeStep.open(partialRoot, List.of(fillBuf), List.of(idxSlot, valSlot), - PrimitiveEncodingEncoder.minMaxStats(ptype, data)); + return new CascadeStep(partialRoot, List.of(fillBuf), List.of(idxSlot, valSlot), null, null, true); } private static Object idxArr(List patchIdx, PType idxPtype) { @@ -324,8 +323,7 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { EncodeNode valNode = EncodeNode.leaf(EncodingId.VORTEX_PRIMITIVE, 2); EncodeNode root = new EncodeNode(EncodingId.VORTEX_SPARSE, MemorySegment.ofArray(metaBytes), new EncodeNode[]{idxNode, valNode}, new int[]{0}); - return EncodeResult.of(root, List.of(fillBuf, idxBuf, valBuf), - PrimitiveEncodingEncoder.minMaxStats(ptype, data)); + return new EncodeResult(root, List.of(fillBuf, idxBuf, valBuf), null, null); } private static int arrayLength(Object data, PType ptype) { diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/VarBinViewEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/VarBinViewEncodingEncoder.java index 2408c9ae..0982ffa3 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/VarBinViewEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/VarBinViewEncodingEncoder.java @@ -68,9 +68,6 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { } EncodeNode root = new EncodeNode(EncodingId.VORTEX_VARBINVIEW, null, new EncodeNode[0], bufIndices); - // Zone-map min/max is lexicographic-string-only, matching VarBinEncodingEncoder: a - // Binary blob isn't usefully zone-mapped. - byte[][] stats = data instanceof String[] strings ? VarBinEncodingEncoder.minMaxStats(strings) : null; - return EncodeResult.of(root, buffers, stats); + return new EncodeResult(root, buffers, null, null); } } diff --git a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ZstdEncodingEncoder.java b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ZstdEncodingEncoder.java index 0d9fd92d..c7351a0e 100644 --- a/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ZstdEncodingEncoder.java +++ b/writer/src/main/java/io/github/dfa1/vortex/writer/encode/ZstdEncodingEncoder.java @@ -5,7 +5,6 @@ import io.github.dfa1.vortex.core.model.PType; import io.github.dfa1.vortex.core.error.VortexException; import io.github.dfa1.vortex.core.model.EncodingId; -import io.github.dfa1.vortex.core.compute.PrimitiveArrays; import io.github.dfa1.vortex.core.io.VortexFormat; import io.github.dfa1.vortex.core.proto.ProtoZstdFrameMetadata; import io.github.dfa1.vortex.core.proto.ProtoZstdMetadata; @@ -116,10 +115,7 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) { throw new VortexException(EncodingId.VORTEX_ZSTD, "non-nullable " + dtype + " contains null"); } - // Zone-map min/max is lexicographic-string-only, matching VarBinEncodingEncoder: a - // Binary blob (e.g. audio bytes) isn't usefully zone-mapped. - byte[][] stats = data instanceof String[] strings ? VarBinEncodingEncoder.minMaxStats(strings) : null; - return encodeVarBin(encoded, ctx.arena(), stats); + return encodeVarBin(encoded, ctx.arena()); } throw new VortexException(EncodingId.VORTEX_ZSTD, "unsupported dtype: " + dtype); } @@ -128,22 +124,21 @@ private EncodeResult encodePrimitive(DType.Primitive dt, Object data, Arena aren int byteWidth = dt.ptype().byteSize(); MemorySegment raw = primitiveToLeBytes(dt.ptype(), data, arena); long n = primitiveLength(dt.ptype(), data); - byte[][] stats = PrimitiveEncodingEncoder.minMaxStats(dt.ptype(), data); - return buildResult(raw, uniformLayout(n, byteWidth), arena, stats); + return buildResult(raw, uniformLayout(n, byteWidth), arena); } - private EncodeResult encodeVarBin(byte[][] encoded, Arena arena, byte[][] stats) { + private EncodeResult encodeVarBin(byte[][] encoded, Arena arena) { MemorySegment raw = buildLengthPrefixed(encoded, arena); - return buildResult(raw, varBinLayout(raw, encoded.length), arena, stats); + return buildResult(raw, varBinLayout(raw, encoded.length), arena); } - private EncodeResult buildResult(MemorySegment raw, FrameLayout layout, Arena arena, byte[][] stats) { + private EncodeResult buildResult(MemorySegment raw, FrameLayout layout, Arena arena) { // Zero-copy: each frame is an arena-native slice of raw, compressed straight into another // arena segment. A single-value-per-array config yields one frame (the prior behavior). Frames frames = compressFrames(raw, layout, arena); EncodeNode root = new EncodeNode(EncodingId.VORTEX_ZSTD, MemorySegment.ofArray(frames.metadata()), new EncodeNode[0], frameBufferIndices(frames.compressed().size(), 0)); - return EncodeResult.of(root, List.copyOf(frames.compressed()), stats); + return new EncodeResult(root, List.copyOf(frames.compressed()), null, null); } private EncodeResult encodeNullablePrimitive(DType.Primitive dt, NullableData nd, EncodeContext ctx) { @@ -154,11 +149,7 @@ private EncodeResult encodeNullablePrimitive(DType.Primitive dt, NullableData nd // reference). The decoder scatters them back over the validity mask carried by child[0]. MemorySegment full = primitiveToLeBytes(dt.ptype(), nd.values(), arena); MemorySegment packed = packValidBytes(full, validity, byteWidth, arena); - // Stats must come from only the valid elements -- the dense values array carries - // placeholder garbage (commonly 0) at invalid positions, same as MaskedEncodingEncoder. - Object compacted = PrimitiveArrays.compact(dt.ptype(), nd.values(), validity); - byte[][] stats = PrimitiveEncodingEncoder.minMaxStats(dt.ptype(), compacted); - return buildNullableResult(packed, uniformLayout(countValid(validity), byteWidth), validity, ctx, stats); + return buildNullableResult(packed, uniformLayout(countValid(validity), byteWidth), validity, ctx); } private EncodeResult encodeNullableVarBin(NullableData nd, EncodeContext ctx) { @@ -166,13 +157,11 @@ private EncodeResult encodeNullableVarBin(NullableData nd, EncodeContext ctx) { // reference). The decoder scatters them back over the validity mask carried by child[0]. byte[][] valid = stripNulls(VarBinBytes.toRawByteArrays(nd.values())); MemorySegment packed = buildLengthPrefixed(valid, ctx.arena()); - // minMaxStats already skips null entries itself, so the un-stripped values array is fine. - byte[][] stats = nd.values() instanceof String[] strings ? VarBinEncodingEncoder.minMaxStats(strings) : null; - return buildNullableResult(packed, varBinLayout(packed, valid.length), nd.validity(), ctx, stats); + return buildNullableResult(packed, varBinLayout(packed, valid.length), nd.validity(), ctx); } private EncodeResult buildNullableResult( - MemorySegment raw, FrameLayout layout, boolean[] validity, EncodeContext ctx, byte[][] stats) { + MemorySegment raw, FrameLayout layout, boolean[] validity, EncodeContext ctx) { Frames frames = compressFrames(raw, layout, ctx.arena()); int frameCount = frames.compressed().size(); @@ -187,7 +176,7 @@ private EncodeResult buildNullableResult( EncodeNode root = new EncodeNode(EncodingId.VORTEX_ZSTD, MemorySegment.ofArray(frames.metadata()), new EncodeNode[]{validityNode}, frameBufferIndices(frameCount, 0)); - return EncodeResult.of(root, buffers, stats); + return new EncodeResult(root, buffers, null, null); } /// Byte spans and value counts of each frame; spans sum to the payload size. diff --git a/writer/src/test/java/io/github/dfa1/vortex/writer/ZoneMapStatCodecTest.java b/writer/src/test/java/io/github/dfa1/vortex/writer/ZoneMapStatCodecTest.java new file mode 100644 index 00000000..b9a51204 --- /dev/null +++ b/writer/src/test/java/io/github/dfa1/vortex/writer/ZoneMapStatCodecTest.java @@ -0,0 +1,218 @@ +package io.github.dfa1.vortex.writer; + +import io.github.dfa1.vortex.core.model.DType; +import io.github.dfa1.vortex.core.model.PType; +import io.github.dfa1.vortex.core.proto.ProtoScalarValue; +import io.github.dfa1.vortex.core.testing.DTypes; +import io.github.dfa1.vortex.writer.encode.ComparableValues; +import io.github.dfa1.vortex.writer.encode.NullableData; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.lang.foreign.MemorySegment; + +import static org.assertj.core.api.Assertions.assertThat; + +/// [ZoneMapStatCodec#columnMinMax] is the generic fallback [VortexWriter#writeSegment] calls +/// whenever the winning encoder's own [io.github.dfa1.vortex.writer.encode.EncodeResult] didn't +/// already supply stats -- the fix for the whole class of bug where an individual encoder simply +/// forgot to (#382/#384-#386 and ten more, ADR 0025). Since encoders are no longer *required* to +/// report their own stats, this is the actual place the guarantee now lives, so it gets direct +/// coverage over every dtype shape it dispatches on, independent of which specific encoder a +/// cascade might pick for any of them. +class ZoneMapStatCodecTest { + + @Nested + class Primitive { + + @Test + void signed_reportsRealMinMax() throws IOException { + // Given + long[] data = {10L, -5L, 3L, 7L}; + + // When + byte[][] stats = ZoneMapStatCodec.columnMinMax(DTypes.I64, data); + + // Then + assertThat(scalar(stats[0]).int64_value()).isEqualTo(-5L); + assertThat(scalar(stats[1]).int64_value()).isEqualTo(10L); + } + + @Test + void unsigned_usesUnsignedComparison() throws IOException { + // Given -- -1's raw bits are the largest possible U32 magnitude, not the smallest + int[] data = {1, -1, 5}; + + // When + byte[][] stats = ZoneMapStatCodec.columnMinMax(DTypes.U32, data); + + // Then + assertThat(scalar(stats[0]).uint64_value()).isEqualTo(1L); + assertThat(scalar(stats[1]).uint64_value()).isEqualTo(4294967295L); + } + + @Test + void nullable_compactsToValidElementsOnly() throws IOException { + // Given -- the #381 bug this generalizes: invalid slots carry a placeholder (here 0), + // which is not a min/max identity and must not corrupt the reported extremes + long[] values = {0L, 100L, 0L, -50L}; + boolean[] validity = {false, true, false, true}; + NullableData data = new NullableData(values, validity); + + // When + byte[][] stats = ZoneMapStatCodec.columnMinMax(DTypes.I64.asNullable(), data); + + // Then + assertThat(scalar(stats[0]).int64_value()).isEqualTo(-50L); + assertThat(scalar(stats[1]).int64_value()).isEqualTo(100L); + } + + @Test + void empty_returnsNull() { + // Given + long[] data = {}; + + // When + byte[][] stats = ZoneMapStatCodec.columnMinMax(DTypes.I64, data); + + // Then + assertThat(stats).isNull(); + } + } + + @Nested + class ExtensionType { + + private static final DType TIMESTAMP_MS = + new DType.Extension("vortex.timestamp", DType.I64, MemorySegment.ofArray(new byte[]{0, 0, 0}), false); + + @Test + void plainPrimitiveStorage_unwrapsToStoragePrimitive() throws IOException { + // Given -- an extension whose data is a bare primitive array, not a special carrier + long[] data = {100L, 200L, 50L}; + + // When + byte[][] stats = ZoneMapStatCodec.columnMinMax(TIMESTAMP_MS, data); + + // Then + assertThat(scalar(stats[0]).int64_value()).isEqualTo(50L); + assertThat(scalar(stats[1]).int64_value()).isEqualTo(200L); + } + + @Test + void comparableValuesCarrier_usesItsOwnComparableForm() throws IOException { + // Given -- a carrier whose literal shape isn't a plain primitive array (like + // DateTimePartsData splitting a timestamp into day/second/subsecond parts); its + // *comparable* form is what ComparableValues exposes, not the carrier itself + record FakeCarrier(long[] comparable) implements ComparableValues { + @Override + public PType ptype() { + return PType.I64; + } + + @Override + public Object values() { + return comparable; + } + } + FakeCarrier carrier = new FakeCarrier(new long[]{9L, 1L, 5L}); + + // When + byte[][] stats = ZoneMapStatCodec.columnMinMax(TIMESTAMP_MS, carrier); + + // Then + assertThat(scalar(stats[0]).int64_value()).isEqualTo(1L); + assertThat(scalar(stats[1]).int64_value()).isEqualTo(9L); + } + + @Test + void nullablePlainStorage_compactsToValidElementsOnly() throws IOException { + // Given + long[] values = {0L, 300L, 0L}; + boolean[] validity = {false, true, false}; + NullableData data = new NullableData(values, validity); + + // When + byte[][] stats = ZoneMapStatCodec.columnMinMax(TIMESTAMP_MS, data); + + // Then + assertThat(scalar(stats[0]).int64_value()).isEqualTo(300L); + assertThat(scalar(stats[1]).int64_value()).isEqualTo(300L); + } + } + + @Nested + class Utf8Type { + + @Test + void reportsLexicographicMinMax() throws IOException { + // Given + String[] data = {"banana", "apple", "cherry"}; + + // When + byte[][] stats = ZoneMapStatCodec.columnMinMax(DTypes.UTF8, data); + + // Then + assertThat(scalar(stats[0]).string_value()).isEqualTo("apple"); + assertThat(scalar(stats[1]).string_value()).isEqualTo("cherry"); + } + + @Test + void nullable_skipsNullEntriesDirectly() throws IOException { + // Given -- Utf8's NullableData carries actual null array entries, not a placeholder + // scheme, so no compaction step is needed before the lexicographic scan + String[] values = {null, "banana", null, "apple"}; + NullableData data = new NullableData(values, new boolean[]{false, true, false, true}); + + // When + byte[][] stats = ZoneMapStatCodec.columnMinMax(DTypes.UTF8.asNullable(), data); + + // Then + assertThat(scalar(stats[0]).string_value()).isEqualTo("apple"); + assertThat(scalar(stats[1]).string_value()).isEqualTo("banana"); + } + } + + @Nested + class NotEligible { + + @Test + void binary_returnsNull() { + // Given -- min/max is lexicographic-string-only; a binary blob isn't zone-mapped + byte[][] data = {{1, 2}, {3, 4}}; + + // When / Then + assertThat(ZoneMapStatCodec.columnMinMax(DTypes.BINARY, data)).isNull(); + } + + @Test + void decimal_returnsNull() { + // Given -- Decimal is not in ZoneMapStatCodec#zoneMinMaxDtype's supported set + DType decimal = new DType.Decimal((byte) 10, (byte) 2, false); + + // When / Then + assertThat(ZoneMapStatCodec.columnMinMax(decimal, new long[]{100L})).isNull(); + } + + @Test + void bool_returnsNull() { + // When / Then + assertThat(ZoneMapStatCodec.columnMinMax(DTypes.BOOL, new boolean[]{true, false})).isNull(); + } + + @Test + void structuralType_returnsNull() { + // Given + DType list = new DType.List(DTypes.I64, false); + + // When / Then + assertThat(ZoneMapStatCodec.columnMinMax(list, new Object[0])).isNull(); + } + } + + private static ProtoScalarValue scalar(byte[] bytes) throws IOException { + MemorySegment seg = MemorySegment.ofArray(bytes); + return ProtoScalarValue.decode(seg, 0, seg.byteSize()); + } +} diff --git a/writer/src/test/java/io/github/dfa1/vortex/writer/encode/SequenceEncodingEncoderTest.java b/writer/src/test/java/io/github/dfa1/vortex/writer/encode/SequenceEncodingEncoderTest.java index 1b7c464b..09682728 100644 --- a/writer/src/test/java/io/github/dfa1/vortex/writer/encode/SequenceEncodingEncoderTest.java +++ b/writer/src/test/java/io/github/dfa1/vortex/writer/encode/SequenceEncodingEncoderTest.java @@ -453,4 +453,68 @@ void encode_i64_metadata_base_andMultiplier_areSet() throws Exception { assertThat(meta.multiplier().int64_value()).isEqualTo(2L); } } + + /// A perfect arithmetic sequence is monotonic (or constant) end to end, so its extremes are + /// always the first and last element -- kept as a fast-path override (ADR 0025) instead of + /// relying on VortexWriter's generic fallback, which would otherwise rescan the array. + @Nested + class Stats { + + @Test + void encode_i64_increasing_reportsFirstAsMinLastAsMax() throws java.io.IOException { + // Given + long[] data = {10L, 20L, 30L, 40L}; + + // When + EncodeResult result = ENCODER.encode(DTypes.I64, data, EncodeTestHelper.testCtx()); + + // Then + assertThat(scalar(result.statsMin()).int64_value()).isEqualTo(10L); + assertThat(scalar(result.statsMax()).int64_value()).isEqualTo(40L); + } + + @Test + void encode_i64_decreasing_reportsLastAsMinFirstAsMax() throws java.io.IOException { + // Given -- a negative multiplier means the extremes sit at the OPPOSITE endpoints + long[] data = {40L, 30L, 20L, 10L}; + + // When + EncodeResult result = ENCODER.encode(DTypes.I64, data, EncodeTestHelper.testCtx()); + + // Then + assertThat(scalar(result.statsMin()).int64_value()).isEqualTo(10L); + assertThat(scalar(result.statsMax()).int64_value()).isEqualTo(40L); + } + + @Test + void encode_f64_reportsEndpointsAsMinMax() throws java.io.IOException { + // Given + double[] data = {1.5, 3.0, 4.5, 6.0}; + + // When + EncodeResult result = ENCODER.encode(DTypes.F64, data, EncodeTestHelper.testCtx()); + + // Then + assertThat(scalar(result.statsMin()).f64_value()).isEqualTo(1.5); + assertThat(scalar(result.statsMax()).f64_value()).isEqualTo(6.0); + } + + @Test + void encode_empty_statsAreNull() { + // Given + long[] data = {}; + + // When + EncodeResult result = ENCODER.encode(DTypes.I64, data, EncodeTestHelper.testCtx()); + + // Then + assertThat(result.statsMin()).isNull(); + assertThat(result.statsMax()).isNull(); + } + + private static ProtoScalarValue scalar(byte[] bytes) throws java.io.IOException { + MemorySegment seg = MemorySegment.ofArray(bytes); + return ProtoScalarValue.decode(seg, 0, seg.byteSize()); + } + } } diff --git a/writer/src/test/java/io/github/dfa1/vortex/writer/encode/ZoneMapStatsCoverageTest.java b/writer/src/test/java/io/github/dfa1/vortex/writer/encode/ZoneMapStatsCoverageTest.java deleted file mode 100644 index 22b832fd..00000000 --- a/writer/src/test/java/io/github/dfa1/vortex/writer/encode/ZoneMapStatsCoverageTest.java +++ /dev/null @@ -1,116 +0,0 @@ -package io.github.dfa1.vortex.writer.encode; - -import io.github.dfa1.vortex.core.model.DType; -import io.github.dfa1.vortex.core.model.TimeUnit; -import io.github.dfa1.vortex.core.testing.DTypes; -import io.github.dfa1.vortex.writer.WriteRegistry; - -import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.Arguments; -import org.junit.jupiter.params.provider.MethodSource; - -import java.lang.foreign.MemorySegment; -import java.util.HashSet; -import java.util.Set; -import java.util.stream.Stream; - -import static org.assertj.core.api.Assertions.assertThat; - -/// Fitness function for #382/#384/#385/#386 and the sibling bugs found in the same audit -/// (`Pco`/`Rle`/`Sparse`/`Patched`/`Zstd`/`Fsst`/`VarBinView`/`Ext`/`DateTimeParts`/`Sequence`): -/// every registered [EncodingEncoder] that can encode a `Primitive`/`Extension`/`Utf8` value must -/// report zone-map `MIN`/`MAX` stats for representative non-empty input, or `RowFilter` pruning -/// silently no-ops for any column that encoder wins. -/// -/// Two checks: -/// - [#everyStatsEligibleDefaultEncoder_hasACoverageCase] — registry-driven: every default encoder -/// whose [EncodingEncoder#accepts] matches one of a few representative dtypes must appear in -/// [#coverageCases]. Add an encoder capable of `Primitive`/`Extension`/`Utf8` without adding a -/// case here and this fails — the whole point, so the next such bug can't land silently. -/// - [#encode_reportsMinMaxStats] — the actual per-encoder assertion, run over every case. -/// -/// Deliberately excluded (no zone-map `MIN`/`MAX` concept at all, per -/// `ZoneMapStatCodec#zoneMinMaxDtype`): `Decimal`/`DecimalByteParts` (dtype `Decimal`, not in the -/// codec's supported set), `Bool`/`ByteBool` (dtype `Bool`), every structural/collection encoder -/// (`Chunked`, `FixedSizeList`, `List`, `ListView`, `Map`, `Null`, `Struct`, `Variant`) — none -/// accept a `Primitive`/`Extension`/`Utf8` dtype, so they never surface via the registry probe -/// below. `MaskedEncodingEncoder` is excluded too: its `accepts()` is unconditionally `false` (it -/// is special-dispatched for nullable columns, never registry-selected), so it cannot appear via -/// this probe either — its own stats correctness (#381) is covered by its dedicated test class. -class ZoneMapStatsCoverageTest { - - private static final DType TIMESTAMP_MS = new DType.Extension( - "vortex.timestamp", DType.I64, MemorySegment.ofArray(new byte[]{(byte) TimeUnit.Milliseconds.ordinal(), 0, 0}), false); - - /// Representative dtypes to probe every default-registered encoder's [EncodingEncoder#accepts] - /// with. Deliberately not exhaustive over every `PType` -- just enough to surface every - /// stats-eligible encoder class at least once (an integer, a float, a string, an extension). - private static final DType[] PROBE_DTYPES = {DTypes.I64, DTypes.F64, DTypes.UTF8, TIMESTAMP_MS}; - - @Test - void everyStatsEligibleDefaultEncoder_hasACoverageCase() { - // Given -- every encoder the default registry would actually select - WriteRegistry registry = WriteRegistry.builder().registerDefaults().build(); - - // When -- narrowed to those that can encode at least one representative comparable dtype - Set> statsEligible = new HashSet<>(); - for (EncodingEncoder encoder : registry.encoderMap().values()) { - for (DType probe : PROBE_DTYPES) { - if (encoder.accepts(probe)) { - statsEligible.add(encoder.getClass()); - break; - } - } - } - Set> covered = coverageCases().map(a -> a.get()[1].getClass()).collect(java.util.stream.Collectors.toSet()); - - // Then -- every stats-eligible encoder has a coverage case (new encoder + no case here = failure) - assertThat(covered).containsAll(statsEligible); - } - - @ParameterizedTest(name = "{0}") - @MethodSource("coverageCases") - void encode_reportsMinMaxStats(String label, EncodingEncoder encoder, DType dtype, Object data) { - // Given / When - EncodeResult result = encoder.encode(dtype, data, EncodeTestHelper.testCtx()); - - // Then - assertThat(result.hasStats()).as(label).isTrue(); - } - - static Stream coverageCases() { - return Stream.of( - Arguments.of("Alp/f64", new AlpEncodingEncoder(), DTypes.F64, new double[]{1.1, 2.2, 3.3, 4.4}), - Arguments.of("AlpRd/f64", new AlpRdEncodingEncoder(), DTypes.F64, new double[]{0.5, -3.25, 10.0, 2.0}), - Arguments.of("Bitpacked/i64", new BitpackedEncodingEncoder(), DTypes.I64, new long[]{1L, 2L, 3L, 4L, 5L}), - Arguments.of("Constant/i64", new ConstantEncodingEncoder(), DTypes.I64, new long[]{7L, 7L, 7L}), - Arguments.of("DateTimeParts/timestamp", new DateTimePartsEncodingEncoder(), TIMESTAMP_MS, - new DateTimePartsData(new long[]{1_700_000_000_000L, 1_700_000_100_000L, 1_699_999_900_000L}, false)), - Arguments.of("Delta/i64", new DeltaEncodingEncoder(), DTypes.I64, new long[]{10L, 20L, 15L, 30L}), - Arguments.of("Dict/i32", new DictEncodingEncoder(), DTypes.I32, new int[]{1, 1, 2, 2, 3}), - Arguments.of("Ext/timestamp", new ExtEncodingEncoder(), TIMESTAMP_MS, new long[]{100L, 200L, 300L}), - Arguments.of("FrameOfReference/i64", new FrameOfReferenceEncodingEncoder(), DTypes.I64, new long[]{1000L, 1001L, 1002L, 1003L}), - Arguments.of("Fsst/utf8", new FsstEncodingEncoder(), DTypes.UTF8, new String[]{"hello", "world", "hello"}), - Arguments.of("Patched/i32", new PatchedEncodingEncoder(), DTypes.I32, new int[]{1, 2, 3, 4, 1_000_000}), - Arguments.of("Pco/i64", new PcoEncodingEncoder(), DTypes.I64, longRange(0, 4096)), - Arguments.of("Primitive/i32", new PrimitiveEncodingEncoder(), DTypes.I32, new int[]{1, 2, 3}), - Arguments.of("Rle/i32", new RleEncodingEncoder(), DTypes.I32, new int[]{1, 1, 2, 2, 3, 3}), - Arguments.of("RunEnd/i64", new RunEndEncodingEncoder(), DTypes.I64, new long[]{1L, 1L, 2L, 2L, 3L}), - Arguments.of("Sequence/i64", new SequenceEncodingEncoder(), DTypes.I64, new long[]{10L, 20L, 30L, 40L}), - Arguments.of("Sparse/i32", new SparseEncodingEncoder(), DTypes.I32, new int[]{0, 0, 5, 0, 0}), - Arguments.of("VarBin/utf8", new VarBinEncodingEncoder(), DTypes.UTF8, new String[]{"apple", "banana"}), - Arguments.of("VarBinView/utf8", new VarBinViewEncodingEncoder(), DTypes.UTF8, new String[]{"apple", "banana"}), - Arguments.of("ZigZag/i32", new ZigZagEncodingEncoder(), DTypes.I32, new int[]{-1, 1, -2, 2}), - Arguments.of("Zstd/i64", new ZstdEncodingEncoder(), DTypes.I64, new long[]{1L, 2L, 3L, 4L, 5L}) - ); - } - - private static long[] longRange(long start, int n) { - long[] a = new long[n]; - for (int i = 0; i < n; i++) { - a[i] = start + i; - } - return a; - } -}