saintstack commented on a change in pull request #2776:
URL: https://github.com/apache/hbase/pull/2776#discussion_r546265019
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
##########
@@ -920,6 +930,14 @@ public static boolean matchingColumn(final Cell left,
final Cell right) {
return matchingQualifier(left, right);
}
+ public static boolean matchingColumn(final Cell left, final byte lFamLen,
final int lQualLength,
Review comment:
These have to be public? They are in same package as comparator and Cell
types.
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
##########
@@ -1598,16 +1621,22 @@ 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();
Review comment:
We are decoding ints, bytes and shorts that we might end up not using at
all.
##########
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -57,29 +57,285 @@ 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;
+ }
+ } else if (l instanceof ByteBufferKeyValue && r instanceof
ByteBufferKeyValue) {
Review comment:
This if/else is a lot of code. Would be good to break it out... check
for it at top of the compare.... For a follow-on.
##########
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -57,29 +57,285 @@ 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;
+ }
+ } 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 compareKeyValues(final KeyValue left, final KeyValue
right) {
+ int diff;
+ // Compare Rows. Cache row length.
+ int leftRowLength = left.getRowLength();
+ int rightRowLength = right.getRowLength();
+ diff = Bytes.compareTo(left.getRowArray(), left.getRowOffset(),
leftRowLength,
+ right.getRowArray(), right.getRowOffset(), 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 (leftType == KeyValue.Type.Minimum.getCode()
+ && leftFamilyLength + leftQualifierLength == 0) {
+ // 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 (rightType == KeyValue.Type.Minimum.getCode()
+ && rightFamilyLength + rightQualifierLength == 0) {
+ return -1;
+ }
+
+ // Compare families.
+ int leftFamilyPosition = left.getFamilyOffset(leftFamilyLengthPosition);
+ int rightFamilyPosition = right.getFamilyOffset(rightFamilyLengthPosition);
+ diff = Bytes.compareTo(left.getFamilyArray(), leftFamilyPosition,
leftFamilyLength,
+ right.getFamilyArray(), rightFamilyPosition, rightFamilyLength);
+ if (diff != 0) {
+ return diff;
+ }
+
+ // Compare qualifiers
+ diff = Bytes.compareTo(left.getQualifierArray(),
+ left.getQualifierOffset(leftFamilyPosition, leftFamilyLength),
leftQualifierLength,
+ right.getQualifierArray(), right.getQualifierOffset(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));
+ if (diff != 0) {
+ return diff;
+ }
+
+ // Compare types. Let the delete types sort ahead of puts; i.e. types
+ // of higher numbers sort before those of lesser numbers. Maximum (255)
+ // appears ahead of everything, and minimum (0) appears after
+ // everything.
+ return (0xff & rightType) - (0xff & leftType);
+ }
Review comment:
In follow-on, we should look at produced byte code. Might get ideas on
how to make this go faster.
##########
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:
The perf tests were PE @ramkrish86 ? Yeah, we need a JMH test so can do
the combinations @anoopsjohn suggests. I can work on that...
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java
##########
@@ -1396,7 +1403,14 @@ public int getQualifierOffset() {
* @return Qualifier offset
*/
private int getQualifierOffset(int foffset) {
- return foffset + getFamilyLength(foffset);
+ return getQualifierOffset(foffset, getFamilyLength());
+ }
+
+ /**
+ * @return Qualifier offset
+ */
+ int getQualifierOffset(int foffset, int flength) {
Review comment:
Looks good
----------------------------------------------------------------
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:
us...@infra.apache.org
