Copilot commented on code in PR #7432:
URL: https://github.com/apache/hbase/pull/7432#discussion_r2485390645
##########
hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/TableSnapshotInputFormatImpl.java:
##########
@@ -257,7 +257,10 @@ public ClientSideRegionScanner getScanner() {
public void initialize(InputSplit split, Configuration conf) throws
IOException {
this.scan = TableMapReduceUtil.convertStringToScan(split.getScan());
this.split = split;
- this.rowLimitPerSplit =
conf.getInt(SNAPSHOT_INPUTFORMAT_ROW_LIMIT_PER_INPUTSPLIT, 0);
+ int confLimit =
conf.getInt(SNAPSHOT_INPUTFORMAT_ROW_LIMIT_PER_INPUTSPLIT, 0);
+ int scanLimit = Math.max(scan.getLimit(), 0);
+ this.rowLimitPerSplit =
+ confLimit == 0 ? scanLimit : scanLimit == 0 ? confLimit :
Math.min(confLimit, scanLimit);
Review Comment:
[nitpick] The logic for merging confLimit and scanLimit uses nested ternary
operators which are difficult to read and maintain. Consider refactoring to use
if-else statements or extracting this logic into a separate helper method with
a descriptive name like `resolveLimitPerSplit(int confLimit, int scanLimit)`.
##########
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestTableSnapshotInputFormat.java:
##########
@@ -332,15 +333,14 @@ public void testScanLimit() throws Exception {
Job job = new Job(UTIL.getConfiguration());
Path tmpTableDir = UTIL.getDataTestDirOnTestFS(snapshotName);
- Scan scan = new Scan();
TableMapReduceUtil.addDependencyJarsForClasses(job.getConfiguration(),
TestTableSnapshotInputFormat.class);
TableMapReduceUtil.initTableSnapshotMapperJob(snapshotName, scan,
RowCounter.RowCounterMapper.class, NullWritable.class,
NullWritable.class, job, true,
tmpTableDir);
Assert.assertTrue(job.waitForCompletion(true));
- Assert.assertEquals(10 * regionNum,
+ Assert.assertEquals(expectedLimit * regionNum,
Review Comment:
Potential overflow in [int multiplication](1) before it is converted to long
by use in an invocation context.
```suggestion
Assert.assertEquals((long) expectedLimit * regionNum,
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java:
##########
@@ -1204,6 +1207,9 @@ public static Scan toScan(final ClientProtos.Scan proto)
throws IOException {
scan.setNeedCursorResult(true);
}
scan.setQueryMetricsEnabled(proto.getQueryMetricsEnabled());
+ if (proto.hasLimit()) {
+ scan.setLimit(proto.getLimit());
Review Comment:
The protobuf `limit` field is defined as `uint32` but `scan.setLimit()`
expects `int`. When deserializing, a protobuf uint32 value greater than
Integer.MAX_VALUE will be converted to a negative int in Java, which could
cause unexpected behavior. Consider adding validation to ensure the value fits
within int range or document this limitation.
```suggestion
int limit = proto.getLimit();
if (limit < 0) {
throw new IOException("Scan limit value " + (limit & 0xFFFFFFFFL) +
" exceeds Integer.MAX_VALUE (" + Integer.MAX_VALUE +
") and cannot be represented as a positive int in Java.");
}
scan.setLimit(limit);
```
--
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]
