This is an automated email from the ASF dual-hosted git repository. ptupitsyn pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/ignite-3.git
The following commit(s) were added to refs/heads/main by this push: new 354352ed7a IGNITE-21011 Fix SQL script API in Java client (#2917) 354352ed7a is described below commit 354352ed7af1efcb7ee0a868390a32bd64a27549 Author: Pavel Tupitsyn <ptupit...@apache.org> AuthorDate: Mon Dec 4 17:20:12 2023 +0200 IGNITE-21011 Fix SQL script API in Java client (#2917) * Fix protocol - propagate session details correctly, skip `pageSize` for scripts * Fix `ItSqlClientAsynchronousApiTest` and `ItSqlClientSynchronousApiTest` - remove overloaded methods, they were missing `@Test` annotation, and we don't need an override anyway * Add script and properties propagation tests to `ClientSqlTest` --- .../internal/client/proto/ClientMessagePacker.java | 2 +- .../requests/sql/ClientSqlExecuteRequest.java | 10 ++++-- .../sql/ClientSqlExecuteScriptRequest.java | 26 ++++++++++++-- .../ignite/internal/client/sql/ClientSession.java | 37 ++++++++++++-------- .../org/apache/ignite/client/ClientSqlTest.java | 39 +++++++++++++++++++++ .../ignite/client/fakes/FakeAsyncResultSet.java | 10 ++++-- .../apache/ignite/client/fakes/FakeIgniteSql.java | 4 ++- .../apache/ignite/client/fakes/FakeSession.java | 40 ++++++++++++++++++++-- .../ignite/client/fakes/FakeSessionBuilder.java | 8 ++++- .../sql/api/ItSqlClientAsynchronousApiTest.java | 10 ------ .../sql/api/ItSqlClientSynchronousApiTest.java | 12 ------- 11 files changed, 149 insertions(+), 49 deletions(-) diff --git a/modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientMessagePacker.java b/modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientMessagePacker.java index e2ef846964..5d660fe66b 100644 --- a/modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientMessagePacker.java +++ b/modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientMessagePacker.java @@ -262,7 +262,7 @@ public class ClientMessagePacker implements AutoCloseable { * * @param v the value to be written. */ - public void packLongNullable(Long v) { + public void packLongNullable(@Nullable Long v) { if (v == null) { packNil(); } else { diff --git a/modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlExecuteRequest.java b/modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlExecuteRequest.java index 702c5f6325..993c4e97f9 100644 --- a/modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlExecuteRequest.java +++ b/modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlExecuteRequest.java @@ -157,7 +157,7 @@ public class ClientSqlExecuteRequest { return statementBuilder.build(); } - static Session readSession(ClientMessageUnpacker in, IgniteSql sql, @Nullable IgniteTransactions transactions) { + private static Session readSession(ClientMessageUnpacker in, IgniteSql sql, @Nullable IgniteTransactions transactions) { SessionBuilder sessionBuilder = sql.sessionBuilder(); if (transactions != null && sessionBuilder instanceof SessionBuilderImpl) { @@ -179,14 +179,18 @@ public class ClientSqlExecuteRequest { sessionBuilder.idleTimeout(in.unpackLong(), TimeUnit.MILLISECONDS); } + readSessionProperties(in, sessionBuilder); + + return sessionBuilder.build(); + } + + static void readSessionProperties(ClientMessageUnpacker in, SessionBuilder sessionBuilder) { var propCount = in.unpackInt(); var reader = new BinaryTupleReader(propCount * 4, in.readBinaryUnsafe()); for (int i = 0; i < propCount; i++) { sessionBuilder.property(reader.stringValue(i * 4), ClientBinaryTupleUtils.readObject(reader, i * 4 + 1)); } - - return sessionBuilder.build(); } private static void packMeta(ClientMessagePacker out, @Nullable ResultSetMetadata meta) { diff --git a/modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlExecuteScriptRequest.java b/modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlExecuteScriptRequest.java index 0bdfe3dcbe..7b6a84dba0 100644 --- a/modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlExecuteScriptRequest.java +++ b/modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlExecuteScriptRequest.java @@ -17,15 +17,17 @@ package org.apache.ignite.client.handler.requests.sql; -import static org.apache.ignite.client.handler.requests.sql.ClientSqlExecuteRequest.readSession; +import static org.apache.ignite.client.handler.requests.sql.ClientSqlExecuteRequest.readSessionProperties; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; import org.apache.ignite.internal.client.proto.ClientMessageUnpacker; import org.apache.ignite.internal.hlc.HybridTimestamp; import org.apache.ignite.internal.tx.impl.IgniteTransactionsImpl; import org.apache.ignite.internal.util.ArrayUtils; import org.apache.ignite.sql.IgniteSql; import org.apache.ignite.sql.Session; +import org.apache.ignite.sql.Session.SessionBuilder; /** * Client SQL execute script request. @@ -43,7 +45,7 @@ public class ClientSqlExecuteScriptRequest { IgniteSql sql, IgniteTransactionsImpl transactions ) { - Session session = readSession(in, sql, null); + Session session = readSession(in, sql); String script = in.unpackString(); Object[] arguments = in.unpackObjectArrayFromBinaryTuple(); @@ -59,4 +61,24 @@ public class ClientSqlExecuteScriptRequest { return session.executeScriptAsync(script, arguments); } + + private static Session readSession(ClientMessageUnpacker in, IgniteSql sql) { + SessionBuilder sessionBuilder = sql.sessionBuilder(); + + if (!in.tryUnpackNil()) { + sessionBuilder.defaultSchema(in.unpackString()); + } + + if (!in.tryUnpackNil()) { + sessionBuilder.defaultQueryTimeout(in.unpackLong(), TimeUnit.MILLISECONDS); + } + + if (!in.tryUnpackNil()) { + sessionBuilder.idleTimeout(in.unpackLong(), TimeUnit.MILLISECONDS); + } + + readSessionProperties(in, sessionBuilder); + + return sessionBuilder.build(); + } } diff --git a/modules/client/src/main/java/org/apache/ignite/internal/client/sql/ClientSession.java b/modules/client/src/main/java/org/apache/ignite/internal/client/sql/ClientSession.java index 448f2d7b09..1a6a10e3c4 100644 --- a/modules/client/src/main/java/org/apache/ignite/internal/client/sql/ClientSession.java +++ b/modules/client/src/main/java/org/apache/ignite/internal/client/sql/ClientSession.java @@ -154,7 +154,7 @@ public class ClientSession implements AbstractSession { w.out().packLongNullable(defaultSessionTimeout); - packProperties(w, clientStatement.properties()); + packProperties(w, properties, clientStatement.properties()); w.out().packString(clientStatement.query()); @@ -228,6 +228,12 @@ public class ClientSession implements AbstractSession { Objects.requireNonNull(query); PayloadWriter payloadWriter = w -> { + w.out().packString(defaultSchema); + w.out().packLongNullable(defaultQueryTimeout); + w.out().packLongNullable(defaultSessionTimeout); + + packProperties(w, properties, null); + w.out().packString(query); w.out().packObjectArrayAsBinaryTuple(arguments); w.out().packLong(ch.observableTimestamp()); @@ -303,39 +309,42 @@ public class ClientSession implements AbstractSession { throw new UnsupportedOperationException("Not implemented yet."); } - private void packProperties(PayloadOutputChannel w, Map<String, Object> props) { + private static void packProperties( + PayloadOutputChannel w, + @Nullable Map<String, Object> sessionProps, + @Nullable Map<String, Object> statementProps) { int size = 0; - if (props != null) { - size += props.size(); + if (statementProps != null) { + size += statementProps.size(); } // Statement properties override session properties. - if (properties != null) { - if (props != null) { - for (String k : properties.keySet()) { - if (!props.containsKey(k)) { + if (sessionProps != null) { + if (statementProps != null) { + for (String k : sessionProps.keySet()) { + if (!statementProps.containsKey(k)) { size++; } } } else { - size += properties.size(); + size += sessionProps.size(); } } w.out().packInt(size); var builder = new BinaryTupleBuilder(size * 4); - if (props != null) { - for (Entry<String, Object> entry : props.entrySet()) { + if (statementProps != null) { + for (Entry<String, Object> entry : statementProps.entrySet()) { builder.appendString(entry.getKey()); ClientBinaryTupleUtils.appendObject(builder, entry.getValue()); } } - if (properties != null) { - for (Entry<String, Object> entry : properties.entrySet()) { - if (props == null || !props.containsKey(entry.getKey())) { + if (sessionProps != null) { + for (Entry<String, Object> entry : sessionProps.entrySet()) { + if (statementProps == null || !statementProps.containsKey(entry.getKey())) { builder.appendString(entry.getKey()); ClientBinaryTupleUtils.appendObject(builder, entry.getValue()); } diff --git a/modules/client/src/test/java/org/apache/ignite/client/ClientSqlTest.java b/modules/client/src/test/java/org/apache/ignite/client/ClientSqlTest.java index 3fe72fa7c1..ca567169f0 100644 --- a/modules/client/src/test/java/org/apache/ignite/client/ClientSqlTest.java +++ b/modules/client/src/test/java/org/apache/ignite/client/ClientSqlTest.java @@ -52,6 +52,7 @@ import org.junit.jupiter.api.Test; /** * SQL tests. */ +@SuppressWarnings("resource") public class ClientSqlTest extends AbstractClientTableTest { @Test public void testExecuteAsync() { @@ -215,4 +216,42 @@ public class ClientSqlTest extends AbstractClientTableTest { assertEquals(BigInteger.valueOf(42), row.value(17)); assertEquals(ColumnType.NUMBER, meta.columns().get(17).type()); } + + @Test + public void testExecuteScript() { + Session session = client.sql().createSession(); + + session.executeScript("foo"); + + ResultSet<SqlRow> resultSet = session.execute(null, "SELECT LAST SCRIPT"); + SqlRow row = resultSet.next(); + + assertEquals( + "foo, arguments: [], properties: [], defaultPageSize=null, defaultSchema=null, " + + "defaultQueryTimeout=null, defaultSessionTimeout=null", + row.value(0)); + } + + @Test + public void testExecuteScriptWithPropertiesAndArguments() { + Session session = client.sql().sessionBuilder() + .property("prop1", "val1") + .property("prop2", -5) + .property("prop3", null) + .defaultPageSize(123) // Should be ignored - not applicable to scripts. + .defaultQueryTimeout(456, TimeUnit.MILLISECONDS) + .defaultSchema("script-schema") + .idleTimeout(789, TimeUnit.SECONDS) + .build(); + + session.executeScript("do bar baz", "arg1", null, 2); + + ResultSet<SqlRow> resultSet = session.execute(null, "SELECT LAST SCRIPT"); + SqlRow row = resultSet.next(); + + assertEquals( + "do bar baz, arguments: [arg1, null, 2, ], properties: [prop2=-5, prop1=val1, prop3=null, ], " + + "defaultPageSize=null, defaultSchema=script-schema, defaultQueryTimeout=456, defaultSessionTimeout=789000", + row.value(0)); + } } diff --git a/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeAsyncResultSet.java b/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeAsyncResultSet.java index 5e96bbb538..acb7d6d719 100644 --- a/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeAsyncResultSet.java +++ b/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeAsyncResultSet.java @@ -62,6 +62,8 @@ public class FakeAsyncResultSet implements AsyncResultSet { private final boolean hasMorePages; + private final FakeIgniteSql sql; + /** * Constructor. * @@ -70,7 +72,7 @@ public class FakeAsyncResultSet implements AsyncResultSet { * @param statement Statement. * @param arguments Arguments. */ - public FakeAsyncResultSet(Session session, Transaction transaction, Statement statement, Object[] arguments) { + public FakeAsyncResultSet(Session session, Transaction transaction, Statement statement, Object[] arguments, FakeIgniteSql sql) { assert session != null; assert statement != null; @@ -78,6 +80,7 @@ public class FakeAsyncResultSet implements AsyncResultSet { this.transaction = transaction; this.statement = statement; this.arguments = arguments; + this.sql = sql; hasMorePages = session.property("hasMorePages") != null; @@ -142,6 +145,9 @@ public class FakeAsyncResultSet implements AsyncResultSet { BigInteger.valueOf(42)); rows = List.of(row); + } else if ("SELECT LAST SCRIPT".equals(statement.query())) { + rows = List.of(getRow(sql.lastScript)); + columns = List.of(new FakeColumnMetadata("script", ColumnType.STRING)); } else { rows = List.of(getRow(1)); columns = List.of(new FakeColumnMetadata("col1", ColumnType.INT32)); @@ -197,7 +203,7 @@ public class FakeAsyncResultSet implements AsyncResultSet { /** {@inheritDoc} */ @Override public CompletableFuture<? extends AsyncResultSet> fetchNextPage() { - return CompletableFuture.completedFuture(new FakeAsyncResultSet(session, transaction, statement, arguments)); + return CompletableFuture.completedFuture(new FakeAsyncResultSet(session, transaction, statement, arguments, sql)); } /** {@inheritDoc} */ diff --git a/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeIgniteSql.java b/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeIgniteSql.java index 2c3d69ed5b..7095c58fd5 100644 --- a/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeIgniteSql.java +++ b/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeIgniteSql.java @@ -28,6 +28,8 @@ import org.apache.ignite.sql.Statement.StatementBuilder; * Fake SQL implementation. */ public class FakeIgniteSql implements IgniteSql { + String lastScript; + @Override public Session createSession() { return sessionBuilder().build(); @@ -35,7 +37,7 @@ public class FakeIgniteSql implements IgniteSql { @Override public SessionBuilder sessionBuilder() { - return new FakeSessionBuilder(); + return new FakeSessionBuilder(this); } @Override diff --git a/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeSession.java b/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeSession.java index 43f677b34d..9a0e79eed1 100644 --- a/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeSession.java +++ b/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeSession.java @@ -58,6 +58,8 @@ public class FakeSession implements AbstractSession { @Nullable private final Map<String, Object> properties; + private final FakeIgniteSql sql; + /** * Constructor. * @@ -65,6 +67,7 @@ public class FakeSession implements AbstractSession { * @param defaultSchema Default schema. * @param defaultQueryTimeout Default timeout. * @param properties Properties. + * @param sql SQL. */ @SuppressWarnings("AssignmentOrReturnOfFieldWithMutableType") public FakeSession( @@ -72,12 +75,14 @@ public class FakeSession implements AbstractSession { @Nullable String defaultSchema, @Nullable Long defaultQueryTimeout, @Nullable Long defaultSessionTimeout, - @Nullable Map<String, Object> properties) { + @Nullable Map<String, Object> properties, + FakeIgniteSql sql) { this.defaultPageSize = defaultPageSize; this.defaultSchema = defaultSchema; this.defaultQueryTimeout = defaultQueryTimeout; this.defaultSessionTimeout = defaultSessionTimeout; this.properties = properties; + this.sql = sql; } /** {@inheritDoc} */ @@ -101,7 +106,7 @@ public class FakeSession implements AbstractSession { return CompletableFuture.failedFuture(new SqlException(STMT_VALIDATION_ERR, "Query failed")); } - return CompletableFuture.completedFuture(new FakeAsyncResultSet(this, transaction, statement, arguments)); + return CompletableFuture.completedFuture(new FakeAsyncResultSet(this, transaction, statement, arguments, sql)); } /** {@inheritDoc} */ @@ -178,7 +183,36 @@ public class FakeSession implements AbstractSession { /** {@inheritDoc} */ @Override public CompletableFuture<Void> executeScriptAsync(String query, @Nullable Object... arguments) { - throw new UnsupportedOperationException(); + var sb = new StringBuilder(query); + + if (arguments != null) { + sb.append(", arguments: ["); + + for (Object arg : arguments) { + sb.append(arg).append(", "); + } + + sb.append(']'); + } + + if (properties != null) { + sb.append(", properties: ["); + + for (Map.Entry<String, Object> entry : properties.entrySet()) { + sb.append(entry.getKey()).append('=').append(entry.getValue()).append(", "); + } + + sb.append(']'); + } + + sb.append(", ").append("defaultPageSize=").append(defaultPageSize); + sb.append(", ").append("defaultSchema=").append(defaultSchema); + sb.append(", ").append("defaultQueryTimeout=").append(defaultQueryTimeout); + sb.append(", ").append("defaultSessionTimeout=").append(defaultSessionTimeout); + + sql.lastScript = sb.toString(); + + return nullCompletedFuture(); } /** {@inheritDoc} */ diff --git a/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeSessionBuilder.java b/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeSessionBuilder.java index 9c71662fb1..3d8876167a 100644 --- a/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeSessionBuilder.java +++ b/modules/client/src/test/java/org/apache/ignite/client/fakes/FakeSessionBuilder.java @@ -31,6 +31,8 @@ import org.jetbrains.annotations.Nullable; public class FakeSessionBuilder implements SessionBuilder { private final Map<String, Object> properties = new HashMap<>(); + private final FakeIgniteSql sql; + private String defaultSchema; private Long defaultQueryTimeoutMs; @@ -39,6 +41,10 @@ public class FakeSessionBuilder implements SessionBuilder { private Integer pageSize; + public FakeSessionBuilder(FakeIgniteSql sql) { + this.sql = sql; + } + /** {@inheritDoc} */ @Override public long defaultQueryTimeout(TimeUnit timeUnit) { @@ -118,6 +124,6 @@ public class FakeSessionBuilder implements SessionBuilder { /** {@inheritDoc} */ @Override public Session build() { - return new FakeSession(pageSize, defaultSchema, defaultQueryTimeoutMs, defaultSessionTimeoutMs, new HashMap<>(properties)); + return new FakeSession(pageSize, defaultSchema, defaultQueryTimeoutMs, defaultSessionTimeoutMs, new HashMap<>(properties), sql); } } diff --git a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlClientAsynchronousApiTest.java b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlClientAsynchronousApiTest.java index 200029fc80..2b2e174fd3 100644 --- a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlClientAsynchronousApiTest.java +++ b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlClientAsynchronousApiTest.java @@ -68,14 +68,4 @@ public class ItSqlClientAsynchronousApiTest extends ItSqlAsynchronousApiTest { public void testLockIsNotReleasedAfterTxRollback() { super.testLockIsNotReleasedAfterTxRollback(); } - - @Override - public void runScriptThatCompletesSuccessfully() { - super.runScriptThatCompletesSuccessfully(); - } - - @Override - public void runScriptThatFails() { - super.runScriptThatFails(); - } } diff --git a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlClientSynchronousApiTest.java b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlClientSynchronousApiTest.java index f69a054fc2..fa48bff50f 100644 --- a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlClientSynchronousApiTest.java +++ b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlClientSynchronousApiTest.java @@ -31,8 +31,6 @@ import org.junit.jupiter.api.Disabled; public class ItSqlClientSynchronousApiTest extends ItSqlSynchronousApiTest { private IgniteClient client; - private static final int ROW_COUNT = 16; - @BeforeAll public void startClient() { client = IgniteClient.builder().addresses(getClientAddresses(List.of(CLUSTER.aliveNode())).get(0)).build(); @@ -70,14 +68,4 @@ public class ItSqlClientSynchronousApiTest extends ItSqlSynchronousApiTest { public void testLockIsNotReleasedAfterTxRollback() { super.testLockIsNotReleasedAfterTxRollback(); } - - @Override - public void runScriptThatCompletesSuccessfully() { - super.runScriptThatCompletesSuccessfully(); - } - - @Override - public void runScriptThatFails() { - super.runScriptThatFails(); - } }