Copilot commented on code in PR #7099:
URL: https://github.com/apache/ignite-3/pull/7099#discussion_r2568824995
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItSqlMultiStatementTest.java:
##########
@@ -207,6 +208,43 @@ void scriptStopsExecutionOnError() {
}
}
+ /**
+ * The test verifies that if an error occurs within the script,
+ * previous statements will not be forcibly cancelled.
+ *
+ * <p>Note: this behavior is important for {@code
JdbcStatement.executeBatch},
+ * because the method executes a batch of statements as a script,
+ * and if an error occurs somewhere, it is impossible to
correctly count
+ * the number of updates (for a batch update exception).
+ */
+ @Test
+ void precedingStatementsDontAbortedOnError() throws InterruptedException {
Review Comment:
The method name has a grammatical error. Consider renaming to
`precedingStatementsAreNotAbortedOnError` or
`precedingStatementsDontGetAbortedOnError` for correct grammar. The current
name mixes "don't" (a contraction) with "aborted" (past participle) incorrectly.
```suggestion
void precedingStatementsAreNotAbortedOnError() throws
InterruptedException {
```
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItSqlMultiStatementTest.java:
##########
@@ -207,6 +208,43 @@ void scriptStopsExecutionOnError() {
}
}
+ /**
+ * The test verifies that if an error occurs within the script,
+ * previous statements will not be forcibly cancelled.
+ *
+ * <p>Note: this behavior is important for {@code
JdbcStatement.executeBatch},
+ * because the method executes a batch of statements as a script,
+ * and if an error occurs somewhere, it is impossible to
correctly count
+ * the number of updates (for a batch update exception).
+ */
+ @Test
+ void precedingStatementsDontAbortedOnError() throws InterruptedException {
+ String script = "CREATE TABLE test (id INT PRIMARY KEY, val INT);"
+ + "INSERT INTO test VALUES (0, 0), (1, 1), (2, 2);"
+ + "UPDATE test SET val = val + 1;"
+ + "INSERT INTO test VALUES (3, 3/0);";
+
+ AsyncSqlCursor<InternalSqlRow> ddlCursor = runScript(script);
+ AsyncSqlCursor<InternalSqlRow> dmlCursor1 =
await(ddlCursor.nextResult());
+ AsyncSqlCursor<InternalSqlRow> dmlCursor2 =
await(dmlCursor1.nextResult());
+
+ assertThrowsSqlException(
+ RUNTIME_ERR,
+ "Division by zero",
+ () -> await(dmlCursor2.nextResult())
+ );
+
+ // Put some delay to ensure no one tries to close the cursors in the
background.
+ Thread.sleep(100);
+
+ validateSingleResult(ddlCursor, true);
+ validateSingleResult(dmlCursor1, 3L);
+ validateSingleResult(dmlCursor2, 3L);
+
+ List.of(ddlCursor, dmlCursor1, dmlCursor2)
+ .forEach(AsyncCursor::closeAsync);
Review Comment:
The cursors are being closed twice. The `validateSingleResult` method (line
240-242) already calls `cursor.closeAsync()` on each cursor (see
BaseSqlMultiStatementTest.java:186), so calling `closeAsync()` again here is
redundant and could cause issues. Remove these explicit close calls since
they're already handled by `validateSingleResult`.
```suggestion
// Cursors are already closed by validateSingleResult above.
```
--
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]
