21462 cursor compaction 6.0 stabilization - #4972
Open
rustyrazorblade wants to merge 35 commits into
Open
Conversation
Differential testing of cursor-based vs iterator-based compaction over identical inputs found five output divergences on the supported surface, one affecting data correctness: 1. Same-timestamp cell reconciliation kept the lexically smaller value: the value comparison in CursorCompactor.mergeCells was inverted relative to Cells.resolveRegular (left/current must win unless the challenger value is strictly greater). Equal-timestamp conflicting writes resolved to a different survivor than iterator compaction, silently diverging replicas. Also adds the missing lower-TTL tie-break (both expiring, same timestamp and expiration) to CursorCompactor.resolveRegular, mirroring Cells.java. 2. previousUnfilteredSize was written as 0 for every row and range tombstone marker. Now tracked per partition and written exactly as SortedTablePartitionWriter.addUnfiltered does, including the vint length in the row/marker size and the static-row special case (hardcoded 0, does not advance the chain). The field is unread today, but cursor output now matches iterator output byte for byte. 3. Tombstone cells carried a spurious IS_EXPIRING flag plus a wasted TTL vint: ReusableLivenessInfo.isExpiring() means "has an expiration time" (also true for tombstones), and the flag rebuild used two independent ifs where Cell.Serializer uses if/else-if. Now uses strict ttl != NO_TTL semantics and the else-if structure. 4. The final index block width excluded the end-of-partition marker byte; BigFormatPartitionWriter.finish() indexes the final block after the marker is written, so the reference width includes it. 5. Partitions crossing column_index_size exactly once got no promoted index: the promotion decision ran before the tail block was counted, while the iterator promotes when the total including the tail exceeds one block. Such partitions (64-128KiB at default settings) lost intra-partition index seeks. The tail block is now counted before deciding, matching RowIndexEntry.create for every cut/tail combination.
Runs identical input sstables through both the cursor and iterator compaction pipelines via the full production CompactionTask path and asserts equivalence at two levels: byte-identical output components (with an explicit allowlist for justified divergences — currently empty) and a logical walk (sstabledump-format JSON with a fixed clock, stats spot-checks, plus IVerifier extended verification per output). Harness invariants: - Inputs are byte-identical for both runs: the first run keeps original files (keepOriginals), restore delists outputs by descriptor and re-opens the original descriptors directly. SSTableImporter is deliberately avoided because resetLevel can rewrite input stats. - The same gcBefore is passed to both runs so purge decisions cannot flip between them. - The cursor run asserts CursorCompactor.isSupported up front, using the same check production uses to select the pipeline: a scenario that would silently fall back to the iterator fails loudly instead of passing vacuously. - Preemptive open is disabled during runs: SSTableRewriter.moveStarts obsoletes fully-covered originals regardless of keepOriginals, which would delete the input files between runs. Early open affects reader visibility during compaction, not the bytes written. Scenarios cover the currently supported cursor compaction surface: overwrites, all tombstone kinds including overlapping and adjacent range tombstones, purge with empty-partition drop, static rows, DESC clustering, composite clusterings with prefix deletes, wide partitions crossing many index blocks, the index promotion boundary (one cut block plus tail), frozen collections, TTLs, same-timestamp reconciliation ties, shadowed partitions, single-input/8-way/disjoint merges, and empty vs null values. CursorSupportMatrixTest pins the unsupportedMetadata verdict per schema shape so a change that silently widens or narrows the cursor fallback becomes a test failure.
For a static-column table whose partition has no static values, both compaction paths write an empty static row, but the iterator path only collects row statistics for non-empty rows (SortedTableWriter.addStaticRow guards Rows.collectStats with !row.isEmpty()). The cursor writer counted every written row, inflating totalRows and totalColumnsSet by one per such partition. Found by randomized differential testing within its first examples; the prior hand-written static-row scenario gave every partition static data and could not produce the trigger. writeRowEnd now skips row-stats collection when the row is empty by the iterator's definition: no cells, no liveness timestamp or TTL, and no row deletion.
RandomDifferentialCompactionTest generates random schemas restricted to the cursor-supported surface using the same unsupportedMetadata filter production uses to select the pipeline (the generator space widens automatically as gaps close), plus a guard rejecting invalid generated CQL (static columns require clustering columns). Each example runs a random multi-round workload — 15-40 inserts per round with 30% primary-key overwrites, row deletes, occasional partition deletes, flush per round, 2-4 rounds — through the byte+logical differential harness. Failures are reproducible: every failure message carries the seed (seed chaining follows RandomSchemaTest) and the generated schema is logged per example. Known v1 coverage limits, exercised by the deterministic corpus instead: NORMAL value domain only (no generated nulls/empty values) and no generated range or cell deletes. Also adds the deterministic emptyStaticRows scenario pinning the stats over-count this soak found on its first run.
Partial-set compactions (PartialSetDifferentialCompactionTest): only a subset of live sstables participates and the purge evaluator must consult overlapping non-participating sstables before dropping tombstones. Scenarios pin purge blocked by an overlapping non-participant holding older data, purge allowed with a disjoint non-participant, and tombstones retained over a shadowed non-participant left out of the compaction. All writes use explicit timestamps so purge decisions are deterministic. Multi-output compactions (MultiOutputDifferentialCompactionTest): a size-capped MaxSSTableSizeWriter (8KiB, compression disabled) forces writer switches at partition boundaries; 4-6 outputs per compaction are compared pairwise. Covers switch-decision parity and per-output finalization (first/last keys, promoted index, per-output stats). Harness extensions in DifferentialCompactionTester: - compactPath takes an input subset; outputs are identified by before/after descriptor diff; non-participating sstables stay live and untouched across the restore - TaskFactory hook so scenarios can shape the CompactionTask (e.g. override getCompactionAwareWriter for multi-output) identically for both pipelines - assertCursorMatchesIterator returns the captured output so scenarios can assert their mechanism actually fired: multi-output scenarios require >= 2 outputs, partial-set scenarios verify the purge outcome in the captured dump. Motivated by the first multi-output run being vacuously green: MaxSSTableSizeWriter switches on estimated on-disk (compressed) bytes and repetitive test padding compressed below the cap, so no switch ever happened.
PurgeBoundaryDifferentialCompactionTest: exact purge-boundary scenarios without clock control. The harness gains an explicit-gcBefore overload; the scenario reads the real deletion second from the flushed sstable's stats (excluding the Long.MAX_VALUE no-deletion sentinel) and runs the differential at gcBefore == ldt (tombstones retained: purge requires localDeletionTime strictly less than gcBefore) and gcBefore == ldt + 1 (purged), asserting the outcome in the captured dump both ways. Also covers compaction reads with direct disk access mode and the Zstd, Deflate and Snappy compressors. HarryDifferentialCompactionTest: deep probabilistic tombstone histories (interleaved inserts with row, column, range and partition deletes over multiple flush rounds, reversed clustering, static columns) built with Harry's HistoryBuilder and executed as plain CQL in this JVM via CQLTesterVisitExecutor. Seeded and logged for reproduction. The differential harness is the oracle, so no Harry validation reads are issued. The TTL-expiry-exactly-at-nowInSec boundary remains uncovered: nowInSec is taken inside CompactionTask per run and a custom Clock can only be installed at JVM startup, so deterministic coverage needs a dedicated test-runner configuration.
…e property The differential harness cannot catch allocation regressions: output bytes are identical whether or not the path allocates. This gate measures ThreadMXBean thread-allocated bytes around CompactionTask execution for a small and a 10x-larger table (warmed, min over several iterations) and asserts the growth stays under a ceiling. Two scales are gated: - allocationDoesNotScaleWithRows: 1,200 vs 12,000 rows, 512KB ceiling (measured residual ~450KB is entirely outside cursor-owned code: Ref debug tracking enabled in the ant test JVM, chunk cache machinery, per-key metadata; run-to-run variance is ~hundreds of bytes, so the gate trips at roughly +6 bytes per row) - allocationAtLargeFileSizes: 4 input sstables of ~10MB each, gated as a per-input-byte ratio (measured ~0.27 B allocated per extra input byte in the test environment; ceiling 0.5) since the residual scales with data volume, not row count Both measurements assert the cursor pipeline actually ran (the isSupported guard) so a silent fallback to the iterator path cannot produce a vacuous pass, and both log iterator-path numbers for context (~8x higher at scale). JFR diagnostic recorders for both scales are included for offline attribution of any future gate failure. Also extracts the harness restore/output-identification machinery (restoreAfterCompaction, identifyOutputs) for capture-free callers, and removes references to local working notes from test documentation so the committed tests are self-contained.
…t decoding Rows that do not contain every header column are serialized with a column-subset encoding. The cursor reader materialized that subset via Columns.serializer.deserializeSubset for every such row (a new Columns plus BTree build per row), which also defeated CellCursor's identity-cached column arrays, rebuilding them per row — roughly 150 bytes of garbage per sparse row on a path that is meant to be allocation-free. The site carried a 'TODO: re-implement GC free' comment. Sparse rows are an everyday shape: cell deletes, partial updates, inserts that do not set every column. For header supersets of fewer than 64 columns the subset wire format is a single unsigned vint bitmask of missing columns (bit i = i-th column in iteration order; 0 = all present). UnfilteredDescriptor now reads that mask directly and keeps rowColumns pointing at the superset; CellCursor iterates the present columns by bit-walking the complement (numberOfTrailingZeros / clear-lowest-bit) over its once-initialized superset arrays, preserving exact serializer iteration order. Supersets of 64 or more columns keep the allocating path: the large-subset wire format is structural rather than a mask, and the shape is rare. Measured with the allocation-scaling gate using a scenario where every other row omits one column: growth between a 1,200-row and 12,000-row compaction drops from 1,487,448+ bytes (sparse rows added ~830KB over the full-row baseline) back to the full-row residual, passing the 512KB ceiling. All differential suites remain byte-identical: the change is read-side only.
Every other row omits a column, so rows carry the column-subset encoding instead of the all-columns flag — the trigger class the original full-row scenario could not exercise. Before the subset decoding fix this scenario failed the gate at 1,280,440 bytes of scaling growth (~154 bytes per sparse row); it now passes the same 512KB ceiling as the full-row scenario.
Pure refactor: SSTableCursorWriter's index machinery — promoted index blocks serialized incrementally, the tail-counted promotion decision, Index.db entries, bloom filter and summary appends — moves verbatim into BigCursorIndexWriter behind a new CursorIndexWriter seam (startPartition / rowWritten / staticRowWritten / endPartition / block clock). The seam is event-shaped because unfiltered descriptors are transient: implementations needing block-boundary clusterings must capture them at row time, which is also how the BIG code already worked (eager first-clustering serialization, reusable last-clustering copy). One deliberate substitution: the tail block passes DeletionTime.LIVE where the old code read the writer's open marker — equivalent because range tombstones close before partition end. No behavior change: this branch's full differential suite passes byte-identically before and after. Kept purely as enabling infrastructure — later fixes (2GiB index-offset widening, disabled bloom filter tolerance) are written against BigCursorIndexWriter's class structure rather than the original monolithic writer.
moveStarts() obsoleted fully-covered originals unconditionally — only the bulk obsoleteOriginals() call honored the keepOriginals flag — so a full-range rewrite over an online transaction with early open in play deleted every original after the last reference was released. The in-tree keepOriginals users (scrub, offline tooling) run offline transactions where moveStarts returns early, so this was a latent contract violation rather than live data loss; any online caller relying on keepOriginals as a retain-inputs mechanism silently lost them. One-line guard: fully-covered originals are not obsoleted when the caller asked to keep them. SSTableRewriterKeepOriginalsTest pins the contract: an online, early-open, keepOriginals=true rewrite must leave the original data file on disk after pending deletions run. It lives in its own class deliberately — a kept original leaves intentional accounting residue (its bytes stay counted while no longer in the live set) that would poison the shared fixture of SSTableRewriterTest; per-class forked test JVMs make the residue harmless. The differential compaction harness previously disabled preemptive open to work around exactly this bug (it depends on keepOriginals retaining its inputs); that workaround is removed, so every differential suite now runs with early open enabled and doubles as a standing regression test. Also expands comments at the least-obvious code on the branch: the present-columns bitmask construction and bit-walk in the cursor reader (the n-low-ones shift template, the shift-mod-64 special case, and the bit-order == serializer-order == disk-order invariant that makes the walk correct), the column-subset corruption check, and the BIG index writer's eager first-clustering serialization and single-row-block self-copy.
The cursor path's raw clustering comparison (ClusteringComparator's descriptor overload, also backing CursorCompactor's merge ordering) decided null/empty/valued component ordering purely from the serialized flag bits: null < empty < valued, never consulting the type. The iterator's reference comparison handles nulls outside the type — nulls sort first even on DESC columns — but routes empty-vs-valued THROUGH the type, and ReversedType swaps operands around the base type's empty-sorts-first rule, so on a DESC clustering column an empty value sorts AFTER values. The flag-only rule was therefore inverted for empty-vs-valued on reversed columns. Consequences within the trigger window (DESC clustering column plus a legal zero-length clustering value): rows merged in the wrong order when an empty and a valued clustering for the same partition arrived from different sstables — an output sstable ordered inconsistently with its comparator — and the covered-clustering stats bounds picked the wrong extreme across partitions (an 8-byte Statistics.db divergence, which is how the randomized differential soak caught it, seed 99303954147053). Fix: flip the flag-derived order for empty-vs-valued (never for null) when the column type is reversed. Flag arithmetic only — no allocation on the hot path. EdgeCaseDifferentialCompactionTest.emptyClusteringValuesDescending pins both variants (fails pre-fix with a row-level logical divergence); emptyClusteringValuesAscending guards the unflipped ASC ordering.
The differential harness compared single-generation outputs: both paths compact the same inputs once. Write-side corruption that only manifests when the NEXT merge re-reads the output — a bug class where a flag is decided after its byte is already written — was invisible to it. assertCursorMatchesIteratorAcrossGenerations runs the normal differential, then genuinely commits a cursor-path compaction (no restore — the live set becomes cursor-produced sstables) and runs the differential again over those outputs. Every edge-corpus scenario now uses it, so each scenario also proves cursor output is a correct INPUT to both pipelines.
View tables are admitted by the cursor gate and compact through the same pipeline, but their row shapes are view-specific: strict liveness (CursorCompactor.enforceStrictLiveness mirrors PurgeFunction's), view-generated row tombstones when a base update moves a row between view partitions, and tombstones meeting tombstones on repeated moves. The new suite compacts the VIEW's table differentially, including across two generations. The audit behind this found no code gap: shadowable row deletions have no remaining producers (view maintenance stopped writing them in CASSANDRA-13409; Deletion.shadowable has zero callers), legacy sstables carrying the flag cannot reach the cursor through the sstable-version gate, and the cursor reader fails loudly on the extended flag rather than misreading it.
The row-centric allocation gates barely touch the marker path (ReusableDeletionTime pool, open-marker tracking, marker serialization). The new gate compacts partitions carrying 300 bounded range tombstones per round, with the second round's bounds shifted so every marker pair overlaps across sstables and forces real deletion reconciliation per marker. Asserted per input byte at ~2MB scale rather than as a fixed delta: the first fixed-ceiling version failed on pure test-environment residual (JFR attribution via the new permanent recordRangeTombstoneAllocationProfile diagnostic showed zero cursor-owned sites — Ref$Debug stack captures and buffer-pool machinery only). Measured 0.684 B/B (ceiling 1.0). A leak of one small object per marker costs upwards of 1.5 B/B extra and trips the ceiling with margin. The cursor's marker path is allocation-clean by measurement.
The soak previously generated inserts, row deletes, and partition
deletes over NORMAL-domain values with single-column partition keys —
much narrower than the deterministic corpus. It now also generates:
UPDATE-built rows (cells without primary-key liveness),
primary-key-only INSERTs (liveness without cells), long-TTL inserts,
explicit USING TIMESTAMP collisions from a small pool (same-timestamp
tie-breaks on overwritten keys), null values on non-key columns (cell
tombstones on simple columns; empty buffers on clustering columns),
single-sided and prefix range deletes, multi-column and static cell
deletes, and composite partition keys. Any schema the generator
produces that needs multi-cell-column support is rejected before use
by the same unsupportedMetadata filter production uses, so the
null-value generation is safe on this branch even though it does not
support multi-cell columns.
Two new knobs, both forwarded via -Dtest.jvm.args:
- cassandra.test.differential.examples=N scales the example count
(default 10; the pre-JIRA validation run uses thousands)
- cassandra.test.differential.seed=N replays a failing seed as
example 0 — no code edit needed to reproduce
The widened space found a real bug in its first 25-example run: the
reversed-column empty-clustering comparison fix ("Respect clustering
reversal when comparing absent components"), caught as an 8-byte
covered-clustering divergence in Statistics.db (seed 99303954147053).
Three new differential scenarios cover row-liveness and TTL shapes: UPDATE-built rows without primary-key liveness and primary-key-only inserts with liveness but no cells (rowLivenessShapes), row-TTL vs cell-TTL mixtures including same-timestamp expiring ties with different TTLs (rowAndCellTtlMix), and same-timestamp ties between an expiring cell and a live one in both directions, the CASSANDRA-14592 rule (expiringVsLiveTies). Also fixes a stats-poisoning bug: DeletionTime.ReusableDeletionTime.reset(long, long) classified NO_DELETION_TIME (Long.MAX_VALUE, the canonical "no deletion" long) as invalid instead of mapping it to the LIVE marker the way Cell.deletionTimeLongToUnsignedInteger does, so resetting from LIVE's long values produced a NON-live deletion. A live marker slipping past MetadataCollector's isLive guard this way would record minTimestamp=Long.MIN_VALUE, a bogus tombstone count, and an encoding-stats minimum collapsed to the epoch — stats-only severity, but those stats feed compaction heuristics. DeletionTimeTest.resetWithLiveLongsStaysLive pins the round-trip directly.
Adds a vector/duration column scenario (single-cell types, overwritten and null-overwritten across sstables) plus a support-matrix pin confirming they're inside the supported surface, and >64-regular-column scenarios that byte-compare the large-subset wire format between the cursor and iterator paths for the first time. The >64-column scenario immediately exposed real output corruption: SSTableCursorWriter's encodeLargeColumnsSubset had two bugs against the reference Columns.Serializer format. First, its trailing-present loop was bounded by the last missing index instead of the superset size — vacuously empty — so present columns sorting after the last missing column were dropped from the encoding, and the deserializer then consumed row-body bytes as column indices: desync, EOF, an sstable that crashes every subsequent read. Second, encoding-mode selection used missing > supersetCount/2 where both the reference serializer and the deserializer use presentCount < supersetCount/2 — equivalent for even superset sizes but flipped for odd sizes at the boundary, making the decoder read the wrong mode. No pre-existing test exercised present-index mode (few-missing rows always take missing-index mode), which is how both survived. over64Columns pins the dropped-tail shape (fails pre-fix with CorruptSSTableException); over64ColumnsOddSupersetBoundary pins the mode boundary with a 71-column superset and rows at 34/35/36 present columns.
Open-ended (single-sided) range tombstones were covered only probabilistically by the randomized soak. Two deterministic scenarios now pin them: markers open to TOP and from BOTTOM, nested open ranges, a partition containing only an open range tombstone, resurrection inside open-deleted ranges, interleaving with bounded tombstones, and a DESC variant where the bound direction inverts in on-disk order. Unbounded bounds are zero-component clustering prefixes — the same empty-prefix comparison region as the reversed-column finding. Separately, the materialized-view suite exposed a harness hole: JsonTransformer computes its "expired" fields from the wall clock, ignoring the nowInSec parameter the harness fixes for deterministic rendering. The two paths' captures run seconds apart, so a localExpirationTime falling between them renders expired=false on one side and expired=true on the other over byte-identical sstables — materialized-view expired-liveness rows sit permanently on that boundary because their expiration is the write second. The harness now normalizes the flag out of the dump before comparing; it is derived from expires_at, which is still compared. (Upstream observation for the eventual ticket: sstabledump's "expired" ignores the tool's own time parameter.)
Writes 2,000,000 rows across twenty input sstables — 2,000 partitions x 50 rows per round x 20 flush rounds, each round's clustering window overlapping the previous by half so most output rows genuinely merge from two or three inputs — with a realistic mutation mix (TTL'd rows, explicit-timestamp ties on overwritten keys, null-overwrite cell tombstones, row/range/partition deletes). To keep harness memory flat at this scale, capture gains a scale mode: the logical dump streams through the expired-field normalization into a SHA-256 digest instead of being retained as a String, and the byte-level comparison streams with 64KB buffers instead of reading whole components into memory. Byte-identical on both paths.
Following the soak's examples/seed knob pattern (system properties forwarded to the forked test JVM via -Dtest.jvm.args): cassandra.test.differential.bigvolume.rounds (default 20) cassandra.test.differential.bigvolume.partitions (default 2000) cassandra.test.differential.bigvolume.rows_per_round (default 50) cassandra.test.differential.bigvolume.value_padding (default 0) Defaults reproduce the already-validated runs exactly. Delete counts scale with the partition count, and the clustering stride keeps consecutive rounds overlapping by half a window. Each run logs its effective parameters.
One giant partition merged from many inputs, sized for the POSITIONAL boundaries that row count alone never touches: intra-partition offsets crossing Integer.MAX_VALUE, and the promoted index at hundreds of thousands of index blocks for a single partition entry. Small partitions flank the giant one every round so per-partition state provably resets on both sides. Also fixes the scale-mode digest stream, which previously buffered one full dump line — and the dump emits one line PER PARTITION, which a multi-GB partition would turn into a multi-GB buffer. Oversized lines now flush in bounded chunks at content-determined cut points, keeping a small unprocessed tail so the expired-field normalization token can never straddle a cut; capture memory stays flat at any partition size.
The differential harness carried an escape hatch from day one: a set of component names permitted to differ byte-wise between the cursor and iterator outputs, each requiring written justification. Across the entire corpus, it was never used once: every byte divergence flagged was a real bug in one of the paths. This makes that structural. The parameter, the checks, and every suite's empty constant are gone; byte identity of every output component is unconditional and there is no exception mechanism to reach for. Also fixes the format-gate debug message in CursorCompactor, which claimed "Only BIG and BTI sstable output formats are supported" over a condition that only accepts BIG on this branch (BTI support is not part of this stabilization branch) — corrected to say BIG only, and dropped the resulting unused BtiFormat import.
ReusableLivenessInfo.isExpiring() means "has an expiration time", which is also true for tombstones (a deletion also carries a localExpirationTime). The tie-break used !left.isExpiring() to mean "is a tombstone", so it was false for BOTH sides whenever both already passed the earlier presence check — making the tie-break dead code and letting the localDeletionTime comparison below it pick an expiring cell over a genuine tombstone at the same timestamp. Tombstone semantics need ttl() == NO_TTL instead: within this code path both sides already have localExpirationTime set, so ttl == NO_TTL is exactly "is a tombstone" and mirrors Cells.resolveRegular's actual rule. EdgeCaseDifferentialCompactionTest.tombstoneVsExpiringTies pins both flush orders and both tombstone shapes (UPDATE SET v = null, DELETE v).
…d the index CursorIndexWriter.indexBlockStartOffset was an int, unlike the reference SortedTablePartitionWriter's long. Partitions larger than 2GiB wrap the offset negative past Integer.MAX_VALUE, and the block-cut arithmetic then cuts every row — corrupting the serialized offsets in the promoted index. Pinned by the giant-partition boundary run (ck_stride = rows_per_sstable, forcing disjoint windows so the merged partition size is maximized rather than halved by overlap). Also: the differential harness's structural verifier debug-logs every index block during its extended walk (~560K lines for a >2GiB partition), and ant's junit formatter buffers all test output in memory — at that log volume the buffering, not the verification itself, OOMs the test fork. The verifier's debug stream is now silenced in scale mode. Also corrects the giant-partition scenario's own math: the logged/documented partition size used sstables * rows_per_sstable (the total PRE-MERGE input rows), not the actual MERGED partition size, which with the default half-window overlap is (sstables-1) * ck_stride + rows_per_sstable — a smaller number. A prior "2.6GiB" boundary run under this miscount actually reached only ~1.4GiB. ck_stride is now a knob (defaulting to the existing half-window overlap); setting it equal to rows_per_sstable produces disjoint windows and the true worst-case merged partition size, needed to genuinely exercise the 2GiB boundary.
…ables allocated per row Rows in a >= 64-column table that don't set every column use the large-subset wire format (index vints, present- or missing-mode). The reader decoded this by materializing a fresh Columns object per row via Columns.serializer.deserializeSubset, which also defeated CellCursor's identity-cached column arrays — rebuilding them per row instead of once per superset. This is the same class of allocation the < 64-column mask path was already fixed for; the >= 64-column path had a "keep the allocating path" comment marking it as a known gap. UnfilteredDescriptor now decodes the large-subset wire format directly into reusable present-mask words (one bit per superset column, same semantics as the < 64-column mask); CellCursor walks those words instead of materializing a subset Columns. The allocation-gate test (allocationDoesNotScaleWithWideSchemaSparseRows) pins the fix at a 70-column superset with both present-mode and missing-mode rows.
The >= 64-column subset encoder (encodeLargeColumnsSubset, fixed for two corruption bugs in the previous commit) is hand-mirrored from Columns.Serializer.serializeLargeSubset because the upstream serializer needs a materialized Columns plus iterator per call — an allocation this path must not make. A hand-mirrored implementation can drift from the reference again the same way it already did once, so this extracts the encoder into a static, directly-testable method (encodeColumnsSubset) and pins it byte-for-byte against the real serializer across sizes, shapes, and both mode-selection boundaries (CursorColumnsSubsetEncodingTest). No behavior change.
With bloom_filter_fp_chance = 1.0, FilterFactory hands out the AlwaysPresentFilter instead of a concrete BloomFilter — the iterator path calls add() on it through the generic IFilter interface (a harmless no-op), but BigCursorIndexWriter unconditionally cast indexWriter.bf to BloomFilter to use its garbage-free add() overload, throwing ClassCastException the first time a table disabled its bloom filter. The cast now happens once at construction time behind an instanceof check; a null result means "nothing to add to" and the add() call is skipped. BasicDifferentialCompactionTest gains a bloomFilterDisabled scenario compacting a table created with bloom_filter_fp_chance = 1.
CompactionIterator.purger() purges and expires accord-enabled (and accord-migrating) tables relative to gcBefore, derived from accord's durability bounds, rather than the wall-clock nowInSec used elsewhere — retaining data accord may still need to read at earlier timestamps. CursorCompactor unconditionally used nowInSec for every purge/expiry decision, so an accord-enabled table compacted through the cursor path could purge or expire data accord still needed. The constructor now mirrors CompactionIterator's check: for accord-enabled or accord-migrating tables, nowInSec is set to controller.gcBefore instead of the caller's nowInSec. AccordTableDifferentialCompactionTest pins the differential for such a table.
- SSTableCursorWriter.currentOffsetInPartition had zero callers: its logic moved into BigCursorIndexWriter when the index-writer seam was extracted, but the dead private method was left behind. - Five imports in SSTableCursorWriter (Ints, FSWriteError, RowIndexEntry, BloomFilter, ByteArrayUtil) are unused: BloomFilter/RowIndexEntry usage moved into BigCursorIndexWriter along with the rest of the index machinery. - DifferentialCompactionTester recompiled the "expired" field's normalization regex on every dump line instead of once; now a static final Pattern. - CursorCompactor.Purger took a nowInSec constructor parameter and stored it in a field that was never read; shouldPurge() only ever uses controller.gcBefore. Removed the dead field and parameter. No behavior change.
…sence from sstable headers
After ALTER TABLE ... DROP of a column, older sstables can still carry cells for it with
timestamps at or before the drop. The iterator path discards those cells at
deserialization (UnfilteredSerializer.readSimpleColumn, via DeserializationHelper.isDropped);
the cursor path did not, so dropped data could survive cursor compaction and reappear if
the same column name were re-added later. CellCursor now tracks each column's drop
horizon (droppedTimesArray, built once per superset) and discards a cell's value when its
timestamp is at or before that horizon, mirroring the iterator exactly.
DroppedColumnDifferentialCompactionTest pins dropped regular columns, cells newer than
the drop surviving, a drop-then-re-add resurrection shape, and dropped static columns.
Also fixes hasStaticColumns: it was read from current table metadata, but after ALTER
TABLE ... DROP of the last static column, current metadata has no static columns while
older input sstables legitimately still carry static rows. It's now derived from the
union of the input sstables' own headers instead.
The dropped-column filter initially used a plain boolean return from
CellCursor.readCellHeader, conflating two distinct outcomes: a genuine valueless cell
(tombstone) versus no cell at all (every remaining column in the row was
dropped-filtered). When a dropped column was also a row's last column, the reader
returned false without clearing cellFlags, leaving CursorCompactor.mergeCells reading a
stale hasValue=true flag against a state that said no value was available ("Flags and
state contradict"). Fixed with a tri-state return (-1/0/1): 1 = has value, 0 = valueless
cell (still a real cell to merge), -1 = no cell remains at all. The outer state machine
handles -1 by skipping straight past the row via continueReading(), while 0 keeps the
existing last-column end-of-cells handling. Verified by rerunning the full differential
suite, including DroppedColumnDifferentialCompactionTest.
CellCursor.cellDroppedTime was a field but only ever set and read within
readCellHeader() itself; made it a local variable.
…lass javadoc Fixed "implmentation" -> "implementation" and "Keep tracks" -> "Keeps track", normalized inconsistent indentation in the javadoc block, and added an explicit mention that complex (collection/UDT) and counter columns are unsupported, pointing at isSupported/ unsupportedMetadata for the full gate list. No code change.
…ilding code - CursorCompactionAllocationGateTest: extract withMeasurementEnv (the repeated preemptive-open/cursor-enabled save-set-restore boilerplate in every @test), measureBest (the warmup/measured min-allocation loop), captureLastInputBytes, and dumpAllocationProfile (the JFR recording/dump block shared by the three diagnostic tests). No behavior change. - DifferentialCompactionTester: move the allJson helper (duplicated identically in PartialSetDifferentialCompactionTest and PurgeBoundaryDifferentialCompactionTest) to the shared base class and delete both copies. - RandomDifferentialCompactionTest: extract appendNames/appendEqPredicates/appendPlaceholders for the column-name-list and "name = ?" predicate-list building repeated across insertStmt/pkOnlyInsertStmt/updateStmt/cellDeleteStmt/rangeDeleteStmt/deleteStmt. Verified by rerunning the full differential suite (all 15 test classes).
…rence CursorCompactor.mergePartition tracked the partition's last-written unfiltered via a direct reference to the source cursor's descriptor (sstableCursors[0].unfiltered()). That descriptor is reusable and gets overwritten in place as soon as the cursor reads its next unfiltered -- including ones that are subsequently merged away or purged and never actually written. By the time the partition closed and updateClusteringMetadata was called, the reference could describe a clustering that was never written, corrupting the sstable's min/max clustering metadata. Fixed by copying the clustering into a dedicated, owned ClusteringDescriptor at the moment something is genuinely written, rather than holding a reference into a cursor's mutable scratch state. SSTableCursorWriter.updateClusteringMetadata now takes a ClusteringDescriptor instead of an UnfilteredDescriptor to make that ownership explicit. Found by an extended randomized differential soak run (hundreds of examples rather than the default handful): a single-byte Statistics.db divergence between the iterator and cursor paths that the logical JSON dump and coarse stats summary both missed, since neither checks min/max clustering metadata directly.
…reaks The same-timestamp value tie-break in mergeCells compared the WIRE form of the two candidate values, which for variable-length types carries a leading length vint. That vint's leading byte encodes the value's length, so comparing from offset 0 orders candidates by LENGTH first rather than by their actual content -- the reference rule (Cells.resolveRegular, via ValueAccessor.compare) is a plain unsigned lexicographic comparison over the RAW bytes only. A tie between two different-length values could therefore pick the wrong winner. Fixed by skipping the leading vint before comparing, for variable-length types only (fixed-length types carry no vint and are always equal-length, so the skip is 0 there). timestampTiesDifferentLengthValues pins both directions of the bug and the 1-byte/2-byte vint length boundary; the existing timestampTies scenario used equal-length values and could not see the divergence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds multiple layers of testing around cursor compaction to ensure it generates the same exact outputs as the iterator path.