Copilot commented on code in PR #6347:
URL: https://github.com/apache/hive/pull/6347#discussion_r2888059862
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/DatabaseProduct.java:
##########
@@ -852,10 +852,23 @@ public int getMaxRows(int batch, int paramSize) {
// SQL Server supports a maximum of 2100 parameters in a request. Adjust
the maxRowsInBatch accordingly
int maxAllowedRows = (2100 - paramSize) / paramSize;
return Math.min(batch, maxAllowedRows);
+ } else if (isPOSTGRES()) {
+ int maxAllowedRows = (32767 - paramSize) / paramSize;
+ return Math.min(batch, maxAllowedRows);
}
return batch;
}
+ public int getMaxBatch(int batch, int totalQueryParams) {
+ int minBatch = batch;
+ if (isSQLSERVER()) {
+ minBatch = Math.ceilDiv(totalQueryParams, 2100);
+ } else if (isPOSTGRES()) {
+ minBatch = Math.ceilDiv(totalQueryParams, 32767);
+ }
+ return batch <= 0 ? minBatch : Math.max(batch, minBatch);
+ }
+
Review Comment:
`Math.ceilDiv(...)` is not available in common LTS Java baselines like
8/11/17; if this module targets those (as Hive historically has), this will not
compile. Use an equivalent integer ceil-division implementation compatible with
the project’s Java version, or a utility already used in the codebase.
```suggestion
minBatch = ceilDiv(totalQueryParams, 2100);
} else if (isPOSTGRES()) {
minBatch = ceilDiv(totalQueryParams, 32767);
}
return batch <= 0 ? minBatch : Math.max(batch, minBatch);
}
/**
* Equivalent of Math.ceilDiv(int, int) available in Java 9+.
*/
private static int ceilDiv(int x, int y) {
// Mirrors the JDK implementation: -Math.floorDiv(-x, y)
return -Math.floorDiv(-x, y);
}
```
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java:
##########
@@ -717,7 +718,8 @@ public List<Partition> getPartitionsViaPartNames(final
String catName, final Str
if (partNames.isEmpty()) {
return Collections.emptyList();
}
- return Batchable.runBatched(batchSize, partNames, new Batchable<String,
Partition>() {
+ int batch = dbType.getMaxBatch(batchSize, partNames.size() + 3);
+ return Batchable.runBatched(batch, partNames, new Batchable<String,
Partition>() {
@Override
public List<Partition> run(List<String> input) throws MetaException {
return getPartitionsByNames(catName, dbName, tblName, partNames,
false, args);
Review Comment:
This callback ignores the batched `input` and instead passes the full
`partNames` list into `getPartitionsByNames(...)`, defeating batching and
reintroducing the risk of exceeding the database parameter limit. Pass `input`
to `getPartitionsByNames(...)` so each batch query uses only the intended
subset.
```suggestion
return getPartitionsByNames(catName, dbName, tblName, input, false,
args);
```
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java:
##########
@@ -10058,7 +10060,9 @@ public List<Void> run(List<String> input) throws
Exception {
}
};
try {
- Batchable.runBatched(batchSize, partNames, b);
+ int batch = dbType.getMaxBatch(batchSize,
+ partNames.size() + (colNames != null ? colNames.size() : 0) +
(engine != null ? 4 : 3));
Review Comment:
The new batching logic relies on unexplained magic numbers (`+ 4`, `+ 3`) to
approximate additional bind parameters, which is easy to get wrong and hard to
review/maintain. Consider replacing these with named constants (e.g.,
`FIXED_PARAMS_WITH_ENGINE`, `FIXED_PARAMS_NO_ENGINE`) or a small helper that
derives the fixed-parameter count from the actual query construction.
```suggestion
final int fixedParamsWithEngine = 4;
final int fixedParamsWithoutEngine = 3;
int batch = dbType.getMaxBatch(batchSize,
partNames.size() + (colNames != null ? colNames.size() : 0)
+ (engine != null ? fixedParamsWithEngine :
fixedParamsWithoutEngine));
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
