You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2023/10/07 16:25:23 UTC
[PR] Change getFirstKey/getLastKey to getFirstRow/getLastRow [accumulo]
cshannon opened a new pull request, #3830:
URL: https://github.com/apache/accumulo/pull/3830
This commit changes FileSKVIterator getFirstKey and getLastKey methods to return the first and last row instead. These methods are only used for splits and in that case only the row is relevant. This was done because fenced RFiles will only return the row portion of a key when these methods are called (if the fencing is being enforced) so this keeps the API consistent.
This closes #3798
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change getFirstKey/getLastKey to getFirstRow/getLastRow [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3830:
URL: https://github.com/apache/accumulo/pull/3830#discussion_r1356911623
##########
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:
I agree, I think the fix here is to add the null check for getFirstRow() and getLastRow() and then look into if/when the rows would actually be null in another issue since this is an existing problem and unrelated to the PR here
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change getFirstKey/getLastKey to getFirstRow/getLastRow [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3830:
URL: https://github.com/apache/accumulo/pull/3830#discussion_r1358950002
##########
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 (rfk != null && fence.beforeStartKey(new Key(rfk))) {
+ return fencedStartKey.getRow();
} else {
return rfk;
}
}
@Override
- public Key getLastKey() throws IOException {
- var rlk = reader.getLastKey();
- if (fence.afterEndKey(rlk)) {
- return fencedEndKey.get();
+ public Text getLastRow() throws IOException {
+ var rlk = reader.getLastRow();
Review Comment:
could change the var name if the `k` was for key
##########
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();
Review Comment:
could change the var name, assuming the `k` was for key?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change getFirstKey/getLastKey to getFirstRow/getLastRow [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3830:
URL: https://github.com/apache/accumulo/pull/3830#discussion_r1356900786
##########
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:
It is likely not going to be worth the effort to do that so I will get rid of these new methods.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change getFirstKey/getLastKey to getFirstRow/getLastRow [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon merged PR #3830:
URL: https://github.com/apache/accumulo/pull/3830
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change getFirstKey/getLastKey to getFirstRow/getLastRow [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3830:
URL: https://github.com/apache/accumulo/pull/3830#issuecomment-1756088952
I kicked off a full IT build for this
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change getFirstKey/getLastKey to getFirstRow/getLastRow [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change getFirstKey/getLastKey to getFirstRow/getLastRow [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3830:
URL: https://github.com/apache/accumulo/pull/3830#discussion_r1356908465
##########
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:
I will add a null check for these methods
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change getFirstKey/getLastKey to getFirstRow/getLastRow [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3830:
URL: https://github.com/apache/accumulo/pull/3830#issuecomment-1757497634
The full IT build passed.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org