keith-turner commented on code in PR #3830:
URL: https://github.com/apache/accumulo/pull/3830#discussion_r1356859875
##########
core/src/main/java/org/apache/accumulo/core/data/Range.java:
##########
@@ -305,6 +320,20 @@ public boolean afterEndKey(Key key) {
return stop.compareTo(key) <= 0;
}
+ /**
+ * Determines if the given row is after the ending row of this range.
+ *
+ * @param row row to check
+ * @return true if the given row is after the range, otherwise false
+ */
+ public boolean afterEndRow(Text row) {
+ if (infiniteStopKey) {
+ return false;
+ }
+
+ return stopKeyInclusive ? stop.getRow().compareTo(row) < 0 :
stop.getRow().compareTo(row) <= 0;
Review Comment:
The semantics of using stopKeyInclusive with only the row portion of the key
is very confusing because the inclusiveness settings are not always
conceptually related to the row. For example the following program prints
`true` and then `false`.
```java
// create a range over an entire column family in a row
Range range1 = Range.exact("987","12");
System.out.println(range1.afterEndRow(new Text("987")));
// create a range over an entire row
Range range2 = Range.exact("987");
System.out.println(range2.afterEndRow(new Text("987")));
```
For range1 stopKeyInclusive is set to false because of the column family and
it being false has nothing to do with the row.
I don't think adding these method in their current form is good because of
the confusing semantics. If something like this were implemented, thinking it
should do the following.
* translate the row to a key and call afterStartKey in its implementation
* explain translation of the row to a key and that afterStartKey is called
in the javadoc
* add tests to RangeTest
* offer a String version of the method like other methods in Range
* add a since tag since this an API change
I am not sure if all of the above is worthwhile w/o seeing what it would
look like.
##########
server/base/src/main/java/org/apache/accumulo/server/util/FileUtil.java:
##########
@@ -544,10 +543,10 @@ public static <T extends TabletFile>
WritableComparable<Key> findLastKey(ServerC
continue;
}
- Key key = reader.getLastKey();
+ Text row = reader.getLastRow();
- if (lastKey == null || key.compareTo(lastKey) > 0) {
- lastKey = key;
+ if (lastRow == null || row.compareTo(lastRow) > 0) {
Review Comment:
This is also an existing problem, not really handling fact that row could be
null. We could open a follow on issue to investigate.
##########
core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java:
##########
@@ -1617,20 +1618,20 @@ public Value getTopValue() {
}
@Override
- public Key getFirstKey() throws IOException {
- var rfk = reader.getFirstKey();
- if (fence.beforeStartKey(rfk)) {
- return fencedStartKey;
+ public Text getFirstRow() throws IOException {
+ var rfk = reader.getFirstRow();
+ if (fence.beforeStartRow(rfk)) {
Review Comment:
It seems that getFirstRow() can return null and passing null to
beforeStartRow or beforeStartKey may cause an NPE. This was an existing
problem. Not sure when or if it would happen.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
