keith-turner commented on code in PR #3342:
URL: https://github.com/apache/accumulo/pull/3342#discussion_r2392617546
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java:
##########
@@ -249,16 +250,36 @@ Locations locate(String tableName, Collection<Range>
ranges)
* Finds the max row within a given range. To find the max row in a table,
pass null for start and
* end row.
*
+ * @param tableName the table to search
* @param auths find the max row that can seen with these auths
* @param startRow row to start looking at, null means -Infinity
* @param startInclusive determines if the start row is included
* @param endRow row to stop looking at, null means Infinity
* @param endInclusive determines if the end row is included
*
* @return The max row in the range, or null if there is no visible data in
the range.
+ *
+ * @deprecated since 4.0.0, use {@link #getMaxRow(String, Authorizations,
RowRange)} instead
+ */
+ @Deprecated(since = "4.0.0")
+ default Text getMaxRow(String tableName, Authorizations auths, Text startRow,
+ boolean startInclusive, Text endRow, boolean endInclusive)
+ throws TableNotFoundException, AccumuloException,
AccumuloSecurityException {
+ return getMaxRow(tableName, auths,
+ RowRange.range(startRow, startInclusive, endRow, endInclusive));
+ }
+
+ /**
+ * Finds the max row within a given row range. To find the max row in the
whole table, pass
+ * {@link RowRange#all()} as the row range.
+ *
+ * @param tableName the table to search
+ * @param auths find the max row that can seen with these auths
+ * @param range the range of rows to search
+ *
+ * @return The max row in the range, or null if there is no visible data in
the range.
*/
- Text getMaxRow(String tableName, Authorizations auths, Text startRow,
boolean startInclusive,
- Text endRow, boolean endInclusive)
+ Text getMaxRow(String tableName, Authorizations auths, RowRange range)
Review Comment:
needs a since tag
##########
core/src/main/java/org/apache/accumulo/core/summary/SummarySerializer.java:
##########
@@ -97,17 +98,8 @@ public Map<String,Long> getSummary(List<RowRange> ranges,
SummarizerFactory sf)
}
public boolean exceedsRange(List<RowRange> ranges) {
- boolean er = false;
- for (LgSummaries lgs : allSummaries) {
- for (RowRange ke : ranges) {
- er |= lgs.exceedsRange(ke.getStartRow(), ke.getEndRow());
- if (er) {
- return er;
- }
- }
- }
-
- return er;
+ return Arrays.stream(allSummaries).anyMatch(lgs -> ranges.stream()
Review Comment:
May be good to check these ranges are exclusive, inclusvie.
##########
core/src/main/java/org/apache/accumulo/core/summary/SummarySerializer.java:
##########
@@ -463,22 +455,22 @@ void print(String prefix, String indent, PrintStream out)
{
void getSummary(List<RowRange> ranges, Combiner combiner, Map<String,Long>
summary) {
boolean[] summariesThatOverlap = new boolean[summaries.length];
- for (RowRange keyExtent : ranges) {
- Text startRow = keyExtent.getStartRow();
- Text endRow = keyExtent.getEndRow();
+ for (RowRange rowRange : ranges) {
+ Text lowerBound = rowRange.getLowerBound();
Review Comment:
The existing code assumes the row range i exclusive, inclusive. Would be
good to add a check here to ensure the row range is like that. Not sure other
types of ranges would be handled.
```suggestion
checkState(!rowRange.isLowerBoundInclusive() &&
rowRange.isUpperBoundInclusive());
Text lowerBound = rowRange.getLowerBound();
```
--
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-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
