You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/12/18 10:43:54 UTC
[GitHub] [hbase] anoopsjohn commented on a change in pull request #2776: Using ContiguousCellFormat as a marker alone
anoopsjohn commented on a change in pull request #2776:
URL: https://github.com/apache/hbase/pull/2776#discussion_r545552619
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
##########
@@ -1598,16 +1621,21 @@ public static boolean matchingRows(final Cell left, final Cell right) {
* @return True if same row and column.
*/
public static boolean matchingRowColumn(final Cell left, final Cell right) {
- if ((left.getRowLength() + left.getFamilyLength()
- + left.getQualifierLength()) != (right.getRowLength() + right.getFamilyLength()
- + right.getQualifierLength())) {
+ short lrowlength = left.getRowLength();
+ short rrowlength = right.getRowLength();
+ byte lfamlength = left.getFamilyLength();
+ byte rfamlength = right.getFamilyLength();
+ int lqlength = left.getQualifierLength();
+ int rqlength = right.getQualifierLength();
+ // match length
+ if ((lrowlength + lfamlength + lqlength) != (rrowlength + rfamlength + rqlength)) {
Review comment:
More efficient way is ((lrowlength == rrowlength) && (lfamlength == rfamlength) && (lqlength == rqlength) ?
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -57,29 +57,287 @@ public final int compare(final Cell a, final Cell b) {
}
@Override
- public int compare(final Cell a, final Cell b, boolean ignoreSequenceid) {
-
+ public int compare(final Cell l, final Cell r, boolean ignoreSequenceid) {
int diff = 0;
// "Peel off" the most common path.
- if (a instanceof ByteBufferKeyValue && b instanceof ByteBufferKeyValue) {
- diff = BBKVComparator.compare((ByteBufferKeyValue)a, (ByteBufferKeyValue)b, ignoreSequenceid);
+ if (l instanceof KeyValue && r instanceof KeyValue) {
+ diff = compareKeyValues((KeyValue) l, (KeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof KeyValue && r instanceof ByteBufferKeyValue) {
+ diff = compareKVVsBBKV((KeyValue) l, (ByteBufferKeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof ByteBufferKeyValue && r instanceof KeyValue) {
+ diff = compareKVVsBBKV((KeyValue) r, (ByteBufferKeyValue) l);
+ if (diff != 0) {
+ // negate- Findbugs will complain?
+ return -diff;
+ }
+ // if ignoreSequenceid then the diff will be 0.
+ return ignoreSequenceid ? diff : Long.compare(r.getSequenceId(), l.getSequenceId());
Review comment:
Is this line needed here? All other branches fall back to the last line in this method. Any specific reason for doing it here?
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -57,29 +57,287 @@ public final int compare(final Cell a, final Cell b) {
}
@Override
- public int compare(final Cell a, final Cell b, boolean ignoreSequenceid) {
-
+ public int compare(final Cell l, final Cell r, boolean ignoreSequenceid) {
int diff = 0;
// "Peel off" the most common path.
- if (a instanceof ByteBufferKeyValue && b instanceof ByteBufferKeyValue) {
- diff = BBKVComparator.compare((ByteBufferKeyValue)a, (ByteBufferKeyValue)b, ignoreSequenceid);
+ if (l instanceof KeyValue && r instanceof KeyValue) {
+ diff = compareKeyValues((KeyValue) l, (KeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof KeyValue && r instanceof ByteBufferKeyValue) {
+ diff = compareKVVsBBKV((KeyValue) l, (ByteBufferKeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof ByteBufferKeyValue && r instanceof KeyValue) {
+ diff = compareKVVsBBKV((KeyValue) r, (ByteBufferKeyValue) l);
+ if (diff != 0) {
+ // negate- Findbugs will complain?
+ return -diff;
+ }
+ // if ignoreSequenceid then the diff will be 0.
+ return ignoreSequenceid ? diff : Long.compare(r.getSequenceId(), l.getSequenceId());
+ } else if (l instanceof ByteBufferKeyValue && r instanceof ByteBufferKeyValue) {
+ diff = compareBBKV((ByteBufferKeyValue) l, (ByteBufferKeyValue) r);
if (diff != 0) {
return diff;
}
} else {
- diff = compareRows(a, b);
+ int leftRowLength = l.getRowLength();
+ int rightRowLength = r.getRowLength();
+ diff = compareRows(l, leftRowLength, r, rightRowLength);
if (diff != 0) {
return diff;
}
- diff = compareWithoutRow(a, b);
+ diff = compareWithoutRow(l, r);
if (diff != 0) {
return diff;
}
}
-
// Negate following comparisons so later edits show up first mvccVersion: later sorts first
- return ignoreSequenceid? diff: Long.compare(b.getSequenceId(), a.getSequenceId());
+ return ignoreSequenceid ? diff : Long.compare(r.getSequenceId(), l.getSequenceId());
+ }
+
+ private static int compareBBKV(final ByteBufferKeyValue left, final ByteBufferKeyValue right) {
+ int diff;
+ // Compare Rows. Cache row length.
+ int leftRowLength = left.getRowLength();
+ int rightRowLength = right.getRowLength();
+ diff = ByteBufferUtils.compareTo(left.getRowByteBuffer(), left.getRowPosition(),
+ leftRowLength, right.getRowByteBuffer(), right.getRowPosition(), rightRowLength);
+ if (diff != 0) {
+ return diff;
+ }
+
+ // If the column is not specified, the "minimum" key type appears as latest in the sorted
+ // order, regardless of the timestamp. This is used for specifying the last key/value in a
+ // given row, because there is no "lexicographically last column" (it would be infinitely
+ // long).
+ // The "maximum" key type does not need this behavior. Copied from KeyValue. This is bad in
+ // that
+ // we can't do memcmp w/ special rules like this.
+ // TODO: Is there a test for this behavior?
+ int leftFamilyLengthPosition = left.getFamilyLengthPosition(leftRowLength);
+ int leftFamilyLength = left.getFamilyLength(leftFamilyLengthPosition);
+ int leftKeyLength = left.getKeyLength();
+ int leftQualifierLength =
+ left.getQualifierLength(leftKeyLength, leftRowLength, leftFamilyLength);
+
+ // No need of left row length below here.
+
+ byte leftType = left.getTypeByte(leftKeyLength);
+ if (leftFamilyLength + leftQualifierLength == 0
+ && leftType == KeyValue.Type.Minimum.getCode()) {
+ // left is "bigger", i.e. it appears later in the sorted order
+ return 1;
+ }
+
+ int rightFamilyLengthPosition = right.getFamilyLengthPosition(rightRowLength);
+ int rightFamilyLength = right.getFamilyLength(rightFamilyLengthPosition);
+ int rightKeyLength = right.getKeyLength();
+ int rightQualifierLength =
+ right.getQualifierLength(rightKeyLength, rightRowLength, rightFamilyLength);
+
+ // No need of right row length below here.
+
+ byte rightType = right.getTypeByte(rightKeyLength);
+ if (rightFamilyLength + rightQualifierLength == 0
+ && rightType == KeyValue.Type.Minimum.getCode()) {
+ return -1;
+ }
+
+ // Compare families.
+ int leftFamilyPosition = left.getFamilyPosition(leftFamilyLengthPosition);
+ int rightFamilyPosition = right.getFamilyPosition(rightFamilyLengthPosition);
+ diff = ByteBufferUtils.compareTo(left.getFamilyByteBuffer(), leftFamilyPosition,
+ leftFamilyLength, right.getFamilyByteBuffer(), rightFamilyPosition, rightFamilyLength);
+ if (diff != 0) {
+ return diff;
+ }
+
+ // Compare qualifiers
+ diff = ByteBufferUtils.compareTo(left.getQualifierByteBuffer(),
+ left.getQualifierPosition(leftFamilyPosition, leftFamilyLength), leftQualifierLength,
+ right.getQualifierByteBuffer(),
+ right.getQualifierPosition(rightFamilyPosition, rightFamilyLength), rightQualifierLength);
+ if (diff != 0) {
+ return diff;
+ }
+
+ // Timestamps.
+ // Swap order we pass into compare so we get DESCENDING order.
+ diff = Long.compare(right.getTimestamp(rightKeyLength), left.getTimestamp(leftKeyLength));
Review comment:
Not suggesting for doing in this patch.
But may be we can avoid the long decodes and do compare but instead bytes level compare we can do. That too 1st with lower order bytes. We are not interested in the exact diff here anyways. A compare value of 0 or +ve number of -ve number. Add a TODO so that we can come back and do some JMH tests and if that helps, we can incorporate in all places. WDYT?
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -125,38 +442,174 @@ public final int compareFamilies(Cell left, Cell right) {
right.getFamilyArray(), right.getFamilyOffset(), right.getFamilyLength());
}
+ static int compareQualifiers(KeyValue left, KeyValue right) {
+ // NOTE: Same method is in CellComparatorImpl, also private, not shared, intentionally. Not
+ // sharing gets us a few percent more throughput in compares. If changes here or there, make
+ // sure done in both places.
+ // Compare Rows. Cache row length.
+ int leftRowLength = left.getRowLength();
+ int rightRowLength = right.getRowLength();
+
+ int leftFamilyLengthPosition = left.getFamilyLengthPosition(leftRowLength);
+ byte leftFamilyLength = left.getFamilyLength(leftFamilyLengthPosition);
+ int leftKeyLength = left.getKeyLength();
+ int leftQualifierLength =
+ left.getQualifierLength(leftKeyLength, leftRowLength, leftFamilyLength);
+
+ // No need of left row length below here.
+
+ int rightFamilyLengthPosition = right.getFamilyLengthPosition(rightRowLength);
+ byte rightFamilyLength = right.getFamilyLength(rightFamilyLengthPosition);
+ int rightKeyLength = right.getKeyLength();
+ int rightQualifierLength =
+ right.getQualifierLength(rightKeyLength, rightRowLength, rightFamilyLength);
+
+ // Compare families.
+ int leftFamilyOffset = left.getFamilyOffset(leftFamilyLengthPosition);
+ int rightFamilyOffset = right.getFamilyOffset(rightFamilyLengthPosition);
+
+ // Compare qualifiers
+ return Bytes.compareTo(left.getQualifierArray(), leftFamilyOffset + leftFamilyLength,
+ leftQualifierLength, right.getQualifierArray(), rightFamilyOffset + rightFamilyLength,
+ rightQualifierLength);
+ }
+
+ static int compareQualifiers(KeyValue left, ByteBufferKeyValue right) {
+ // NOTE: Same method is in CellComparatorImpl, also private, not shared, intentionally. Not
+ // sharing gets us a few percent more throughput in compares. If changes here or there, make
+ // sure done in both places.
+ // Compare Rows. Cache row length.
+ int leftRowLength = left.getRowLength();
+ int rightRowLength = right.getRowLength();
+
+ int leftFamilyLengthPosition = left.getFamilyLengthPosition(leftRowLength);
+ byte leftFamilyLength = left.getFamilyLength(leftFamilyLengthPosition);
+ int leftKeyLength = left.getKeyLength();
+ int leftQualifierLength =
+ left.getQualifierLength(leftKeyLength, leftRowLength, leftFamilyLength);
+
+ // No need of left row length below here.
+
+ int rightFamilyLengthPosition = right.getFamilyLengthPosition(rightRowLength);
+ byte rightFamilyLength = right.getFamilyLength(rightFamilyLengthPosition);
+ int rightKeyLength = right.getKeyLength();
+ int rightQualifierLength =
+ right.getQualifierLength(rightKeyLength, rightRowLength, rightFamilyLength);
+
+ // Compare families.
+ int leftFamilyOffset = left.getFamilyOffset(leftFamilyLengthPosition);
+ int rightFamilyPosition = right.getFamilyPosition(rightFamilyLengthPosition);
+
+ // Compare qualifiers
+ return ByteBufferUtils.compareTo(left.getQualifierArray(),
+ leftFamilyOffset + leftFamilyLength, leftQualifierLength, right.getQualifierByteBuffer(),
+ rightFamilyPosition + rightFamilyLength, rightQualifierLength);
+ }
+
+ static int compareQualifiers(ByteBufferKeyValue left, KeyValue right) {
+ // NOTE: Same method is in CellComparatorImpl, also private, not shared, intentionally. Not
+ // sharing gets us a few percent more throughput in compares. If changes here or there, make
+ // sure done in both places.
+ // Compare Rows. Cache row length.
+ int leftRowLength = left.getRowLength();
+ int rightRowLength = right.getRowLength();
+
+ int leftFamilyLengthPosition = left.getFamilyLengthPosition(leftRowLength);
+ byte leftFamilyLength = left.getFamilyLength(leftFamilyLengthPosition);
+ int leftKeyLength = left.getKeyLength();
+ int leftQualifierLength =
+ left.getQualifierLength(leftKeyLength, leftRowLength, leftFamilyLength);
+
+ // No need of left row length below here.
+
+ int rightFamilyLengthPosition = right.getFamilyLengthPosition(rightRowLength);
+ byte rightFamilyLength = right.getFamilyLength(rightFamilyLengthPosition);
+ int rightKeyLength = right.getKeyLength();
+ int rightQualifierLength =
+ right.getQualifierLength(rightKeyLength, rightRowLength, rightFamilyLength);
+
+ // Compare families.
+ int leftFamilyPosition = left.getFamilyPosition(leftFamilyLengthPosition);
+ int rightFamilyOffset = right.getFamilyOffset(rightFamilyLengthPosition);
+
+ // Compare qualifiers
+ return ByteBufferUtils.compareTo(left.getQualifierByteBuffer(),
+ leftFamilyPosition + leftFamilyLength, leftQualifierLength, right.getQualifierArray(),
+ rightFamilyOffset + rightFamilyLength, rightQualifierLength);
+ }
+
+ static int compareQualifiers(ByteBufferKeyValue left, ByteBufferKeyValue right) {
+ // NOTE: Same method is in CellComparatorImpl, also private, not shared, intentionally. Not
+ // sharing gets us a few percent more throughput in compares. If changes here or there, make
+ // sure done in both places.
+ // Compare Rows. Cache row length.
+ int leftRowLength = left.getRowLength();
+ int rightRowLength = right.getRowLength();
+
+ int leftFamilyLengthPosition = left.getFamilyLengthPosition(leftRowLength);
+ byte leftFamilyLength = left.getFamilyLength(leftFamilyLengthPosition);
+ int leftKeyLength = left.getKeyLength();
+ int leftQualifierLength =
+ left.getQualifierLength(leftKeyLength, leftRowLength, leftFamilyLength);
+
+ // No need of left row length below here.
+
+ int rightFamilyLengthPosition = right.getFamilyLengthPosition(rightRowLength);
+ byte rightFamilyLength = right.getFamilyLength(rightFamilyLengthPosition);
+ int rightKeyLength = right.getKeyLength();
+ int rightQualifierLength =
+ right.getQualifierLength(rightKeyLength, rightRowLength, rightFamilyLength);
+
+ // Compare families.
+ int leftFamilyPosition = left.getFamilyPosition(leftFamilyLengthPosition);
+ int rightFamilyPosition = right.getFamilyPosition(rightFamilyLengthPosition);
+
+ // Compare qualifiers
+ return ByteBufferUtils.compareTo(left.getQualifierByteBuffer(),
+ leftFamilyPosition + leftFamilyLength, leftQualifierLength, right.getQualifierByteBuffer(),
+ rightFamilyPosition + rightFamilyLength, rightQualifierLength);
+ }
+
/**
* Compare the qualifiers part of the left and right cells.
* @return 0 if both cells are equal, 1 if left cell is bigger than right, -1 otherwise
*/
@Override
public final int compareQualifiers(Cell left, Cell right) {
- if (left instanceof ByteBufferExtendedCell && right instanceof ByteBufferExtendedCell) {
- return ByteBufferUtils
- .compareTo(((ByteBufferExtendedCell) left).getQualifierByteBuffer(),
- ((ByteBufferExtendedCell) left).getQualifierPosition(),
- left.getQualifierLength(), ((ByteBufferExtendedCell) right).getQualifierByteBuffer(),
- ((ByteBufferExtendedCell) right).getQualifierPosition(),
- right.getQualifierLength());
- }
- if (left instanceof ByteBufferExtendedCell) {
- return ByteBufferUtils.compareTo(((ByteBufferExtendedCell) left).getQualifierByteBuffer(),
+ if ((left instanceof ByteBufferKeyValue) && (right instanceof ByteBufferKeyValue)) {
+ return compareQualifiers((ByteBufferKeyValue) left, (ByteBufferKeyValue) right);
+ } else if ((left instanceof KeyValue) && (right instanceof KeyValue)) {
+ return compareQualifiers((KeyValue) left, (KeyValue) right);
+ } else if ((left instanceof KeyValue) && (right instanceof ByteBufferKeyValue)) {
+ return compareQualifiers((KeyValue) left, (ByteBufferKeyValue) right);
+ } else if ((left instanceof ByteBufferKeyValue) && (right instanceof KeyValue)) {
+ return compareQualifiers((ByteBufferKeyValue) left, (KeyValue) right);
+ } else {
+ if (left instanceof ByteBufferExtendedCell && right instanceof ByteBufferExtendedCell) {
Review comment:
We have ByteBufferKeyValue which extends ByteBufferExtendedCell and other sub types also. May be its time to reduce these types. I see we have KeyOnlyByteBufferExtendedCell.. But there is some ways.. Same way will see how we can have the DBE seekers also returning Cells of type KV or BBKV. So the optimizations will be used in case of scan over DBEed table:cf. Will do it in a followup issue. I can do that.
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -57,29 +57,287 @@ public final int compare(final Cell a, final Cell b) {
}
@Override
- public int compare(final Cell a, final Cell b, boolean ignoreSequenceid) {
-
+ public int compare(final Cell l, final Cell r, boolean ignoreSequenceid) {
int diff = 0;
// "Peel off" the most common path.
- if (a instanceof ByteBufferKeyValue && b instanceof ByteBufferKeyValue) {
- diff = BBKVComparator.compare((ByteBufferKeyValue)a, (ByteBufferKeyValue)b, ignoreSequenceid);
+ if (l instanceof KeyValue && r instanceof KeyValue) {
+ diff = compareKeyValues((KeyValue) l, (KeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof KeyValue && r instanceof ByteBufferKeyValue) {
+ diff = compareKVVsBBKV((KeyValue) l, (ByteBufferKeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof ByteBufferKeyValue && r instanceof KeyValue) {
+ diff = compareKVVsBBKV((KeyValue) r, (ByteBufferKeyValue) l);
+ if (diff != 0) {
+ // negate- Findbugs will complain?
+ return -diff;
+ }
+ // if ignoreSequenceid then the diff will be 0.
+ return ignoreSequenceid ? diff : Long.compare(r.getSequenceId(), l.getSequenceId());
+ } else if (l instanceof ByteBufferKeyValue && r instanceof ByteBufferKeyValue) {
+ diff = compareBBKV((ByteBufferKeyValue) l, (ByteBufferKeyValue) r);
if (diff != 0) {
return diff;
}
} else {
- diff = compareRows(a, b);
Review comment:
Can we extract this also to a private method pls?
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -57,29 +57,287 @@ public final int compare(final Cell a, final Cell b) {
}
@Override
- public int compare(final Cell a, final Cell b, boolean ignoreSequenceid) {
-
+ public int compare(final Cell l, final Cell r, boolean ignoreSequenceid) {
int diff = 0;
// "Peel off" the most common path.
- if (a instanceof ByteBufferKeyValue && b instanceof ByteBufferKeyValue) {
- diff = BBKVComparator.compare((ByteBufferKeyValue)a, (ByteBufferKeyValue)b, ignoreSequenceid);
+ if (l instanceof KeyValue && r instanceof KeyValue) {
+ diff = compareKeyValues((KeyValue) l, (KeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof KeyValue && r instanceof ByteBufferKeyValue) {
+ diff = compareKVVsBBKV((KeyValue) l, (ByteBufferKeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof ByteBufferKeyValue && r instanceof KeyValue) {
+ diff = compareKVVsBBKV((KeyValue) r, (ByteBufferKeyValue) l);
+ if (diff != 0) {
+ // negate- Findbugs will complain?
+ return -diff;
+ }
+ // if ignoreSequenceid then the diff will be 0.
+ return ignoreSequenceid ? diff : Long.compare(r.getSequenceId(), l.getSequenceId());
+ } else if (l instanceof ByteBufferKeyValue && r instanceof ByteBufferKeyValue) {
+ diff = compareBBKV((ByteBufferKeyValue) l, (ByteBufferKeyValue) r);
if (diff != 0) {
return diff;
}
} else {
- diff = compareRows(a, b);
+ int leftRowLength = l.getRowLength();
+ int rightRowLength = r.getRowLength();
+ diff = compareRows(l, leftRowLength, r, rightRowLength);
if (diff != 0) {
return diff;
}
- diff = compareWithoutRow(a, b);
+ diff = compareWithoutRow(l, r);
if (diff != 0) {
return diff;
}
}
-
// Negate following comparisons so later edits show up first mvccVersion: later sorts first
- return ignoreSequenceid? diff: Long.compare(b.getSequenceId(), a.getSequenceId());
+ return ignoreSequenceid ? diff : Long.compare(r.getSequenceId(), l.getSequenceId());
+ }
+
+ private static int compareBBKV(final ByteBufferKeyValue left, final ByteBufferKeyValue right) {
+ int diff;
+ // Compare Rows. Cache row length.
+ int leftRowLength = left.getRowLength();
+ int rightRowLength = right.getRowLength();
+ diff = ByteBufferUtils.compareTo(left.getRowByteBuffer(), left.getRowPosition(),
+ leftRowLength, right.getRowByteBuffer(), right.getRowPosition(), rightRowLength);
+ if (diff != 0) {
+ return diff;
+ }
+
+ // If the column is not specified, the "minimum" key type appears as latest in the sorted
+ // order, regardless of the timestamp. This is used for specifying the last key/value in a
+ // given row, because there is no "lexicographically last column" (it would be infinitely
+ // long).
+ // The "maximum" key type does not need this behavior. Copied from KeyValue. This is bad in
+ // that
+ // we can't do memcmp w/ special rules like this.
+ // TODO: Is there a test for this behavior?
+ int leftFamilyLengthPosition = left.getFamilyLengthPosition(leftRowLength);
+ int leftFamilyLength = left.getFamilyLength(leftFamilyLengthPosition);
+ int leftKeyLength = left.getKeyLength();
+ int leftQualifierLength =
+ left.getQualifierLength(leftKeyLength, leftRowLength, leftFamilyLength);
+
+ // No need of left row length below here.
+
+ byte leftType = left.getTypeByte(leftKeyLength);
+ if (leftFamilyLength + leftQualifierLength == 0
Review comment:
We can reverse the check. Mostly leftType wont be Minimum type and so we can skip the '+' operator eval. These were like this before.. But can we focus on these small small optimization also now? That would be good.
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -57,29 +57,287 @@ public final int compare(final Cell a, final Cell b) {
}
@Override
- public int compare(final Cell a, final Cell b, boolean ignoreSequenceid) {
-
+ public int compare(final Cell l, final Cell r, boolean ignoreSequenceid) {
int diff = 0;
// "Peel off" the most common path.
- if (a instanceof ByteBufferKeyValue && b instanceof ByteBufferKeyValue) {
- diff = BBKVComparator.compare((ByteBufferKeyValue)a, (ByteBufferKeyValue)b, ignoreSequenceid);
+ if (l instanceof KeyValue && r instanceof KeyValue) {
Review comment:
In read path mostly both cells will be either KV or BBKV. In write path we have BBKV now? I think some patches did that for master branch already. we create the ChunkCell or so. Is that reverted? Pls check.
So better to do both KV check and then both BBKV followed by other 2 options?
Te perf tests u did now was giving all cells are KVs right? is it possible to test for both cells as BBKV also? That will be better. May be a JMH test?
The split of private methods came good now.
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -57,29 +57,287 @@ public final int compare(final Cell a, final Cell b) {
}
@Override
- public int compare(final Cell a, final Cell b, boolean ignoreSequenceid) {
-
+ public int compare(final Cell l, final Cell r, boolean ignoreSequenceid) {
int diff = 0;
// "Peel off" the most common path.
- if (a instanceof ByteBufferKeyValue && b instanceof ByteBufferKeyValue) {
- diff = BBKVComparator.compare((ByteBufferKeyValue)a, (ByteBufferKeyValue)b, ignoreSequenceid);
+ if (l instanceof KeyValue && r instanceof KeyValue) {
+ diff = compareKeyValues((KeyValue) l, (KeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof KeyValue && r instanceof ByteBufferKeyValue) {
+ diff = compareKVVsBBKV((KeyValue) l, (ByteBufferKeyValue) r);
+ if (diff != 0) {
+ return diff;
+ }
+ } else if (l instanceof ByteBufferKeyValue && r instanceof KeyValue) {
+ diff = compareKVVsBBKV((KeyValue) r, (ByteBufferKeyValue) l);
+ if (diff != 0) {
+ // negate- Findbugs will complain?
+ return -diff;
+ }
+ // if ignoreSequenceid then the diff will be 0.
+ return ignoreSequenceid ? diff : Long.compare(r.getSequenceId(), l.getSequenceId());
+ } else if (l instanceof ByteBufferKeyValue && r instanceof ByteBufferKeyValue) {
+ diff = compareBBKV((ByteBufferKeyValue) l, (ByteBufferKeyValue) r);
if (diff != 0) {
return diff;
}
} else {
- diff = compareRows(a, b);
+ int leftRowLength = l.getRowLength();
+ int rightRowLength = r.getRowLength();
+ diff = compareRows(l, leftRowLength, r, rightRowLength);
if (diff != 0) {
return diff;
}
- diff = compareWithoutRow(a, b);
+ diff = compareWithoutRow(l, r);
if (diff != 0) {
return diff;
}
}
-
// Negate following comparisons so later edits show up first mvccVersion: later sorts first
- return ignoreSequenceid? diff: Long.compare(b.getSequenceId(), a.getSequenceId());
+ return ignoreSequenceid ? diff : Long.compare(r.getSequenceId(), l.getSequenceId());
+ }
+
+ private static int compareBBKV(final ByteBufferKeyValue left, final ByteBufferKeyValue right) {
+ int diff;
+ // Compare Rows. Cache row length.
+ int leftRowLength = left.getRowLength();
+ int rightRowLength = right.getRowLength();
+ diff = ByteBufferUtils.compareTo(left.getRowByteBuffer(), left.getRowPosition(),
+ leftRowLength, right.getRowByteBuffer(), right.getRowPosition(), rightRowLength);
+ if (diff != 0) {
+ return diff;
+ }
+
+ // If the column is not specified, the "minimum" key type appears as latest in the sorted
+ // order, regardless of the timestamp. This is used for specifying the last key/value in a
+ // given row, because there is no "lexicographically last column" (it would be infinitely
+ // long).
+ // The "maximum" key type does not need this behavior. Copied from KeyValue. This is bad in
+ // that
+ // we can't do memcmp w/ special rules like this.
+ // TODO: Is there a test for this behavior?
+ int leftFamilyLengthPosition = left.getFamilyLengthPosition(leftRowLength);
+ int leftFamilyLength = left.getFamilyLength(leftFamilyLengthPosition);
+ int leftKeyLength = left.getKeyLength();
+ int leftQualifierLength =
+ left.getQualifierLength(leftKeyLength, leftRowLength, leftFamilyLength);
+
+ // No need of left row length below here.
+
+ byte leftType = left.getTypeByte(leftKeyLength);
+ if (leftFamilyLength + leftQualifierLength == 0
+ && leftType == KeyValue.Type.Minimum.getCode()) {
+ // left is "bigger", i.e. it appears later in the sorted order
+ return 1;
+ }
+
+ int rightFamilyLengthPosition = right.getFamilyLengthPosition(rightRowLength);
+ int rightFamilyLength = right.getFamilyLength(rightFamilyLengthPosition);
+ int rightKeyLength = right.getKeyLength();
+ int rightQualifierLength =
+ right.getQualifierLength(rightKeyLength, rightRowLength, rightFamilyLength);
+
+ // No need of right row length below here.
+
+ byte rightType = right.getTypeByte(rightKeyLength);
+ if (rightFamilyLength + rightQualifierLength == 0
Review comment:
Same here also
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org