mirror of https://github.com/apache/cassandra
Respect clustering reversal when comparing absent components
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.
This commit is contained in:
parent
6dbe123751
commit
85d05c30f7
|
|
@ -245,7 +245,17 @@ public class ClusteringComparator implements Comparator<Clusterable>
|
|||
// compare swapped arguments to reverse the order
|
||||
int cmp = Long.compare(v2Flags, v1Flags);
|
||||
if (cmp != 0)
|
||||
{
|
||||
// Null ordering is type-independent: nulls sort first even on reversed
|
||||
// (DESC) columns, mirroring ClusteringComparator.compareComponent which
|
||||
// handles nulls before consulting the type. EMPTY vs VALUED, however, is
|
||||
// decided by the type: every base type sorts empty before values, so
|
||||
// ReversedType sorts empty AFTER values (it swaps operands around the
|
||||
// base comparison) — flip the flag-derived order for reversed columns.
|
||||
if ((v1Flags & 0b10) == 0 && (v2Flags & 0b10) == 0 && type.isReversed())
|
||||
cmp = -cmp;
|
||||
return cmp;
|
||||
}
|
||||
// null/empty == null/empty, continue...
|
||||
}
|
||||
clusteringBlock1 = clusteringBlock1 >>> 2;
|
||||
|
|
|
|||
|
|
@ -376,4 +376,67 @@ public class EdgeCaseDifferentialCompactionTest extends DifferentialCompactionTe
|
|||
|
||||
assertCursorMatchesIterator(cfs, ALLOWLIST);
|
||||
}
|
||||
|
||||
/**
|
||||
* Empty clustering values on a DESC (reversed) clustering column, found by the randomized
|
||||
* soak (seed 99303954147053). Every base type sorts empty before
|
||||
* values, so a reversed column sorts empty AFTER values (ReversedType swaps operands
|
||||
* around the base comparison). The cursor path's raw clustering comparison decided
|
||||
* empty-vs-valued purely from the serialized flag bits, ignoring reversal:
|
||||
* - same-partition variant: rows with empty and valued clusterings for the SAME pk in
|
||||
* different sstables merge in the wrong order (Data.db divergence — corruption class);
|
||||
* - cross-partition variant: the global covered-clustering max picks the wrong row
|
||||
* (Statistics.db divergence — what the soak caught).
|
||||
*/
|
||||
@Test
|
||||
public void emptyClusteringValuesDescending() throws Exception
|
||||
{
|
||||
createTable("CREATE TABLE %s (pk bigint, ck bigint, v text, PRIMARY KEY (pk, ck)) " +
|
||||
"WITH CLUSTERING ORDER BY (ck DESC)");
|
||||
ColumnFamilyStore cfs = getCurrentColumnFamilyStore();
|
||||
cfs.disableAutoCompaction();
|
||||
|
||||
// sstable 1: valued clusterings for pk 1 (same-partition variant) and pk 2 (the
|
||||
// lexically-largest valued rows, cross-partition variant)
|
||||
for (long ck = 1; ck <= 5; ck++)
|
||||
{
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 1L, ck, "p1v" + ck);
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 2L, ck * 1000, "p2v" + ck);
|
||||
}
|
||||
flush();
|
||||
|
||||
// sstable 2: EMPTY clustering values — same partition as pk 1's valued rows (the
|
||||
// merge must order empty AFTER values under DESC), plus an empty-only partition
|
||||
// (the global max clustering must be the empty value, not pk 2's large bigints)
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 1L, ByteBufferUtil.EMPTY_BYTE_BUFFER, "p1empty");
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 3L, ByteBufferUtil.EMPTY_BYTE_BUFFER, "p3empty");
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 1L, 3L, "p1v3-overwrite");
|
||||
flush();
|
||||
|
||||
assertCursorMatchesIterator(cfs, ALLOWLIST);
|
||||
}
|
||||
|
||||
/** ASC counterpart of emptyClusteringValuesDescending: empty sorts BEFORE values on a
|
||||
* non-reversed column; pins the unflipped flag ordering. */
|
||||
@Test
|
||||
public void emptyClusteringValuesAscending() throws Exception
|
||||
{
|
||||
createTable("CREATE TABLE %s (pk bigint, ck bigint, v text, PRIMARY KEY (pk, ck))");
|
||||
ColumnFamilyStore cfs = getCurrentColumnFamilyStore();
|
||||
cfs.disableAutoCompaction();
|
||||
|
||||
for (long ck = 1; ck <= 5; ck++)
|
||||
{
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 1L, ck, "p1v" + ck);
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 2L, -ck * 1000, "p2v" + ck);
|
||||
}
|
||||
flush();
|
||||
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 1L, ByteBufferUtil.EMPTY_BYTE_BUFFER, "p1empty");
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 3L, ByteBufferUtil.EMPTY_BYTE_BUFFER, "p3empty");
|
||||
execute("INSERT INTO %s (pk, ck, v) VALUES (?, ?, ?)", 1L, 3L, "p1v3-overwrite");
|
||||
flush();
|
||||
|
||||
assertCursorMatchesIterator(cfs, ALLOWLIST);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue