korlov42 commented on code in PR #1616:
URL: https://github.com/apache/ignite-3/pull/1616#discussion_r1094256124


##########
modules/api/src/main/java/org/apache/ignite/sql/ResultSet.java:
##########
@@ -32,7 +32,7 @@
  * <p>Note: one and only one of following is possible: {@link #hasRowSet()} 
returns {@code true}, or {@link #wasApplied()} returns
  * {@code true}, or {@link #affectedRows()} return zero or higher value.
  */
-public interface ResultSet extends Iterator<SqlRow>, AutoCloseable {
+public interface ResultSet<T> extends Iterator<T>, AutoCloseable {

Review Comment:
   let's add description of type parameter to javadoc



##########
modules/api/src/main/java/org/apache/ignite/sql/Session.java:
##########
@@ -68,11 +69,57 @@ default ResultSet execute(@Nullable Transaction 
transaction, String query, @Null
      * @param arguments Arguments for the statement.
      * @return SQL query results set.
      */
-    default ResultSet execute(@Nullable Transaction transaction, Statement 
statement, @Nullable Object... arguments) {
+    default ResultSet<SqlRow> execute(@Nullable Transaction transaction, 
Statement statement, @Nullable Object... arguments) {
         Objects.requireNonNull(statement);
 
         try {
-            return new SyncResultSetAdapter(executeAsync(transaction, 
statement, arguments).join());
+            return new SyncResultSetAdapter<>(executeAsync(transaction, 
statement, arguments).join());
+        } catch (CompletionException e) {
+            throw IgniteException.wrap(e);
+        }
+    }
+
+    /**
+     * Executes single SQL statement and maps results to objects with the 
provided mapper.
+     *
+     * @param transaction Transaction to execute the statement within or 
{@code null}.
+     * @param mapper Mapper.
+     * @param query SQL query template.
+     * @param arguments Arguments for the statement.
+     * @return SQL query results set.
+     */
+    default <T> ResultSet<T> execute(

Review Comment:
   and again, let's add description for type parameter here and every where in 
public API.



##########
modules/api/src/main/java/org/apache/ignite/sql/Session.java:
##########
@@ -98,7 +145,42 @@ default ResultSet execute(@Nullable Transaction 
transaction, Statement statement
      * @return Operation future.
      * @throws SqlException If failed.
      */
-    CompletableFuture<AsyncResultSet> executeAsync(@Nullable Transaction 
transaction, Statement statement, @Nullable Object... arguments);
+    CompletableFuture<AsyncResultSet<SqlRow>> executeAsync(
+            @Nullable Transaction transaction,
+            Statement statement,
+            @Nullable Object... arguments);
+
+    /**
+     * Executes SQL statement in an asynchronous way and maps results to 
objects with the provided mapper.
+     *
+     * @param transaction Transaction to execute the statement within or 
{@code null}.
+     * @param mapper Mapper.
+     * @param query SQL query template.
+     * @param arguments Arguments for the statement.
+     * @return Operation future.
+     * @throws SqlException If failed.

Review Comment:
   this is probably copy-pasted javadoc but anyways... async method won't throw 
any exception, but complete the future exceptionally



##########
modules/api/src/main/java/org/apache/ignite/sql/Session.java:
##########
@@ -68,11 +69,57 @@ default ResultSet execute(@Nullable Transaction 
transaction, String query, @Null
      * @param arguments Arguments for the statement.
      * @return SQL query results set.
      */
-    default ResultSet execute(@Nullable Transaction transaction, Statement 
statement, @Nullable Object... arguments) {
+    default ResultSet<SqlRow> execute(@Nullable Transaction transaction, 
Statement statement, @Nullable Object... arguments) {
         Objects.requireNonNull(statement);
 
         try {
-            return new SyncResultSetAdapter(executeAsync(transaction, 
statement, arguments).join());
+            return new SyncResultSetAdapter<>(executeAsync(transaction, 
statement, arguments).join());
+        } catch (CompletionException e) {
+            throw IgniteException.wrap(e);
+        }
+    }
+
+    /**
+     * Executes single SQL statement and maps results to objects with the 
provided mapper.
+     *
+     * @param transaction Transaction to execute the statement within or 
{@code null}.
+     * @param mapper Mapper.

Review Comment:
   I think we need to provide better description for `mapper` field. This is a 
public API, the user must clearly understand what to pass and how it will 
affect the result 



##########
modules/api/src/main/java/org/apache/ignite/sql/async/AsyncResultSet.java:
##########
@@ -48,7 +47,7 @@
  *
  * @see ResultSet
  */
-public interface AsyncResultSet {
+public interface AsyncResultSet<T> {

Review Comment:
   let's add description of type parameter to javadoc



##########
modules/client/src/test/java/org/apache/ignite/client/fakes/FakeSession.java:
##########
@@ -90,6 +95,23 @@ public CompletableFuture<AsyncResultSet> executeAsync(
         return CompletableFuture.completedFuture(new FakeAsyncResultSet(this, 
transaction, statement, arguments));
     }
 
+    /** {@inheritDoc} */
+    @Override
+    public <T> CompletableFuture<AsyncResultSet<T>> executeAsync(@Nullable 
Transaction transaction, @Nullable Mapper<T> mapper,
+            String query, @Nullable Object... arguments) {
+        return null;

Review Comment:
   let's throw `UnsupportedOperationException` here and below. I believe this 
is better than NPE



-- 
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]

Reply via email to