HBASE-30036 Skip redundant delete markers during flush and minor compaction (#7993) (#8036)

Add DeleteTracker.isRedundantDelete() to detect when a delete marker is
already covered by a previously tracked delete of equal or broader scope.
ScanDeleteTracker implements this for all four delete types:
- DeleteFamily/DeleteFamilyVersion: covered by a tracked DeleteFamily
- DeleteColumn/Delete: covered by a tracked DeleteFamily or DeleteColumn

MinorCompactionScanQueryMatcher calls this check before including a
delete marker, returning SEEK_NEXT_COL to skip past all remaining cells
covered by the previously tracked delete.

Compatible with KEEP_DELETED_CELLS. When set to TRUE, trackDelete() does
not populate the delete tracker, so isRedundantDelete() always returns
false and all markers are retained.

Signed-off-by: Charles Connell <cconnell@apache.org>

Author: junegunn

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/DeleteTracker.java

@@ -83,6 +83,20 @@ enum DeleteResult {
                    // deleted in strong semantics of versions(See MvccTracker)
   }
 
+  /**
+   * Check if the given delete marker is redundant, i.e., it is already covered by a previously
+   * tracked delete of equal or broader scope. A DeleteFamily is redundant if a DeleteFamily with a
+   * higher timestamp was already seen. A DeleteColumn is redundant if a DeleteColumn for the same
+   * qualifier with a higher timestamp, or a DeleteFamily with a higher timestamp, was already seen.
+   * <p>
+   * This is a read-only check with no side effects on tracker state.
+   * @param cell the delete marker cell to check
+   * @return true if the delete marker is redundant and can be skipped
+   */
+  default boolean isRedundantDelete(ExtendedCell cell) {
+    return false;
+  }
+
   /**
    * Return the comparator passed to this delete tracker
    * @return the cell comparator

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/MinorCompactionScanQueryMatcher.java

@@ -19,6 +19,7 @@
 
 import java.io.IOException;
 import org.apache.hadoop.hbase.ExtendedCell;
+import org.apache.hadoop.hbase.KeyValue;
 import org.apache.hadoop.hbase.PrivateCellUtil;
 import org.apache.hadoop.hbase.regionserver.ScanInfo;
 import org.apache.yetus.audience.InterfaceAudience;
@@ -47,6 +48,19 @@ public MatchCode match(ExtendedCell cell) throws IOException {
         // we should not use this delete marker to mask any cell yet.
         return MatchCode.INCLUDE;
       }
+      // Check before tracking: an older DeleteColumn or DeleteFamily is redundant if a newer
+      // one of equal or broader scope was already seen. Must check before trackDelete() since
+      // that overwrites tracker state. Seek past remaining cells for this column/row since
+      // they are all covered by the previously tracked delete.
+      if (deletes.isRedundantDelete(cell)) {
+        // Skip seeking for deletes with empty qualifier, not to skip a subsequent
+        // DeleteFamily marker that covers other qualifiers. DeleteFamily itself can seek
+        // safely because all remaining empty-qualifier cells are redundant under it.
+        if (cell.getQualifierLength() == 0 && typeByte != KeyValue.Type.DeleteFamily.getCode()) {
+          return MatchCode.SKIP;
+        }
+        return columns.getNextRowOrNextColumn(cell);
+      }
       trackDelete(cell);
       return MatchCode.INCLUDE;
     }

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/ScanDeleteTracker.java

@@ -142,6 +142,28 @@ public DeleteResult isDeleted(ExtendedCell cell) {
     return DeleteResult.NOT_DELETED;
   }
 
+  @Override
+  public boolean isRedundantDelete(ExtendedCell cell) {
+    byte type = cell.getTypeByte();
+    boolean coveredByFamily = hasFamilyStamp && cell.getTimestamp() <= familyStamp;
+
+    if (
+      type == KeyValue.Type.DeleteFamily.getCode()
+        || type == KeyValue.Type.DeleteFamilyVersion.getCode()
+    ) {
+      return coveredByFamily;
+    }
+
+    boolean coveredByColumn =
+      deleteCell != null && deleteType == KeyValue.Type.DeleteColumn.getCode()
+        && CellUtil.matchingQualifier(cell, deleteCell) && cell.getTimestamp() <= deleteTimestamp;
+
+    if (type == KeyValue.Type.DeleteColumn.getCode() || type == KeyValue.Type.Delete.getCode()) {
+      return coveredByFamily || coveredByColumn;
+    }
+    return false;
+  }
+
   @Override
   public boolean isEmpty() {
     return deleteCell == null && !hasFamilyStamp && familyVersionStamps.isEmpty();

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFileWriter.java

@@ -176,8 +176,15 @@ public void testCompactedFiles() throws Exception {
       stores[0].getStorefilesCount());
 
     regions[1].compact(false);
-    assertEquals(flushCount - stores[1].getCompactedFiles().size() + 2,
-      stores[1].getStorefilesCount());
+    // HBASE-30036 skips redundant delete markers during minor compaction, so the historical
+    // file may end up empty and not be created. The count can be +1 or +2.
+    int minorCompactedCount = stores[1].getStorefilesCount();
+    int expectedMin = flushCount - stores[1].getCompactedFiles().size() + 1;
+    int expectedMax = flushCount - stores[1].getCompactedFiles().size() + 2;
+    assertTrue(
+      "Expected store file count between " + expectedMin + " and " + expectedMax + " but was "
+        + minorCompactedCount,
+      minorCompactedCount >= expectedMin && minorCompactedCount <= expectedMax);
 
     verifyCells();
 

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/querymatcher/TestCompactionScanQueryMatcher.java

@@ -18,6 +18,7 @@
 package org.apache.hadoop.hbase.regionserver.querymatcher;
 
 import static org.apache.hadoop.hbase.regionserver.querymatcher.ScanQueryMatcher.MatchCode.INCLUDE;
+import static org.apache.hadoop.hbase.regionserver.querymatcher.ScanQueryMatcher.MatchCode.SEEK_NEXT_COL;
 import static org.apache.hadoop.hbase.regionserver.querymatcher.ScanQueryMatcher.MatchCode.SKIP;
 import static org.junit.Assert.assertEquals;
 
@@ -74,6 +75,121 @@ public void testMatch_PartialRangeDropDeletes() throws Exception {
     testDropDeletes(row2, row3, new byte[][] { row1, row1 }, INCLUDE, INCLUDE);
   }
 
+  /**
+   * Test redundant delete marker handling with COMPACT_RETAIN_DELETES. Cells are auto-generated
+   * from the given types with decrementing timestamps.
+   */
+  @Test
+  public void testSkipsRedundantDeleteMarkers() throws IOException {
+    // Interleaved DeleteColumn + Put. First DC included, put triggers SEEK_NEXT_COL.
+    assertRetainDeletes(new Type[] { Type.DeleteColumn, Type.Put, Type.DeleteColumn }, INCLUDE,
+      SEEK_NEXT_COL);
+
+    // Contiguous DeleteColumn. First included, rest redundant.
+    assertRetainDeletes(new Type[] { Type.DeleteColumn, Type.DeleteColumn, Type.DeleteColumn },
+      INCLUDE, SEEK_NEXT_COL, SEEK_NEXT_COL);
+
+    // Contiguous DeleteFamily. First included, rest redundant.
+    assertRetainDeletes(new Type[] { Type.DeleteFamily, Type.DeleteFamily, Type.DeleteFamily },
+      INCLUDE, SEEK_NEXT_COL, SEEK_NEXT_COL);
+
+    // DF + DFV interleaved. DF included, DFV redundant (SKIP because empty qualifier),
+    // older DF redundant (SEEK_NEXT_COL), older DFV redundant (SKIP).
+    assertRetainDeletes(new Type[] { Type.DeleteFamily, Type.DeleteFamilyVersion, Type.DeleteFamily,
+      Type.DeleteFamilyVersion }, INCLUDE, SKIP, SEEK_NEXT_COL, SKIP);
+
+    // Delete (version) covered by DeleteColumn.
+    assertRetainDeletes(new Type[] { Type.DeleteColumn, Type.Delete, Type.Delete, Type.Delete },
+      INCLUDE, SEEK_NEXT_COL, SEEK_NEXT_COL, SEEK_NEXT_COL);
+
+    // KEEP_DELETED_CELLS=TRUE: all markers retained.
+    assertRetainDeletes(KeepDeletedCells.TRUE,
+      new Type[] { Type.DeleteColumn, Type.DeleteColumn, Type.DeleteColumn }, INCLUDE, INCLUDE,
+      INCLUDE);
+  }
+
+  /**
+   * Redundant column-level deletes with empty qualifier must not seek past a subsequent
+   * DeleteFamily. getKeyForNextColumn treats empty qualifier as "no column" and returns
+   * SEEK_NEXT_ROW, which would skip the DF and all remaining cells in the row.
+   */
+  @Test
+  public void testEmptyQualifierDeleteDoesNotSkipDeleteFamily() throws IOException {
+    byte[] emptyQualifier = HConstants.EMPTY_BYTE_ARRAY;
+
+    // DC(empty) + DC(empty) redundant + DF must still be reachable.
+    assertRetainDeletes(emptyQualifier,
+      new Type[] { Type.DeleteColumn, Type.DeleteColumn, Type.DeleteFamily }, INCLUDE, SKIP,
+      INCLUDE);
+
+    // DC(empty) + Delete(empty) redundant + DF must still be reachable.
+    assertRetainDeletes(emptyQualifier,
+      new Type[] { Type.DeleteColumn, Type.Delete, Type.DeleteFamily }, INCLUDE, SKIP, INCLUDE);
+  }
+
+  private void assertRetainDeletes(Type[] types, MatchCode... expected) throws IOException {
+    assertRetainDeletes(KeepDeletedCells.FALSE, types, expected);
+  }
+
+  private void assertRetainDeletes(byte[] qualifier, Type[] types, MatchCode... expected)
+    throws IOException {
+    assertRetainDeletes(KeepDeletedCells.FALSE, qualifier, types, expected);
+  }
+
+  /**
+   * Build cells from the given types with decrementing timestamps (same ts for adjacent
+   * family-level and column-level types at the same position). Family-level types (DeleteFamily,
+   * DeleteFamilyVersion) use empty qualifier; others use col1.
+   */
+  private void assertRetainDeletes(KeepDeletedCells keepDeletedCells, Type[] types,
+    MatchCode... expected) throws IOException {
+    assertRetainDeletes(keepDeletedCells, null, types, expected);
+  }
+
+  /**
+   * Build cells from the given types with decrementing timestamps. If qualifier is null,
+   * family-level types use empty qualifier and others use col1. If qualifier is specified, all
+   * types use that qualifier.
+   */
+  private void assertRetainDeletes(KeepDeletedCells keepDeletedCells, byte[] qualifier,
+    Type[] types, MatchCode... expected) throws IOException {
+    long now = EnvironmentEdgeManager.currentTime();
+    ScanInfo scanInfo = new ScanInfo(this.conf, fam1, 0, 1, ttl, keepDeletedCells,
+      HConstants.DEFAULT_BLOCKSIZE, 0, rowComparator, false);
+    CompactionScanQueryMatcher qm =
+      CompactionScanQueryMatcher.create(scanInfo, ScanType.COMPACT_RETAIN_DELETES, 0L,
+        HConstants.OLDEST_TIMESTAMP, HConstants.OLDEST_TIMESTAMP, now, null, null, null);
+    qm.setToNewRow(KeyValueUtil.createFirstOnRow(row1));
+
+    long ts = now;
+    List<MatchCode> actual = new ArrayList<>(expected.length);
+    for (int i = 0; i < types.length; i++) {
+      byte[] qual;
+      if (qualifier != null) {
+        qual = qualifier;
+      } else {
+        boolean familyLevel = types[i] == Type.DeleteFamily || types[i] == Type.DeleteFamilyVersion;
+        qual = familyLevel ? HConstants.EMPTY_BYTE_ARRAY : col1;
+      }
+      KeyValue kv = types[i] == Type.Put
+        ? new KeyValue(row1, fam1, qual, ts, types[i], data)
+        : new KeyValue(row1, fam1, qual, ts, types[i]);
+      actual.add(qm.match(kv));
+      if (actual.size() >= expected.length) {
+        break;
+      }
+      // Decrement ts for next cell, but keep same ts when the next type has lower type code
+      // at the same logical position (e.g. DF then DFV at the same timestamp).
+      if (i + 1 < types.length && types[i + 1].getCode() < types[i].getCode()) {
+        continue;
+      }
+      ts--;
+    }
+    for (int i = 0; i < expected.length; i++) {
+      assertEquals("Mismatch at index " + i, expected[i], actual.get(i));
+    }
+  }
+
   private void testDropDeletes(byte[] from, byte[] to, byte[][] rows, MatchCode... expected)
     throws IOException {
     long now = EnvironmentEdgeManager.currentTime();