Copilot commented on code in PR #7836:
URL: https://github.com/apache/ignite-3/pull/7836#discussion_r2965382098
##########
modules/compatibility-tests/src/integrationTest/java/org/apache/ignite/internal/client/ClientCompatibilityTests.java:
##########
@@ -236,6 +237,50 @@ default void testSqlBatch() {
assertEquals(2, rows.size());
}
+ @Test
+ default void testSqlBatchException() {
+ // Insert initial row to trigger constraint violation
+ int duplicateId = idGen().incrementAndGet();
+ client().sql().execute("INSERT INTO " + TABLE_NAME_TEST + " (id, name)
VALUES (?, ?)", duplicateId, "initial");
+
+ // Create batch where some rows will succeed and one will fail with
duplicate key
+ int id1 = idGen().incrementAndGet();
+ int id2 = idGen().incrementAndGet();
+ int id3 = idGen().incrementAndGet();
+ int id4 = idGen().incrementAndGet();
+
+ BatchedArguments args = BatchedArguments.create()
+ .add(id1, "test1")
+ .add(id2, "test2")
+ .add(id3, "test3")
+ .add(duplicateId, "duplicate") // This will fail - duplicate
primary key
+ .add(id4, "test4"); // This won't be executed due to error
+
+ var ex = assertThrows(
+ SqlBatchException.class,
+ () -> client().sql().executeBatch("INSERT INTO " +
TABLE_NAME_TEST + " (id, name) VALUES (?, ?)", args));
+
Review Comment:
This new compatibility test calls `client().sql().execute(String, ...)` and
`client().sql().executeBatch(String, ...)`. In the compatibility suite, tests
are executed with older client binaries; these convenience overloads may not
exist in some base versions, leading to `NoSuchMethodError` even when the
transaction/cancellation-token overloads exist. To keep the test runnable
across all supported base versions, use the explicit overloads already used
elsewhere in this file (e.g., `execute((Transaction) null, ...)` and
`executeBatch((Transaction) null, ...)`).
##########
modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientMessageUnpacker.java:
##########
@@ -791,6 +791,24 @@ public long[] unpackLongArray() {
return res;
}
+ /**
+ * Reads a long array from a single binary value.
+ *
+ * @return Array of longs.
+ */
+ public long[] unpackLongArrayAsBinary() {
+ assert refCnt > 0 : "Unpacker is closed";
+
+ int binSize = unpackBinaryHeader();
+ long[] res = new long[binSize / 8];
+
+ for (int i = 0; i < res.length; i++) {
+ res[i] = buf.readLong();
+ }
+
+ return res;
+ }
Review Comment:
`unpackLongArrayAsBinary()` calculates the array length as `binSize / 8` but
does not validate that `binSize` is a multiple of 8. If the payload size is
malformed (or if the encoding changes), the method will leave unread trailing
bytes in the buffer and corrupt subsequent unpacking. Add a guard (e.g., throw
when `binSize % Long.BYTES != 0`) and consider using `Long.BYTES` instead of
the magic number.
##########
modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientMessagePacker.java:
##########
@@ -630,6 +630,27 @@ public void packLongArray(long[] arr) {
}
}
+ /**
+ * Writes a long array as a single binary value.
+ *
+ * @param arr Long array value.
+ */
+ public void packLongArrayAsBinary(long[] arr) {
+ assert !closed : "Packer is closed";
+
+ if (arr == null) {
+ packNil();
+
+ return;
+ }
+
+ packBinaryHeader(arr.length * 8);
+
+ for (long value : arr) {
+ buf.writeLong(value);
+ }
+ }
Review Comment:
`packLongArrayAsBinary()` writes raw `long` bytes without
documenting/standardizing endianness (it currently relies on Netty's default
big-endian `writeLong`). Since this is part of the cross-language client
protocol, please explicitly define the byte order in the protocol/Javadoc and
use the corresponding `writeLongLE`/`readLongLE` or `writeLong`/`readLong`
consistently on both sides.
##########
modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientMessageUnpacker.java:
##########
@@ -791,6 +791,24 @@ public long[] unpackLongArray() {
return res;
}
+ /**
+ * Reads a long array from a single binary value.
+ *
+ * @return Array of longs.
+ */
+ public long[] unpackLongArrayAsBinary() {
+ assert refCnt > 0 : "Unpacker is closed";
+
+ int binSize = unpackBinaryHeader();
+ long[] res = new long[binSize / 8];
+
+ for (int i = 0; i < res.length; i++) {
+ res[i] = buf.readLong();
+ }
+
+ return res;
+ }
Review Comment:
`unpackLongArrayAsBinary()` assumes the next value is a binary and does not
handle `nil`. Since `packLongArrayAsBinary()` can write `nil` (when the array
is null), the unpacker should either support `nil` (e.g., return null/empty) or
the caller should check `tryUnpackNil()` before calling this method to avoid
protocol desync/type exceptions.
--
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]
