xtern commented on code in PR #6806:
URL: https://github.com/apache/ignite-3/pull/6806#discussion_r2448186858
##########
modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc2/JdbcStatement2.java:
##########
@@ -428,16 +423,23 @@ public boolean execute(String sql, String[] colNames)
throws SQLException {
public @Nullable ResultSet getResultSet() throws SQLException {
ensureNotClosed();
- return isQuery() ? resultSet : null;
+ ResultSetWrapper rs = result;
+ if (rs == null) {
+ return null;
+ } else if (rs.isQuery()) {
+ return rs.current();
+ } else {
+ return null;
+ }
Review Comment:
```suggestion
if (rs != null && rs.isQuery()) {
return rs.current();
} else {
return null;
}
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/sql/ItSqlCommandTest.java:
##########
@@ -177,13 +175,12 @@ void multilineScript() {
}
@Test
- @Disabled("https://issues.apache.org/jira/browse/IGNITE-26142";)
void exceptionHandler() {
execute("sql", "SELECT 1/0;", "--jdbc-url", JDBC_URL);
assertAll(
this::assertOutputIsEmpty,
- () -> assertErrOutputContains("SQL query execution error"),
+ // () -> assertErrOutputContains("SQL query execution error"),
Review Comment:
I believe this should be marked with a corresponding TODO
##########
modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc2/ResultSetWrapper.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.jdbc2;
+
+import java.sql.SQLException;
+
+final class ResultSetWrapper {
+
+ private JdbcResultSet resultSet;
+
+ ResultSetWrapper(JdbcResultSet first) {
+ resultSet = first;
+ }
+
+ void setCloseStatement(boolean b) {
+ resultSet.setCloseStatement(b);
+ }
+
+ JdbcResultSet current() {
+ return resultSet;
+ }
+
+ boolean isQuery() {
+ ClientSyncResultSet rs = resultSet.resultSet();
+
+ return rs.hasRowSet();
+ }
+
+ int updateCount() {
+ JdbcResultSet rs = resultSet;
+
+ ClientSyncResultSet clientResultSet = rs.resultSet();
+ if (clientResultSet.hasRowSet()) {
+ return -1;
+ } else if (clientResultSet.wasApplied() ||
!clientResultSet.wasApplied() && clientResultSet.affectedRows() == -1) {
+ // DDL or control statements
+ return 0;
+ } else if (clientResultSet.affectedRows() >= 0) {
+ return (int) clientResultSet.affectedRows();
+ } else {
+ return -1;
+ }
Review Comment:
As far as I understand, we can only have 3 options: either it’s a SELECT, or
it’s DML (there are affectedRows), or everything else - DDL/KILL/etc
(applied/not applied).
```suggestion
} else if (clientResultSet.affectedRows() == -1) {
// DDL or control statements
return 0;
} else {
return (int) clientResultSet.affectedRows();
}
```
##########
modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java:
##########
@@ -145,18 +146,25 @@ public void testSimpleQueryExecute() throws Exception {
public void testSimpleQueryError() throws Exception {
boolean res = stmt.execute("SELECT 1; SELECT 1/0; SELECT 2");
assertTrue(res);
- assertThrowsSqlException("Failed to fetch query results", () ->
stmt.getMoreResults());
+ assertThrowsSqlException("Division by zero", () ->
stmt.getMoreResults());
// Next after exception.
- assertFalse(stmt.getMoreResults());
+ // assertFalse(stmt.getMoreResults());
+ // Do not move past the first result.
+ assertThrowsSqlException("Division by zero", () ->
stmt.getMoreResults());
stmt.closeOnCompletion();
}
+ @Test
+ public void testSimpleQueryErrorDoNotReleaseResources() throws Exception {
+ stmt.execute("SELECT 1; SELECT 2/0; SELECT 3");
Review Comment:
`testSimpleQueryErrorDoNotReleaseResources`
Can you explain what exactly this test checks and how?
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/sql/ItSqlReplCommandTest.java:
##########
@@ -64,11 +63,11 @@ void secondInvocationScript() {
}
@Test
- @Disabled("https://issues.apache.org/jira/browse/IGNITE-26142";)
void multilineCommand() {
execute("CREATE TABLE MULTILINE_TABLE(K INT PRIMARY KEY); \n INSERT
INTO MULTILINE_TABLE VALUES(1);", "--jdbc-url", JDBC_URL);
assertAll(
+ // TODO https://issues.apache.org/jira/browse/IGNITE-26790
// The output from CREATE TABLE is: Updated 0 rows.
Review Comment:
what is expected here?
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlCursorCloseRequest.java:
##########
@@ -40,8 +42,13 @@ public static CompletableFuture<ResponseWriter> process(
) throws IgniteInternalCheckedException {
long resourceId = in.unpackLong();
- ClientSqlResultSet asyncResultSet =
resources.remove(resourceId).get(ClientSqlResultSet.class);
+ try {
+ ClientSqlResultSet asyncResultSet =
resources.remove(resourceId).get(ClientSqlResultSet.class);
- return asyncResultSet.closeAsync().thenApply(v -> null);
+ return asyncResultSet.closeAsync().thenApply(v -> null);
+ } catch (IgniteInternalCheckedException | IgniteInternalException
ignored) {
+ // Ignore: either resource already removed, or registry is closing.
+ return CompletableFutures.nullCompletedFuture();
Review Comment:
Why has it become necessary to ignore such errors now?
##########
modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc2/ResultSetWrapper.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.jdbc2;
+
+import java.sql.SQLException;
+
+final class ResultSetWrapper {
+
+ private JdbcResultSet resultSet;
+
+ ResultSetWrapper(JdbcResultSet first) {
+ resultSet = first;
+ }
+
+ void setCloseStatement(boolean b) {
+ resultSet.setCloseStatement(b);
+ }
+
+ JdbcResultSet current() {
+ return resultSet;
+ }
+
+ boolean isQuery() {
+ ClientSyncResultSet rs = resultSet.resultSet();
+
+ return rs.hasRowSet();
+ }
+
+ int updateCount() {
+ JdbcResultSet rs = resultSet;
+
+ ClientSyncResultSet clientResultSet = rs.resultSet();
+ if (clientResultSet.hasRowSet()) {
+ return -1;
+ } else if (clientResultSet.wasApplied() ||
!clientResultSet.wasApplied() && clientResultSet.affectedRows() == -1) {
+ // DDL or control statements
+ return 0;
+ } else if (clientResultSet.affectedRows() >= 0) {
+ return (int) clientResultSet.affectedRows();
+ } else {
+ return -1;
+ }
+ }
+
+ boolean nextResultSet() throws SQLException {
+ JdbcResultSet current = resultSet;
+ if (current == null) {
+ return false;
+ }
+
+ try {
+ JdbcResultSet nextRs = current.tryNextResultSet();
+ boolean hasNext = nextRs != null;
+
+ resultSet = nextRs;
+
+ return hasNext;
+ } finally {
+ current.close();
+ }
+ }
+
+ void close() throws SQLException {
+ JdbcResultSet current = resultSet;
+ if (current == null || current.closed) {
+ return;
+ }
+ resultSet = null;
+
+ JdbcResultSet rs = current;
+
+ do {
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-26789 Close
properly.
+ // w/o iteration over all cursors, the cursors that are not
retrieved hung in the void
+ // and are never released (this does not seem right).
Review Comment:
Just to not forget.
Even after fixing the bug mentioned in the link (IGNITE-26789), the client
will still need to iterate over the cursors here, due to lazy execution of
SELECTs (it's up to client - fetch whole data or just close the cursor).
I suggest:
* remove link to IGNITE-26789.
* add a link to ticket with proposal how we can rework our script execution
or remove "(this does not seem right)"
--
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]
