jbonofre commented on code in PR #1438:
URL: https://github.com/apache/arrow-java/pull/1438#discussion_r4444614124
##########
flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightMetaImpl.java:
##########
@@ -165,7 +166,15 @@ public ExecuteBatchResult executeBatch(
}
// Update query
- long[] updatedCounts = {preparedStatement.executeUpdate()};
+ long totalUpdated = preparedStatement.executeUpdate();
+ final int batchSize = parameterValuesList.size();
+ long[] updatedCounts = new long[batchSize];
+ if (batchSize > 0) {
+ Arrays.fill(updatedCounts, totalUpdated / batchSize);
+ if (totalUpdated % batchSize != 0) {
+ updatedCounts[0] += totalUpdated % batchSize;
+ }
Review Comment:
`executeUpdate()` returns a single aggregate count for the whole bound
batch, so dividing it evenly across statements produces counts that never
happened. For example, statements affecting 10, 0 and 2 rows come back as
`{4,4,4}`. A total of 10 over 3 statements comes back as `{4,3,3}`, with the
remainder added to the first entry. A `-1` (unknown) total comes back as
`{-1,0,0}`.
Callers that inspect individual counts, such as optimistic-locking checks
that expect 0 or 1, will silently get wrong answers.
Since the per-statement counts are not known, JDBC has
`Statement.SUCCESS_NO_INFO` for this case.
Could we fill the array with it instead?
```java
long[] updatedCounts = new long[batchSize];
Arrays.fill(updatedCounts, Statement.SUCCESS_NO_INFO);
```
If there's a reason to keep the split, please add a note in Javadoc.
Even then, a negative total should not be divided.
##########
flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightMetaImpl.java:
##########
@@ -165,7 +166,15 @@ public ExecuteBatchResult executeBatch(
}
// Update query
- long[] updatedCounts = {preparedStatement.executeUpdate()};
+ long totalUpdated = preparedStatement.executeUpdate();
Review Comment:
`executeUpdate()` runs before we know whether the batch is empty. If
`parameterValuesList` is empty, nothing was bound, but the update still
executes on the server. If the statement has no parameters, or still holds
parameters from an earlier use, that can modify rows.
The caller then gets back an empty array and no sign it happened.
Could we return early for an empty batch, before `executeUpdate()` is called?
```java
final int batchSize = parameterValuesList.size();
if (batchSize == 0) {
return new ExecuteBatchResult(new long[0]);
}
```
--
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]
