You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ptupitsyn (via GitHub)" <gi...@apache.org> on 2026/03/20 11:35:13 UTC
[PR] IGNITE-28012 Client protocol: Fix ErrorExtensions.SQL_UPDATE_COUNTERS handling [ignite-3]
ptupitsyn opened a new pull request, #7836:
URL: https://github.com/apache/ignite-3/pull/7836
Pass as a single value so that clients can skip unknown extensions properly.
https://issues.apache.org/jira/browse/IGNITE-28012
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28012 Client protocol: Fix ErrorExtensions.SQL_UPDATE_COUNTERS handling [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28012 Client protocol: Fix ErrorExtensions.SQL_UPDATE_COUNTERS handling [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn merged PR #7836:
URL: https://github.com/apache/ignite-3/pull/7836
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org