IGNITE-20661 JDBC. Support multi statement queries by JDBC by zstan · Pull Request #2906 · apache/ignite-3

zstan · 2023-11-30T08:07:51Z

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

zstan · 2023-12-01T06:14:08Z

@korlov42 can u make a review plz ?

...client-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryEventHandlerImpl.java

...mmon/src/main/java/org/apache/ignite/internal/jdbc/proto/event/JdbcGetMoreResultsResult.java

...client-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryEventHandlerImpl.java

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcResultSet.java

korlov42 · 2023-12-01T11:00:13Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java

        JdbcResultSet rs = resSets.get(curRes);

-        if (!rs.isQuery()) {
+        if (rs == null || !rs.isQuery()) {


if null is considering as legit element of resSets collection, then we need to mark generic type as @Nullable

this one was not addressed, as far as I can see

a few words about comments above:

at the line 407 you've added null checking for element of resSets collection. This implies that now null is considered legit item of the collection. If so, it's better to state in resSets declaration -- List<JdbcResultSet> resSets; --> List<@Nullable JdbcResultSet> resSets; . Now IDEA shows 4 more warning because of this annotation. Two of these warning actually may cause NPE

getResultSet() marked as nullable, @nullable JdbcResultSet rs = too, did i miss smth ?

i found that resSets has volatile notation seems our Statement implementation is not a thread safe at all.
apeend @nullable, fix nullability checks

should be fixed

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java

modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientHandlerModule.java

korlov42 · 2023-12-06T07:36:25Z

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/AbstractJdbcSelfTest.java

+        // a bit hack, instead of calling stmt.close(), gives a chance to catch potential forgiven cursor.
+        stmt.execute("SELECT 1");
+        ResultSet rs = stmt.getResultSet();
+        rs.close();


to be honest, I don't quite get the purpose of these lines

the very first execution of stmt.execute alredy registers a first cursor in resourses, thus for correct checking resource leakage without this approach i would need to make one of actions, or call :

stmt.close() - but this concrete implementation can work properly well while sequential .getResultset() can holds a bug.

always get last resultset in tests and forcibly close it.

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

xtern · 2023-12-08T09:06:01Z

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

+        assertEquals(false, res);
+        stmt.getResultSet();
+        stmt.getMoreResults();


Suggested change

assertEquals(false, res);

stmt.getResultSet();

stmt.getMoreResults();

assertFalse(res);

assertNull(stmt.getResultSet());

assertFalse(stmt.getMoreResults());

🤔

I understand you, but this case is not for check intermediate statement commands but about correct results after multiple calls, but ok, i fix it, it makes no harm.

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcStatementSelfTest.java

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

xtern · 2023-12-08T09:36:51Z

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java


 /**
- * Tests for ddl queries that contain multiply sql statements, separated by ";".
+ * Tests for queries that contain multiply sql statements, separated by ";".


please add some test(s) for autocommit=false (without TX_CONTROL statements) seems this should work fine,

i think we need to check it after IGNITE-20463

seems need to be addressed

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

korlov42 · 2023-12-12T14:16:39Z

...lient-common/src/main/java/org/apache/ignite/internal/jdbc/proto/JdbcQueryCursorHandler.java

+     * @param req Results request.
+     * @return Result future.
+     */
+    CompletableFuture<JdbcQuerySingleResult> getMoreResultsAsync(JdbcQueryFetchRequest req);


usage of JdbcQueryFetchRequest as an input argument to getMoreResults handler kinda misleading. Let's introduce separate class

probably we need to rename somehow ? but hold additional equal class just different action seems not so good too, wdyt ?

I don't mind to rename it, but have no good options for name.

btw, what is wrong with new class?

korlov42 · 2023-12-12T14:27:14Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcResultSet.java

+
+            close0(true);
+
+            if (!res.hasResults()) {


what if we got an error from handler? Let's add a test to cover this case

append test

probably I'm missing something, but I still no sight of error handling. Could you point out to me on particular test that covers failed response?

ItJdbcMultiStatementSelfTest#testSimpleQueryError got an error from handler, or you are talking about something more ?

as for now - we stop further script execution if exception is raised, i.e.

CREATE TABLE test (id INT PRIMARY KEY); CREATE TABLE test (id INT PRIMARY KEY); SELECT 1;

we can`t obtain SELECT 1 results, thus it seems enough, or you think that some mock are still necessary here ?

Seems we need additional check here, something like

if (res.status() == STATUS_FAILED) { throw createJdbcSqlException(res.err(), res.status()); }

If the JdbcQueryCursorHandler returns a future with an error, we are not handling it properly here.

(see related comment in JdbcQueryCursorHandlerImpl class)

done, append additional handler test

korlov42 · 2023-12-12T14:31:45Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java

        JdbcResultSet rs = resSets.get(curRes);

-        if (!rs.isQuery()) {
+        if (rs == null || !rs.isQuery()) {


this one was not addressed, as far as I can see

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

korlov42 · 2023-12-12T14:36:01Z

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

+        assertFalse(res);
+        assertNull(stmt.getResultSet());
+        assertFalse(stmt.getMoreResults());
+        assertEquals(-1, getResultSetSize());


if you want to check updateCount, please do it explicitly

change getResultSetSize javadoc

Fully support aforementioned comments. Why don't check update Count explicitly?

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcResultSet.java

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcStatementSelfTest.java

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcPreparedStatement.java

korlov42 · 2023-12-14T10:07:28Z

...lient-common/src/main/java/org/apache/ignite/internal/jdbc/proto/JdbcQueryCursorHandler.java

+     * @param req Results request.
+     * @return Result future.
+     */
+    CompletableFuture<JdbcQuerySingleResult> getMoreResultsAsync(JdbcQueryFetchRequest req);


I don't mind to rename it, but have no good options for name.

btw, what is wrong with new class?

korlov42 · 2023-12-14T10:38:17Z

...-common/src/main/java/org/apache/ignite/internal/jdbc/proto/event/JdbcQuerySingleResult.java

     */
    public JdbcQuerySingleResult(long cursorId, List<BinaryTupleReader> rowTuples, List<ColumnType> columnTypes, int[] decimalScales,
-            boolean last) {
+            boolean last, boolean isQuery, long updateCnt) {


we already have contractor for DML statements, so let's use that one

this particular constructor was used to create result for statement returning result set. With that said, it's better keep it as is (without additional isQuery and updateCnt) to avoid confusion and possible misusage

...client-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryEventHandlerImpl.java

korlov42 · 2023-12-14T10:50:18Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcResultSet.java

+
+            close0(true);
+
+            if (!res.hasResults()) {


probably I'm missing something, but I still no sight of error handling. Could you point out to me on particular test that covers failed response?

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcResultSet.java

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

xtern · 2023-12-18T08:53:53Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcResultSet.java

+
+            close0(true);
+
+            if (!res.hasResults()) {


Seems we need additional check here, something like

if (res.status() == STATUS_FAILED) { throw createJdbcSqlException(res.err(), res.status()); }

If the JdbcQueryCursorHandler returns a future with an error, we are not handling it properly here.

(see related comment in JdbcQueryCursorHandlerImpl class)

...lient-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryCursorHandlerImpl.java

xtern · 2023-12-18T15:47:09Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java

                case CLOSE_CURRENT_RESULT:
                    if (curRes > 0) {
-                        resSets.get(curRes - 1).close0();
+                        resSets.get(curRes - 1).close0(false);


Why we are calling close0 with false flag here?

The following example demonstrates resource leakage (or may be I missed something):

@Test public void testX() throws Exception { long initial = openCursorsRegistered(); stmt.executeUpdate("CREATE TABLE TESTX(ID INT PRIMARY KEY)"); stmt.getMoreResults(); stmt.close(); assertEquals(0L, initial - openCursorsRegistered()); }

As I see, the current combination in close0 of isQuery, finished, etc. flags were being used because we were not registering cursors for DML, DDL (and possibly other) queries.

Since we are now registering cursors for this types of queries, I think it is worth reviewing the current conditions in close0, because now they look too complex and leading to errors 😎 .

this is connection context , append below to test and it will pass:

conn.close(); assertTrue(waitForCondition(() -> 0L == openCursorsRegistered(), 5000));

Yes you are right. I missed this.
But as I see, now - getNextResultSet() already closes resultset with enforce=true, why we need this second call here?

korlov42 · 2023-12-20T12:34:28Z

...-common/src/main/java/org/apache/ignite/internal/jdbc/proto/event/JdbcQuerySingleResult.java

     */
    public JdbcQuerySingleResult(long cursorId, List<BinaryTupleReader> rowTuples, List<ColumnType> columnTypes, int[] decimalScales,
-            boolean last) {
+            boolean last, boolean isQuery, long updateCnt) {


this particular constructor was used to create result for statement returning result set. With that said, it's better keep it as is (without additional isQuery and updateCnt) to avoid confusion and possible misusage

korlov42 · 2023-12-20T12:35:13Z

...-common/src/main/java/org/apache/ignite/internal/jdbc/proto/event/JdbcQuerySingleResult.java

+     */
+    public JdbcQuerySingleResult(boolean hasNext, long updCount) {
+        hasResults = hasNext;
+        this.updateCnt = updCount;


if there are no results, why should we provide updateCount?

korlov42 · 2023-12-20T12:36:11Z

...-common/src/main/java/org/apache/ignite/internal/jdbc/proto/event/JdbcQuerySingleResult.java

    @Override
    public void writeBinary(ClientMessagePacker packer) {
        super.writeBinary(packer);
+        packer.packLong(updateCnt);


if there are no results, we should not pack updateCount

korlov42 · 2023-12-20T13:06:08Z

...lient-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryCursorHandlerImpl.java

+
+                            List<ColumnMetadata> columns = cur.metadata().columns();
+
+                            return buildSingleRequest(batch, columns, cursorId, !batch.hasMore(), queryType);


I see that first page response for first statement and next statement are created differently: for first statement only subset of query types goes through buildSingleRequest, while for next statement all types goes through this method. Let's align the creation of result for the first page

korlov42 · 2023-12-20T13:15:01Z

...t-handler/src/test/java/org/apache/ignite/client/handler/JdbcQueryCursorHandlerImplTest.java

+        CompletableFuture<JdbcQuerySingleResult> fut = IgniteTestUtils.runAsync(() ->
+                await(cursorHandler.getMoreResultsAsync(new JdbcFetchQueryResultsRequest(1, 100)), 5, TimeUnit.SECONDS)
+        );
+
+        try {
+            await(fut, 5, TimeUnit.SECONDS);
+        } catch (Throwable e) {
+            fail("Unexpected exception is raised.");
+        }


to be honest, I don't quite understand what you are trying to verify here...

I verify that JdbcQueryCursorHandlerImpl correctly handles exceptions raised from AsyncSqlCursor, do you think it`s redundant ?

korlov42 · 2023-12-20T13:24:50Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java

+    @Nullable
    public ResultSet getResultSet() throws SQLException {


Suggested change

@Nullable

public ResultSet getResultSet() throws SQLException {

public @Nullable ResultSet getResultSet() throws SQLException {

korlov42 · 2023-12-20T13:48:18Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java

        JdbcResultSet rs = resSets.get(curRes);

-        if (!rs.isQuery()) {
+        if (rs == null || !rs.isQuery()) {


a few words about comments above:

at the line 407 you've added null checking for element of resSets collection. This implies that now null is considered legit item of the collection. If so, it's better to state in resSets declaration -- List<JdbcResultSet> resSets; --> List<@Nullable JdbcResultSet> resSets; . Now IDEA shows 4 more warning because of this annotation. Two of these warning actually may cause NPE

korlov42 · 2023-12-20T13:56:26Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java

+            throw exceptionally;
+        }
+
+        return nextResultSet != null && nextResultSet.holdResults();


this condition is not quite correct: empty result set still result set, thus true should be returned (try this query SELECT 1; SELECT 1 FROM table(system_range(1, 0)))

changed, not here but logic at all

korlov42 · 2023-12-20T13:57:23Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java


                default:
-                    throw new SQLException("Invalid 'current' parameter.");
+                    throw new SQLException("Invalid 'curr' parameter.");


better to use current because it is how it named in interface

korlov42 · 2023-12-20T14:26:55Z

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcResultSet.java

    @Override
    public void close() throws SQLException {
-        close0();
+        closed = true;


all resources must be cleaned up as soon as possible, it's not enough to just pretend that result set is closed

zstan · 2023-12-21T15:34:23Z

@xtern @korlov42 guys, seems i finish. There is 1 question about test from me, check above. Nullability checks .. idea due to volatile notation highlight all places there this value used no matter exist previous not null check or not.

...-common/src/main/java/org/apache/ignite/internal/jdbc/proto/event/JdbcQuerySingleResult.java

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java

ygerzhedovich · 2023-12-27T13:53:14Z

...-common/src/main/java/org/apache/ignite/internal/jdbc/proto/event/JdbcQuerySingleResult.java

+            if (cursorId != null) {
+                packer.packLong(cursorId);
+            } else {
+                packer.packNil();
+            }


Suggested change

if (cursorId != null) {

packer.packLong(cursorId);

} else {

packer.packNil();

}

packer.packLongNullable(cursorId);

ygerzhedovich · 2023-12-27T15:47:05Z

...t-handler/src/test/java/org/apache/ignite/client/handler/JdbcQueryCursorHandlerImplTest.java

+            }
+        });
+
+        try {


what reason to have the try/catch block?

look, this test check that no exception is raised from JdbcQueryCursorHandlerImpl#getMoreResultsAsync
comment exception handling in mention method and try to run test.

zstan force-pushed the ignite-20661 branch 3 times, most recently from 866604b to 5923904 Compare December 1, 2023 05:41

korlov42 previously requested changes Dec 1, 2023

View reviewed changes

zstan force-pushed the ignite-20661 branch from 00c8a19 to 9e24ac2 Compare December 5, 2023 13:08

zstan requested a review from korlov42 December 6, 2023 05:47

korlov42 reviewed Dec 6, 2023

View reviewed changes

zstan requested a review from korlov42 December 7, 2023 11:09

xtern reviewed Dec 8, 2023

View reviewed changes

zstan force-pushed the ignite-20661 branch from 55b9def to f266ff3 Compare December 11, 2023 11:55

zstan requested a review from xtern December 11, 2023 12:23

korlov42 reviewed Dec 12, 2023

View reviewed changes

zstan force-pushed the ignite-20661 branch from 039c571 to 6ca04df Compare December 13, 2023 07:54

xtern reviewed Dec 13, 2023

View reviewed changes

modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcPreparedStatement.java Outdated Show resolved Hide resolved

korlov42 reviewed Dec 14, 2023

View reviewed changes

xtern reviewed Dec 15, 2023

View reviewed changes

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java Show resolved Hide resolved

xtern reviewed Dec 15, 2023

View reviewed changes

modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcMultiStatementSelfTest.java Show resolved Hide resolved

xtern reviewed Dec 18, 2023

View reviewed changes

zstan force-pushed the ignite-20661 branch from 676c1c4 to 10061c5 Compare December 20, 2023 06:28

korlov42 reviewed Dec 20, 2023

View reviewed changes

zstan force-pushed the ignite-20661 branch 2 times, most recently from 98af8e0 to 51ccd71 Compare December 21, 2023 15:13

xtern reviewed Dec 22, 2023

View reviewed changes

xtern approved these changes Dec 25, 2023

View reviewed changes

zstan force-pushed the ignite-20661 branch 2 times, most recently from dc98aa0 to ded00df Compare December 26, 2023 08:13

zstan added 19 commits December 27, 2023 15:34

fix

e6f3e4d

fix

1f568f7

fix test

23790f5

fix

ad2b16c

fix

192d9e5

handler test

d7dbabe

move test

0f3390f

rm exception

cab8b29

fix erroneous execution

c03d775

fix after review

2872bf9

minor fix

0f506d7

unit test

45b0c47

fix extends

388cce1

fix

7e38ad2

fix after review

4852ecb

fix

0ea6feb

fix

985cdb5

fix

0875d49

close all cursors

a01ab8b

zstan force-pushed the ignite-20661 branch from a11ff10 to a01ab8b Compare December 27, 2023 12:35

ygerzhedovich reviewed Dec 27, 2023

View reviewed changes

fix after review

c570bde

ygerzhedovich reviewed Dec 27, 2023

View reviewed changes

zstan added 2 commits December 28, 2023 13:38

fix after review 2

f27d97a

fix after review

0e2889c

ygerzhedovich approved these changes Dec 28, 2023

View reviewed changes

ygerzhedovich added 2 commits December 28, 2023 16:28

IGNITE-20661: fix failed test

ace0d54

Merge branch 'main' into ignite-20661

8aee13e

zstan merged commit 704b71d into apache:main Dec 29, 2023

zstan deleted the ignite-20661 branch December 29, 2023 06:48


		List<ColumnMetadata> columns = cur.metadata().columns();

		return buildSingleRequest(batch, columns, cursorId, !batch.hasMore(), queryType);

		@Nullable
		public ResultSet getResultSet() throws SQLException {

	@Nullable
	public ResultSet getResultSet() throws SQLException {
	public @Nullable ResultSet getResultSet() throws SQLException {

+                          }
+                      });
+                      try {

Conversation

zstan commented Nov 30, 2023

The Review Checklist

Notes

Uh oh!

zstan commented Dec 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtern Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtern Dec 18, 2023 •

edited

Loading

xtern Dec 18, 2023 •

edited

Loading

xtern Dec 18, 2023 •

edited

Loading