ptupitsyn commented on a change in pull request #326:
URL: https://github.com/apache/ignite-3/pull/326#discussion_r786559635



##########
File path: 
modules/api/src/main/java/org/apache/ignite/sql/ResultSetMetadata.java
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.sql;
+
+import java.util.List;
+
+/**
+ * ResultSet metadata.
+ */
+public interface ResultSetMetadata {
+    /**
+     * Returns number of columns that every row in a result set contains.
+     *
+     * @return Number of columns.
+     */
+    int columnsCount();

Review comment:
       Seems redundant to me since we can use `columns().size()`.

##########
File path: modules/api/src/main/java/org/apache/ignite/sql/IgniteSql.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql;
+
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Ignite SQL query facade.
+ */
+public interface IgniteSql {
+    /**
+     * Creates SQL session.

Review comment:
       ```suggestion
        * Creates an SQL session.
   ```

##########
File path: modules/api/src/main/java/org/apache/ignite/sql/IgniteSql.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql;
+
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Ignite SQL query facade.
+ */
+public interface IgniteSql {
+    /**
+     * Creates SQL session.
+     *
+     * @return A new session.
+     */
+    Session createSession();
+
+    /**
+     * Creates SQL statement.

Review comment:
       ```suggestion
        * Creates an SQL statement.
   ```

##########
File path: 
modules/api/src/main/java/org/apache/ignite/sql/async/AsyncResultSet.java
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.sql.async;
+
+import java.util.concurrent.CompletionStage;
+import org.apache.ignite.sql.NoRowSetExpectedException;
+import org.apache.ignite.sql.ResultSet;
+import org.apache.ignite.sql.ResultSetMetadata;
+import org.apache.ignite.sql.SqlRow;
+
+/**
+ * Asynchronous result set provides methods for query results processing in 
asynchronous way.
+ *
+ * @see ResultSet
+ */
+public interface AsyncResultSet {
+    /**
+     * Returns metadata for the results if the result contains rows ({@link 
#hasRowSet()} returns {@code true}).
+     *
+     * @return ResultSet metadata.
+     * @see ResultSet#metadata()
+     */
+    ResultSetMetadata metadata();
+
+    /**
+     * Returns whether the result of the query execution is a collection of 
rows, or not.
+     *
+     * <p>Note: when returns {@code false}, then calling {@link 
#currentPage()} will failed, and either {@link #updateCount()} return
+     * number of affected rows or {@link #applied()} returns {@code true}.
+     *
+     * @return {@code True} if the query returns rows, {@code false} otherwise.
+     */
+    boolean hasRowSet();
+
+    /**
+     * Returns number of rows affected by the query or {@code -1} if 
inapplicable.
+     *
+     * <p>Note: when returns {@code -1}, then either {@link #hasRowSet()} or 
{@link #applied()} returns {@code true}.
+     *
+     * @return Number of rows affected by the query or {@code -1} if 
inapplicable.
+     * @see ResultSet#updateCount()
+     */
+    int updateCount();
+
+    /**
+     * Returns whether the query that produce this result was a conditional 
query, or not. E.g. for the query "Create table if not exists"
+     * the method returns {@code true} when an operation was applied 
successfully, and {@code false} when an operation was ignored due to
+     * table was already existed.
+     *
+     * <p>Note: when returns {@code false}, then either {@link #updateCount()} 
return number of affected rows or {@link #hasRowSet()}
+     * returns {@code true}.
+     *
+     * @return {@code True} if conditional query applied, {@code false} 
otherwise.
+     * @see ResultSet#applied()
+     */
+    boolean applied();

Review comment:
       ```suggestion
       boolean isApplied();
   ```
   
   To be consistent with `hasRowSet`.

##########
File path: 
modules/api/src/main/java/org/apache/ignite/sql/async/AsyncResultSet.java
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.sql.async;
+
+import java.util.concurrent.CompletionStage;
+import org.apache.ignite.sql.NoRowSetExpectedException;
+import org.apache.ignite.sql.ResultSet;
+import org.apache.ignite.sql.ResultSetMetadata;
+import org.apache.ignite.sql.SqlRow;
+
+/**
+ * Asynchronous result set provides methods for query results processing in 
asynchronous way.
+ *
+ * @see ResultSet
+ */
+public interface AsyncResultSet {
+    /**
+     * Returns metadata for the results if the result contains rows ({@link 
#hasRowSet()} returns {@code true}).
+     *
+     * @return ResultSet metadata.
+     * @see ResultSet#metadata()
+     */
+    ResultSetMetadata metadata();
+
+    /**
+     * Returns whether the result of the query execution is a collection of 
rows, or not.
+     *
+     * <p>Note: when returns {@code false}, then calling {@link 
#currentPage()} will failed, and either {@link #updateCount()} return
+     * number of affected rows or {@link #applied()} returns {@code true}.
+     *
+     * @return {@code True} if the query returns rows, {@code false} otherwise.
+     */
+    boolean hasRowSet();
+
+    /**
+     * Returns number of rows affected by the query or {@code -1} if 
inapplicable.
+     *
+     * <p>Note: when returns {@code -1}, then either {@link #hasRowSet()} or 
{@link #applied()} returns {@code true}.
+     *
+     * @return Number of rows affected by the query or {@code -1} if 
inapplicable.
+     * @see ResultSet#updateCount()
+     */
+    int updateCount();

Review comment:
       `update` is a verb, so this method name sounds like "perform an update 
to Count property", which is confusing. Can we rename it to `affectedRows`?

##########
File path: 
modules/api/src/main/java/org/apache/ignite/sql/ResultSetMetadata.java
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.sql;
+
+import java.util.List;
+
+/**
+ * ResultSet metadata.
+ */
+public interface ResultSetMetadata {
+    /**
+     * Returns number of columns that every row in a result set contains.
+     *
+     * @return Number of columns.
+     */
+    int columnsCount();
+
+    /**
+     * Returns metadata with description for every column in result set.
+     *
+     * @return Columns metadata.
+     */
+    List<ColumnMetadata> columns();
+
+    /**
+     * Returns metadata for the requested column.
+     *
+     * @param columnIndex Column index in columns list.
+     * @return Column metadata.
+     */
+    ColumnMetadata column(int columnIndex);

Review comment:
       Same as above, can we just use `columns().get(index)` instead?

##########
File path: modules/api/src/main/java/org/apache/ignite/sql/IgniteSql.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql;
+
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Ignite SQL query facade.
+ */
+public interface IgniteSql {
+    /**
+     * Creates SQL session.
+     *
+     * @return A new session.
+     */
+    Session createSession();

Review comment:
       When do I use `createSession` and when do I use `createStatement`? It is 
not clear what exactly is the best way to execute queries, can you please 
improve the javadoc?

##########
File path: modules/api/src/main/java/org/apache/ignite/sql/IgniteSql.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql;
+
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Ignite SQL query facade.
+ */
+public interface IgniteSql {
+    /**
+     * Creates SQL session.
+     *
+     * @return A new session.
+     */
+    Session createSession();
+
+    /**
+     * Creates SQL statement.
+     *
+     * @param sql SQL query template.
+     * @return A new statement.
+     * @throws SqlException If parsing failed.
+     */
+    Statement createStatement(@NotNull String sql);
+
+    /**
+     * Creates prepared SQL statement.
+     *
+     * <p>Prepared statement forces query plan caching on the server side. In 
contrast to regular Statement, an identifier of the prepared
+     * query is send to the server instead of query string when possible. This 
might be useful for large and for "hot" queries.

Review comment:
       ```suggestion
        * query is sent to the server instead of query string when possible. 
This might be useful for large and for "hot" queries.
   ```




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