korlov42 commented on code in PR #2906: URL: https://github.com/apache/ignite-3/pull/2906#discussion_r1411882676
########## modules/client-common/src/main/java/org/apache/ignite/internal/jdbc/proto/event/JdbcGetMoreResultsResult.java: ########## @@ -0,0 +1,120 @@ +/* + * 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.jdbc.proto.event; + +import org.apache.ignite.internal.client.proto.ClientMessagePacker; +import org.apache.ignite.internal.client.proto.ClientMessageUnpacker; + +/** + * JDBC getMoreResults result. + */ +public class JdbcGetMoreResultsResult extends Response { + /** Cursor ID. */ + private long cursorId; + + /** Dml participation. */ + private boolean isDml; + + /** Non dml\ddl query flag. */ + private boolean isQuery; + + /** + * Default constructor is used for deserialization. + */ + public JdbcGetMoreResultsResult() { + } + + /** + * Constructor. + * + * @param hasNext {@code true} if more results are present. + * @param cursorId Cursor ID. + */ + public JdbcGetMoreResultsResult(boolean hasNext, long cursorId) { + hasResults = hasNext; + this.cursorId = cursorId; + } + + /** + * Constructor. + * + * @param hasNext {@code true} if more results are present. + * @param cursorId Cursor ID. + * @param isDml Dml corresponding statement flag. + * @param isQuery Non dml\ddl corresponding statement flag + */ + public JdbcGetMoreResultsResult(boolean hasNext, long cursorId, boolean isDml, boolean isQuery) { Review Comment: I'm wondering what does it mean `new JdbcGetMoreResultsResult(true, <cId>, true, true)`? ########## modules/client-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryEventHandlerImpl.java: ########## @@ -152,13 +152,25 @@ public CompletableFuture<? extends Response> queryAsync(long connectionId, JdbcQ InternalTransaction tx = req.autoCommit() ? null : connectionContext.getOrStartTransaction(); SqlProperties properties = createProperties(req.getStmtType()); - CompletableFuture<AsyncSqlCursor<List<Object>>> result = processor.querySingleAsync( - properties, - igniteTransactions, - tx, - req.sqlQuery(), - req.arguments() == null ? OBJECT_EMPTY_ARRAY : req.arguments() - ); + CompletableFuture<AsyncSqlCursor<List<Object>>> result; + + if (req.multiStatement()) { + result = processor.queryScriptAsync( + properties, + igniteTransactions, + tx, + req.sqlQuery(), + req.arguments() == null ? OBJECT_EMPTY_ARRAY : req.arguments() + ); + } else { + result = processor.querySingleAsync( + properties, + igniteTransactions, + tx, + req.sqlQuery(), + req.arguments() == null ? OBJECT_EMPTY_ARRAY : req.arguments() + ); + } return result.thenCompose(cursor -> createJdbcResult(new JdbcQueryCursor<>(req.maxRows(), cursor), req)) .thenApply(jdbcResult -> new JdbcQueryExecuteResult(List.of(jdbcResult))) Review Comment: I see that `JdbcQueryExecuteResult` used only to wrap a single instance of `JdbcQuerySingleResult`, which in turn extends `Response` as well. So, probably, we could get rid of `JdbcQueryExecuteResult` and simplify protocol a bit ########## modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java: ########## @@ -414,6 +416,10 @@ public int getUpdateCount() throws SQLException { JdbcResultSet rs = resSets.get(curRes); + if (rs == null) { + rs = resSets.get(curRes - 1); + } Review Comment: why do we get the previous result set here? ########## modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java: ########## @@ -454,11 +464,11 @@ public boolean getMoreResults(int curr) throws SQLException { throw new SQLFeatureNotSupportedException("Multiple open results is not supported."); Review Comment: whe don't we support this? ########## modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcResultSet.java: ########## @@ -201,6 +205,37 @@ public JdbcResultSet(List<List<Object>> rows, List<JdbcColumnMeta> meta) throws initColumnOrder(); } + JdbcResultSet getNextResultSet() throws SQLException { + try { + JdbcGetMoreResultsRequest req = new JdbcGetMoreResultsRequest(cursorId); + JdbcGetMoreResultsResult res = cursorHandler.getMoreResultsAsync(req).get(); + + if (!res.hasResults()) { + return null; + } + + long cursorId0 = res.cursorId(); + + long updCount = 0; + + if (res.isDml()) { + CompletableFuture<JdbcQueryFetchResult> fetch = cursorHandler.fetchAsync( + new JdbcQueryFetchRequest(cursorId0, 1)); + JdbcQueryFetchResult fetchRes = fetch.get(); + updCount = (long) fetchRes.items().get(0).get(0); + } + + return new JdbcResultSet(cursorHandler, stmt, cursorId0, getFetchSize(), false, Collections.emptyList(), res.isQuery(), Review Comment: when we receive the very first result for the first statement, we get first page alongside the cursor. Why do we need an extra roundtrip to server if we are moving to next cursor? ########## modules/client-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryEventHandlerImpl.java: ########## @@ -363,29 +375,31 @@ private CompletionStage<JdbcQuerySingleResult> createJdbcResult(AsyncSqlCursor<L return cur.requestNextAsync(req.pageSize()).thenApply(batch -> { boolean hasNext = batch.hasMore(); + long cursorId; Review Comment: now we are registering all the types of cursor, but do we clean all types as well? as far as I see, no, DML and DDL cursors are stuck in client resources forever. we need to cover this with tests ########## modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcResultSet.java: ########## @@ -201,6 +205,37 @@ public JdbcResultSet(List<List<Object>> rows, List<JdbcColumnMeta> meta) throws initColumnOrder(); } + JdbcResultSet getNextResultSet() throws SQLException { Review Comment: should be marked as `@Nullable` ########## modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java: ########## @@ -396,7 +398,7 @@ public ResultSet getResultSet() throws SQLException { JdbcResultSet rs = resSets.get(curRes); - if (!rs.isQuery()) { + if (rs == null || !rs.isQuery()) { Review Comment: if null is considering as legit element of `resSets` collection, then we need to mark generic type as `@Nullable` ########## modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java: ########## @@ -432,10 +438,14 @@ public boolean getMoreResults() throws SQLException { public boolean getMoreResults(int curr) throws SQLException { Review Comment: don't know where to put this, so let it be here: for sequence of DML, expression `!stmt.getMoreResults() && stmt.getUpdateCount() == -1` never returns `true` (that is now we determine there are no more results, see javadoc for `getMoreResults`) -- 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]
