From 5aea5de5a8fcef774b2dfd9e46284890c20e69b9 Mon Sep 17 00:00:00 2001 From: Davide Angelocola Date: Thu, 3 Sep 2026 19:07:22 +0200 Subject: [PATCH] fix: resolve SonarCloud BLOCKER findings and a safe slice of CRITICAL S1192 Scope: every BLOCKER-severity finding, plus the CRITICAL S1192 (duplicated string literal) findings in production code -- excluding the code-generator modules (fbs-gen/proto-gen), which are an interim in-house toolchain not worth polishing further (see adr/0017-in-house-fbs-proto-codegen and the project's own generated-code disclaimer). CRITICAL S3776 (cognitive complexity) findings are deliberately NOT touched here: ~85 of them, several in hot-path decode/encode code with complexity in the 40s-60s (BitpackedEncodingDecoder, PcoEncodingDecoder, DictFilter, PrimitiveEncodingEncoder) -- these need careful, individually-reviewed refactors with benchmarks, not a blind autonomous sweep, and are a natural follow-up PR. S115 (TimeUnit's PascalCase enum constants) is also left alone: renaming a public enum used throughout the codebase and by downstream consumers is a breaking API change that needs an explicit decision, not something to fold into a lint-cleanup PR. BLOCKER fixes: - fsst: LossyPerfectHashTable and ShortCodeTable both had a private instance field named `slots` differing only in case from the `SLOTS` size constant (S1845). Renamed the field to `table` in both (encapsulated, no external API, hot-path logic untouched -- pure identifier rename, verified against the full fsst test suite). - Two Raincloud corpus tests' `corpusIsHydrated()` (S2699, "add an assertion") were flagged for having no assertion, but assumeTrue *is* the check here -- a visible skip marker when the corpus isn't hydrated, not a pass/fail assertion, and there's nothing else to assert. Suppressed with a comment explaining why, rather than inventing a no-op assertion just to satisfy the rule. - CalciteDemo.profileFullScan (S2699) is a profiling harness with a real gap: it printed a row count but never checked it against what was written. Added a genuine assertion (count == rows written) -- cheap, doesn't interfere with profiling, and catches a broken scan before trusting the profile it produces. CRITICAL S1192 fixes (production code only): - SparseEncodingDecoder: "indices"/"values" role literals (used in 15 call sites combined) extracted to ROLE_INDICES/ROLE_VALUES. - Predicate: the six single-value leaf records (Eq/Neq/Lt/Gt/Lte/Gte) each null-check their `value` component against the same literal; extracted to a shared VALUE constant on the sealed interface. The two-component leaves (Between/And/Or) were already below the duplication threshold and are left as split literals ("lo"/"hi", "left"/"right") -- clearer than a single ambiguous shared constant. - VortexInspectorTui: three DataState-rendering blocks (dictionary preview, per-chunk stats, column data) repeated the same "loading" suffix, numbered-entry format, and column-data label prefix; extracted to LOADING_SUFFIX/ENTRY_FORMAT/DATA_COLUMN_PREFIX. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LjU7zWghUiJauRxK58nyUs --- .../dfa1/vortex/calcite/CalciteDemo.java | 4 +++ .../vortex/cli/tui/VortexInspectorTui.java | 27 +++++++++----- .../vortex/fsst/LossyPerfectHashTable.java | 24 ++++++------- .../dfa1/vortex/fsst/ShortCodeTable.java | 20 +++++------ .../RaincloudConformanceIntegrationTest.java | 2 ++ ...aincloudSizeComparisonIntegrationTest.java | 2 ++ .../dfa1/vortex/reader/compute/Predicate.java | 16 +++++---- .../reader/decode/SparseEncodingDecoder.java | 36 +++++++++++-------- 8 files changed, 79 insertions(+), 52 deletions(-) diff --git a/calcite/src/test/java/io/github/dfa1/vortex/calcite/CalciteDemo.java b/calcite/src/test/java/io/github/dfa1/vortex/calcite/CalciteDemo.java index 7ac52b53c..b8613ae01 100644 --- a/calcite/src/test/java/io/github/dfa1/vortex/calcite/CalciteDemo.java +++ b/calcite/src/test/java/io/github/dfa1/vortex/calcite/CalciteDemo.java @@ -3,6 +3,8 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.EnabledIfSystemProperty; +import static org.assertj.core.api.Assertions.assertThat; + import java.nio.file.Files; import java.nio.file.Path; import java.sql.Connection; @@ -53,6 +55,8 @@ void profileFullScan() throws Exception { } double ms = (System.nanoTime() - t0) / 1e6 / iterations; System.out.printf("full scan x%d: %.2f ms/query | count=%,d%n", iterations, ms, count); + // A wrong count here means the profile below is timing a broken scan. + assertThat(count).isEqualTo(rows); } } finally { Files.deleteIfExists(file); diff --git a/cli/src/main/java/io/github/dfa1/vortex/cli/tui/VortexInspectorTui.java b/cli/src/main/java/io/github/dfa1/vortex/cli/tui/VortexInspectorTui.java index 2d180a6c9..42a71f834 100644 --- a/cli/src/main/java/io/github/dfa1/vortex/cli/tui/VortexInspectorTui.java +++ b/cli/src/main/java/io/github/dfa1/vortex/cli/tui/VortexInspectorTui.java @@ -113,6 +113,15 @@ private static final class Loop { /// ASCII spinner frames; cycled by render tick. private static final char[] SPINNER = {'|', '/', '-', '\\'}; + /// Suffix appended after the spinner frame while a background fetch is still pending. + private static final String LOADING_SUFFIX = " loading..."; + + /// Format for one numbered preview line (dictionary entry, zone-stats row, or data row). + private static final String ENTRY_FORMAT = " [%2d] %s"; + + /// Label prefix for the selected column's data preview. + private static final String DATA_COLUMN_PREFIX = "Data (column '"; + private final Terminal term; private final InspectorTree tree; private final VortexHandle handle; @@ -508,13 +517,13 @@ private List detailLines(InspectorTree.Node node) { lines.add(""); switch (dictState) { case DataState.Pending _ -> - lines.add("Dictionary: " + SPINNER[(int) (tick % SPINNER.length)] + " loading..."); + lines.add("Dictionary: " + SPINNER[(int) (tick % SPINNER.length)] + LOADING_SUFFIX); case DataState.Failed(String msg) -> lines.add("Dictionary: ! " + msg); case DataState.Loaded(List values) -> { lines.add("Dictionary (" + values.size() + " entries):"); for (int i = 0; i < values.size(); i++) { - lines.add(String.format(" [%2d] %s", i, values.get(i))); + lines.add(String.format(ENTRY_FORMAT, i, values.get(i))); } } } @@ -529,13 +538,13 @@ private List detailLines(InspectorTree.Node node) { switch (zoneState) { case DataState.Pending _ -> lines.add("Per-chunk stats: " - + SPINNER[(int) (tick % SPINNER.length)] + " loading..."); + + SPINNER[(int) (tick % SPINNER.length)] + LOADING_SUFFIX); case DataState.Failed(String msg) -> lines.add("Per-chunk stats: ! " + msg); case DataState.Loaded(List rows) -> { lines.add("Per-chunk stats (" + rows.size() + " chunks):"); for (int i = 0; i < rows.size(); i++) { - lines.add(String.format(" [%2d] %s", i, rows.get(i))); + lines.add(String.format(ENTRY_FORMAT, i, rows.get(i))); } } } @@ -545,14 +554,14 @@ private List detailLines(InspectorTree.Node node) { lines.add(""); switch (state) { case DataState.Pending _ -> - lines.add("Data (column '" + col + "'): " - + SPINNER[(int) (tick % SPINNER.length)] + " loading..."); + lines.add(DATA_COLUMN_PREFIX + col + "'): " + + SPINNER[(int) (tick % SPINNER.length)] + LOADING_SUFFIX); case DataState.Failed(String msg) -> - lines.add("Data (column '" + col + "'): ! " + msg); + lines.add(DATA_COLUMN_PREFIX + col + "'): ! " + msg); case DataState.Loaded(List values) -> { - lines.add("Data (column '" + col + "', first " + values.size() + " rows):"); + lines.add(DATA_COLUMN_PREFIX + col + "', first " + values.size() + " rows):"); for (int i = 0; i < values.size(); i++) { - lines.add(String.format(" [%2d] %s", i, values.get(i))); + lines.add(String.format(ENTRY_FORMAT, i, values.get(i))); } } } diff --git a/fsst/src/main/java/io/github/dfa1/vortex/fsst/LossyPerfectHashTable.java b/fsst/src/main/java/io/github/dfa1/vortex/fsst/LossyPerfectHashTable.java index 91c8917d7..9b3c58485 100644 --- a/fsst/src/main/java/io/github/dfa1/vortex/fsst/LossyPerfectHashTable.java +++ b/fsst/src/main/java/io/github/dfa1/vortex/fsst/LossyPerfectHashTable.java @@ -35,8 +35,8 @@ final class LossyPerfectHashTable { /// Low three bytes of the input word — the only bytes the hash keys on. private static final long PREFIX_MASK = 0x00FF_FFFFL; - /// Slot table with two adjacent longs per slot (the FSST paper's C layout): `slots[2 * s]` is - /// the candidate's packed symbol bytes (LSB-first, [Symbol] convention) and `slots[2 * s + 1]` + /// Slot table with two adjacent longs per slot (the FSST paper's C layout): `table[2 * s]` is + /// the candidate's packed symbol bytes (LSB-first, [Symbol] convention) and `table[2 * s + 1]` /// is its metadata `ignoredBits << 16 | code << 8 | length` (`ignoredBits = 64 - 8 * length`). /// A lookup derives the keep-mask from the ignored-bits with one shift instead of loading a /// separate mask array, so the whole 16-byte slot lands in a single cache line — one memory @@ -46,10 +46,10 @@ final class LossyPerfectHashTable { /// input word of exactly 0 can "hit" an empty slot — harmlessly, because the returned low 16 /// bits (`code << 8 | length`) are then 0, which is precisely the "no match" answer. No /// occupancy flag or sentinel is needed. - private final long[] slots; + private final long[] table; - private LossyPerfectHashTable(long[] slots) { - this.slots = slots; + private LossyPerfectHashTable(long[] table) { + this.table = table; } /// Builds the table from the trained symbols in descending-gain order, keeping only those of @@ -67,21 +67,21 @@ private LossyPerfectHashTable(long[] slots) { /// @param symbolsByGainDescending the trained symbols, code = list index, gain-descending /// @return a hash table resolving 3-8 byte matches with first-writer-wins on collision static LossyPerfectHashTable of(List symbolsByGainDescending) { - long[] slots = new long[2 * SLOTS]; + long[] table = new long[2 * SLOTS]; for (int code = 0; code < symbolsByGainDescending.size(); code++) { Symbol symbol = symbolsByGainDescending.get(code); if (symbol.length() < 3) { continue; // Length 1-2 belongs to ShortCodeTable. } int slot = slotFor(symbol.packedBytes()); - if (slots[2 * slot + 1] != 0) { + if (table[2 * slot + 1] != 0) { continue; // First writer (higher gain) wins; skip the collision. } long ignoredBits = 64L - 8 * symbol.length(); - slots[2 * slot] = symbol.packedBytes(); - slots[2 * slot + 1] = ignoredBits << 16 | (long) code << 8 | symbol.length(); + table[2 * slot] = symbol.packedBytes(); + table[2 * slot + 1] = ignoredBits << 16 | (long) code << 8 | symbol.length(); } - return new LossyPerfectHashTable(slots); + return new LossyPerfectHashTable(table); } /// Looks up `word` and returns the matched symbol as `code << 8 | length`, or 0 when no stored @@ -99,8 +99,8 @@ static LossyPerfectHashTable of(List symbolsByGainDescending) { /// @return the match as `code << 8 | length`, or 0 when there is no match int lookup(long word) { int slot = slotFor(word) << 1; - long symbol = slots[slot]; - long meta = slots[slot + 1]; + long symbol = table[slot]; + long meta = table[slot + 1]; return (word & (~0L >>> (int) (meta >>> 16))) == symbol ? (int) (meta & 0xFFFF) : 0; } diff --git a/fsst/src/main/java/io/github/dfa1/vortex/fsst/ShortCodeTable.java b/fsst/src/main/java/io/github/dfa1/vortex/fsst/ShortCodeTable.java index 173439fa5..e33e806b5 100644 --- a/fsst/src/main/java/io/github/dfa1/vortex/fsst/ShortCodeTable.java +++ b/fsst/src/main/java/io/github/dfa1/vortex/fsst/ShortCodeTable.java @@ -32,10 +32,10 @@ final class ShortCodeTable { private static final int NO_MATCH = NO_CODE << 8; /// Packed `code << 8 | length` per 16-bit key. A zero length marks "no match" ([#NO_MATCH]). - private final int[] slots; + private final int[] table; - private ShortCodeTable(int[] slots) { - this.slots = slots; + private ShortCodeTable(int[] table) { + this.table = table; } /// Builds the table from symbols in descending-gain order, keeping only the length-1 and @@ -51,8 +51,8 @@ private ShortCodeTable(int[] slots) { /// @param symbolsByGainDescending the trained symbols, code = list index, gain-descending /// @return a table resolving 0/1/2-byte matches for any two-byte input prefix static ShortCodeTable of(List symbolsByGainDescending) { - int[] slots = new int[SLOTS]; - Arrays.fill(slots, NO_MATCH); + int[] table = new int[SLOTS]; + Arrays.fill(table, NO_MATCH); for (int code = 0; code < symbolsByGainDescending.size(); code++) { Symbol symbol = symbolsByGainDescending.get(code); if (symbol.length() == 1) { @@ -60,8 +60,8 @@ static ShortCodeTable of(List symbolsByGainDescending) { int packed = code << 8 | 1; for (int high = 0; high < 256; high++) { int key = high << 8 | low; - if (length(slots[key]) == 0) { - slots[key] = packed; + if (length(table[key]) == 0) { + table[key] = packed; } } } @@ -70,10 +70,10 @@ static ShortCodeTable of(List symbolsByGainDescending) { Symbol symbol = symbolsByGainDescending.get(code); if (symbol.length() == 2) { int key = (int) (symbol.packedBytes() & 0xFFFF); - slots[key] = code << 8 | 2; + table[key] = code << 8 | 2; } } - return new ShortCodeTable(slots); + return new ShortCodeTable(table); } /// Returns the match for the low two bytes of `word` as `code << 8 | length`, or @@ -84,7 +84,7 @@ static ShortCodeTable of(List symbolsByGainDescending) { /// @param word an input word; only its low 16 bits (first two input bytes) are consulted /// @return the match as `code << 8 | length`; length 0 (and code [#NO_CODE]) means no match int packedFor(long word) { - return slots[(int) (word & 0xFFFF)]; + return table[(int) (word & 0xFFFF)]; } /// Returns the code matched by the low two bytes of `word`, or [#NO_CODE] if none. diff --git a/integration/src/test/java/io/github/dfa1/vortex/integration/RaincloudConformanceIntegrationTest.java b/integration/src/test/java/io/github/dfa1/vortex/integration/RaincloudConformanceIntegrationTest.java index 62051c169..4cc777817 100644 --- a/integration/src/test/java/io/github/dfa1/vortex/integration/RaincloudConformanceIntegrationTest.java +++ b/integration/src/test/java/io/github/dfa1/vortex/integration/RaincloudConformanceIntegrationTest.java @@ -70,6 +70,8 @@ class RaincloudConformanceIntegrationTest { Path.of(System.getProperty("user.home"), ".cache", "raincloud", "corpus-manifest.tsv"); @Test + @SuppressWarnings("java:S2699") // assumeTrue IS this test's check: a visible skip marker, + // not a pass/fail assertion — there is nothing else to assert void corpusIsHydrated() { // Given / When / Then — visible skip marker when the corpus is absent; the // factory below yields zero tests in that case, which would otherwise pass silently diff --git a/integration/src/test/java/io/github/dfa1/vortex/integration/RaincloudSizeComparisonIntegrationTest.java b/integration/src/test/java/io/github/dfa1/vortex/integration/RaincloudSizeComparisonIntegrationTest.java index c53a7c5f7..79fe92e3b 100644 --- a/integration/src/test/java/io/github/dfa1/vortex/integration/RaincloudSizeComparisonIntegrationTest.java +++ b/integration/src/test/java/io/github/dfa1/vortex/integration/RaincloudSizeComparisonIntegrationTest.java @@ -40,6 +40,8 @@ class RaincloudSizeComparisonIntegrationTest { Path.of(System.getProperty("user.home"), ".cache", "raincloud", "corpus-manifest.tsv"); @Test + @SuppressWarnings("java:S2699") // assumeTrue IS this test's check: a visible skip marker, + // not a pass/fail assertion — there is nothing else to assert void corpusIsHydrated() { // Given / When / Then — visible skip marker when the corpus is absent assumeTrue(Files.exists(manifestPath()), diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/compute/Predicate.java b/reader/src/main/java/io/github/dfa1/vortex/reader/compute/Predicate.java index dbaa89225..35a6e52bc 100644 --- a/reader/src/main/java/io/github/dfa1/vortex/reader/compute/Predicate.java +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/compute/Predicate.java @@ -32,6 +32,10 @@ public sealed interface Predicate Predicate.Gte, Predicate.Between, Predicate.IsNull, Predicate.IsNotNull, Predicate.And, Predicate.Or { + /// `Objects.requireNonNull` message naming the rejected component, shared by every leaf + /// below whose sole component is named `value`. + String VALUE = "value"; + /// Matches rows whose value equals `value`. /// /// Equality is value-domain equality (the kernel decides how that maps onto the encoded form); @@ -44,7 +48,7 @@ record Eq(Object value) implements Predicate { /// /// @param value the value to compare against, must be non-null public Eq { - Objects.requireNonNull(value, "value"); + Objects.requireNonNull(value, VALUE); } } @@ -61,7 +65,7 @@ record Neq(Object value) implements Predicate { /// /// @param value the value to compare against, must be non-null public Neq { - Objects.requireNonNull(value, "value"); + Objects.requireNonNull(value, VALUE); } } @@ -74,7 +78,7 @@ record Lt(Comparable value) implements Predicate { /// /// @param value the exclusive upper bound, must be non-null public Lt { - Objects.requireNonNull(value, "value"); + Objects.requireNonNull(value, VALUE); } } @@ -87,7 +91,7 @@ record Gt(Comparable value) implements Predicate { /// /// @param value the exclusive lower bound, must be non-null public Gt { - Objects.requireNonNull(value, "value"); + Objects.requireNonNull(value, VALUE); } } @@ -100,7 +104,7 @@ record Lte(Comparable value) implements Predicate { /// /// @param value the inclusive upper bound, must be non-null public Lte { - Objects.requireNonNull(value, "value"); + Objects.requireNonNull(value, VALUE); } } @@ -113,7 +117,7 @@ record Gte(Comparable value) implements Predicate { /// /// @param value the inclusive lower bound, must be non-null public Gte { - Objects.requireNonNull(value, "value"); + Objects.requireNonNull(value, VALUE); } } diff --git a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoder.java b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoder.java index 681bf8911..062d8151e 100644 --- a/reader/src/main/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoder.java +++ b/reader/src/main/java/io/github/dfa1/vortex/reader/decode/SparseEncodingDecoder.java @@ -35,6 +35,12 @@ /// Read-only decoder for `vortex.sparse`. public final class SparseEncodingDecoder implements EncodingDecoder { + /// `role` argument naming the patch indices child, for error messages. + private static final String ROLE_INDICES = "indices"; + + /// `role` argument naming the patch values child, for error messages. + private static final String ROLE_VALUES = "values"; + @Override public EncodingId encodingId() { return EncodingId.VORTEX_SPARSE; @@ -119,10 +125,10 @@ public Array decode(DecodeContext ctx) { valData = m.inner(); patchValidity = m.validity(); } - checkPatchChild(idxData, numPatches, "indices"); - checkPatchChild(valData, numPatches, "values"); + checkPatchChild(idxData, numPatches, ROLE_INDICES); + checkPatchChild(valData, numPatches, ROLE_VALUES); boolean fillValue = Boolean.TRUE.equals(fillScalar.bool_value()); - BoolArray boolValues = checkedCast(valData, BoolArray.class, "values"); + BoolArray boolValues = checkedCast(valData, BoolArray.class, ROLE_VALUES); Array result = new LazySparseBoolArray(ctx.dtype(), n, fillValue, boolValues, idxData, offset); return withSparseValidity(ctx, result, fillValid, patchValidity, idxData, numPatches, n, offset); } @@ -146,26 +152,26 @@ public Array decode(DecodeContext ctx) { valData = m.inner(); patchValidity = m.validity(); } - checkPatchChild(idxData, numPatches, "indices"); - checkPatchChild(valData, numPatches, "values"); + checkPatchChild(idxData, numPatches, ROLE_INDICES); + checkPatchChild(valData, numPatches, ROLE_VALUES); Array result = switch (valuePtype) { case I64, U64 -> new LazySparseLongArray(ctx.dtype(), n, fillBits, - checkedCast(valData, LongArray.class, "values"), idxData, offset); + checkedCast(valData, LongArray.class, ROLE_VALUES), idxData, offset); case I32, U32 -> new LazySparseIntArray(ctx.dtype(), n, (int) fillBits, - checkedCast(valData, IntArray.class, "values"), idxData, offset); + checkedCast(valData, IntArray.class, ROLE_VALUES), idxData, offset); case F64 -> new LazySparseDoubleArray(ctx.dtype(), n, Double.longBitsToDouble(fillBits), - checkedCast(valData, DoubleArray.class, "values"), idxData, offset); + checkedCast(valData, DoubleArray.class, ROLE_VALUES), idxData, offset); case F32 -> new LazySparseFloatArray(ctx.dtype(), n, Float.intBitsToFloat((int) fillBits), - checkedCast(valData, FloatArray.class, "values"), idxData, offset); + checkedCast(valData, FloatArray.class, ROLE_VALUES), idxData, offset); case I16 -> new LazySparseShortArray(ctx.dtype(), n, (short) fillBits, (short) fillBits, - checkedCast(valData, ShortArray.class, "values"), idxData, offset); + checkedCast(valData, ShortArray.class, ROLE_VALUES), idxData, offset); case U16 -> new LazySparseShortArray(ctx.dtype(), n, (short) fillBits, (int) (fillBits & 0xFFFFL), - checkedCast(valData, ShortArray.class, "values"), idxData, offset); + checkedCast(valData, ShortArray.class, ROLE_VALUES), idxData, offset); case I8 -> new LazySparseByteArray(ctx.dtype(), n, (byte) fillBits, (byte) fillBits, - checkedCast(valData, ByteArray.class, "values"), idxData, offset); + checkedCast(valData, ByteArray.class, ROLE_VALUES), idxData, offset); case U8 -> new LazySparseByteArray(ctx.dtype(), n, (byte) fillBits, (int) (fillBits & 0xFFL), - checkedCast(valData, ByteArray.class, "values"), idxData, offset); + checkedCast(valData, ByteArray.class, ROLE_VALUES), idxData, offset); default -> throw new VortexException(EncodingId.VORTEX_SPARSE, "unsupported ptype " + valuePtype); }; return withSparseValidity(ctx, result, fillValid, patchValidity, idxData, numPatches, n, offset); @@ -275,7 +281,7 @@ private static Array decodeVarBin( DType indicesDtype = new DType.Primitive(indicesPtype, false); Array patchIndices = ctx.decodeChild(0, indicesDtype, numPatches); Array idxData = patchIndices instanceof MaskedArray m ? m.inner() : patchIndices; - checkPatchChild(idxData, numPatches, "indices"); + checkPatchChild(idxData, numPatches, ROLE_INDICES); byte[] fill = fillBytes(fillScalar, fillValid); if (numPatches == 0) { @@ -294,7 +300,7 @@ private static Array decodeVarBin( valData = m.inner(); patchValidity = m.validity(); } - VarBinArray values = checkedCast(valData, VarBinArray.class, "values"); + VarBinArray values = checkedCast(valData, VarBinArray.class, ROLE_VALUES); Array result = new VarBinSparseArray(ctx.dtype(), n, fill, values, idxData, offset); return withSparseValidity(ctx, result, fillValid, patchValidity, idxData, numPatches, n, offset); }