[GitHub] [iotdb] ericpai commented on a diff in pull request #6747: [IOTDB-3923] Add Sorted Cluster Test ResultSet for the Function of last

[GitBox <gi...@apache.org>]

ericpai commented on code in PR #6747:
URL: https://github.com/apache/iotdb/pull/6747#discussion_r927241931


##########
jdbc/src/main/java/org/apache/iotdb/jdbc/IoTDBConnection.java:
##########
@@ -421,6 +421,8 @@ public void setClientInfo(String name, String value) throws SQLClientInfoExcepti
       } catch (TException | IoTDBSQLException e) {
         throw new SQLClientInfoException("Set time_zone error: ", null, e);
       }
+    } else if (name.equalsIgnoreCase("is_result_set_disorder")) {

Review Comment:
   Use `result_set_disordered` instead.



##########
integration-test/src/main/java/org/apache/iotdb/itbase/runtime/ClusterTestConnection.java:
##########
@@ -46,6 +46,7 @@ public class ClusterTestConnection implements Connection {
   private final NodeConnection writeConnection;
   private final List<NodeConnection> readConnections;
   private boolean isClosed;
+  private boolean isResultSetDisorder;

Review Comment:
   Use `resultSetDisordered` instead.



##########
integration-test/src/main/java/org/apache/iotdb/itbase/runtime/SortedClusterTestResultSet.java:
##########
@@ -0,0 +1,1218 @@
+/*
+ * 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.iotdb.itbase.runtime;
+
+import org.apache.iotdb.itbase.exception.InconsistentDataException;
+
+import java.io.InputStream;
+import java.io.Reader;
+import java.math.BigDecimal;
+import java.net.URL;
+import java.sql.Array;
+import java.sql.Blob;
+import java.sql.Clob;
+import java.sql.Date;
+import java.sql.NClob;
+import java.sql.Ref;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.RowId;
+import java.sql.SQLException;
+import java.sql.SQLWarning;
+import java.sql.SQLXML;
+import java.sql.Statement;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class SortedClusterTestResultSet implements ResultSet {
+
+  private final List<ResultSet> resultSets;
+  private final List<String> endpoints;
+  private final int queryTimeoutSeconds;
+  private final List<List<StringBuilder>> cachedResult = new ArrayList<>();
+
+  public SortedClusterTestResultSet(
+      List<Statement> statements, List<String> endpoints, String sql, int queryTimeoutSeconds)
+      throws SQLException {
+    this.queryTimeoutSeconds = queryTimeoutSeconds;
+    this.endpoints = endpoints;
+    RequestDelegate<ResultSet> delegate = createRequestDelegate();
+    for (Statement st : statements) {
+      delegate.addRequest(() -> st.executeQuery(sql));
+      cachedResult.add(new ArrayList<>());
+    }
+    resultSets = delegate.requestAll();
+  }
+
+  @Override
+  public boolean next() throws SQLException {
+    RequestDelegate<Boolean> delegate = createRequestDelegate();
+    for (ResultSet rs : resultSets) {
+      delegate.addRequest(rs::next);
+    }
+
+    boolean result = delegate.requestAllAndCompare();
+    if (result) {
+      for (List<StringBuilder> list : cachedResult) {
+        list.add(new StringBuilder());
+      }
+    }
+    return result;
+  }
+
+  @Override
+  public void close() throws SQLException {

Review Comment:
   As we have enriched the logic in `close()` when using disordered result set, it implies that the developers should remember to close the dataset after usage. Is there any way to force to check the `Connection`, `Statement` and `ResultSet` closed? Here I have two ideas:
   1. Use `checkstyle` to prevent unclosed resources, which will be a prefect and simple solution but I'm not sure whether `checkstyle` can really do this.
   2. When closing an upper layer resource, It should check whether the direct lower layer resources have been closed. Otherwise it should throw an Exception. So we need to record the allocations of Connection in an Env, allocations of Statement in an Connection, and the allocations of ResultSet in an Statement. After an Env is destoryed, it will check whether there are any unclosed connections. If so, thrown an Exception to tell the developers. So do the Connection and Statement.



##########
integration-test/src/main/java/org/apache/iotdb/itbase/runtime/SortedClusterTestResultSet.java:
##########
@@ -0,0 +1,1218 @@
+/*
+ * 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.iotdb.itbase.runtime;
+
+import org.apache.iotdb.itbase.exception.InconsistentDataException;
+
+import java.io.InputStream;
+import java.io.Reader;
+import java.math.BigDecimal;
+import java.net.URL;
+import java.sql.Array;
+import java.sql.Blob;
+import java.sql.Clob;
+import java.sql.Date;
+import java.sql.NClob;
+import java.sql.Ref;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.RowId;
+import java.sql.SQLException;
+import java.sql.SQLWarning;
+import java.sql.SQLXML;
+import java.sql.Statement;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class SortedClusterTestResultSet implements ResultSet {
+
+  private final List<ResultSet> resultSets;
+  private final List<String> endpoints;
+  private final int queryTimeoutSeconds;
+  private final List<List<StringBuilder>> cachedResult = new ArrayList<>();
+
+  public SortedClusterTestResultSet(
+      List<Statement> statements, List<String> endpoints, String sql, int queryTimeoutSeconds)
+      throws SQLException {
+    this.queryTimeoutSeconds = queryTimeoutSeconds;
+    this.endpoints = endpoints;
+    RequestDelegate<ResultSet> delegate = createRequestDelegate();
+    for (Statement st : statements) {
+      delegate.addRequest(() -> st.executeQuery(sql));
+      cachedResult.add(new ArrayList<>());
+    }
+    resultSets = delegate.requestAll();
+  }
+
+  @Override
+  public boolean next() throws SQLException {
+    RequestDelegate<Boolean> delegate = createRequestDelegate();
+    for (ResultSet rs : resultSets) {
+      delegate.addRequest(rs::next);
+    }
+
+    boolean result = delegate.requestAllAndCompare();
+    if (result) {
+      for (List<StringBuilder> list : cachedResult) {
+        list.add(new StringBuilder());
+      }
+    }
+    return result;
+  }
+
+  @Override
+  public void close() throws SQLException {
+    RequestDelegate<Void> delegate = createRequestDelegate();
+    for (ResultSet rs : resultSets) {
+      delegate.addRequest(
+          () -> {
+            rs.close();
+            return null;
+          });
+    }
+    delegate.requestAll();
+
+    List<String> result = new ArrayList<>();
+    for (StringBuilder item : cachedResult.get(0)) {
+      result.add(item.toString());
+      Collections.sort(result);
+    }
+
+    for (int i = 1; i < cachedResult.size(); i++) {
+      List<String> anotherResult = new ArrayList<>();
+
+      for (StringBuilder item : cachedResult.get(i)) {
+        anotherResult.add(item.toString());
+      }
+
+      Collections.sort(anotherResult);
+
+      if (!result.equals(anotherResult)) {

Review Comment:
   Use `Objects.equals` instead.



##########
integration-test/src/main/java/org/apache/iotdb/itbase/runtime/SortedClusterTestResultSet.java:
##########
@@ -0,0 +1,1218 @@
+/*
+ * 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.iotdb.itbase.runtime;
+
+import org.apache.iotdb.itbase.exception.InconsistentDataException;
+
+import java.io.InputStream;
+import java.io.Reader;
+import java.math.BigDecimal;
+import java.net.URL;
+import java.sql.Array;
+import java.sql.Blob;
+import java.sql.Clob;
+import java.sql.Date;
+import java.sql.NClob;
+import java.sql.Ref;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.RowId;
+import java.sql.SQLException;
+import java.sql.SQLWarning;
+import java.sql.SQLXML;
+import java.sql.Statement;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class SortedClusterTestResultSet implements ResultSet {
+
+  private final List<ResultSet> resultSets;
+  private final List<String> endpoints;
+  private final int queryTimeoutSeconds;
+  private final List<List<StringBuilder>> cachedResult = new ArrayList<>();
+
+  public SortedClusterTestResultSet(
+      List<Statement> statements, List<String> endpoints, String sql, int queryTimeoutSeconds)
+      throws SQLException {
+    this.queryTimeoutSeconds = queryTimeoutSeconds;
+    this.endpoints = endpoints;
+    RequestDelegate<ResultSet> delegate = createRequestDelegate();
+    for (Statement st : statements) {
+      delegate.addRequest(() -> st.executeQuery(sql));
+      cachedResult.add(new ArrayList<>());
+    }
+    resultSets = delegate.requestAll();
+  }
+
+  @Override
+  public boolean next() throws SQLException {
+    RequestDelegate<Boolean> delegate = createRequestDelegate();
+    for (ResultSet rs : resultSets) {
+      delegate.addRequest(rs::next);
+    }
+
+    boolean result = delegate.requestAllAndCompare();
+    if (result) {
+      for (List<StringBuilder> list : cachedResult) {
+        list.add(new StringBuilder());
+      }
+    }
+    return result;
+  }
+
+  @Override
+  public void close() throws SQLException {
+    RequestDelegate<Void> delegate = createRequestDelegate();
+    for (ResultSet rs : resultSets) {
+      delegate.addRequest(
+          () -> {
+            rs.close();
+            return null;
+          });
+    }
+    delegate.requestAll();
+
+    List<String> result = new ArrayList<>();
+    for (StringBuilder item : cachedResult.get(0)) {
+      result.add(item.toString());
+      Collections.sort(result);
+    }
+
+    for (int i = 1; i < cachedResult.size(); i++) {
+      List<String> anotherResult = new ArrayList<>();
+
+      for (StringBuilder item : cachedResult.get(i)) {
+        anotherResult.add(item.toString());
+      }
+
+      Collections.sort(anotherResult);
+
+      if (!result.equals(anotherResult)) {
+        throw new InconsistentDataException(cachedResult, endpoints);
+      }
+    }
+  }
+
+  private <T> T cachedResultAndReturnOne(List<T> data) {
+    for (int i = 0; i < data.size(); i++) {
+      List<StringBuilder> list = cachedResult.get(i);
+      list.get(list.size() - 1).append(data.get(i).toString()).append(",");

Review Comment:
   If `data.get(i)` returns `null`, there will be an NullPointerException thrown. I think it's better to use `List<Set<List<Object>>>` to cache the result data, which is designed as the follow:
   1. The first dimension is the node, which is the same as the current design.
   2. The second dimension is the row results of one node, as the row sequence is unpredictable, we use a set to do later   comparsion.
   3. The thrid dimension is the column result of one row, as we may have different data types in one row, we use a generic list to save them.
   
   Then we can simply use `Objects.equal(cachedResult.get(0), cachedResult.get(i))` to check whether they are consistent.



##########
integration-test/src/main/java/org/apache/iotdb/itbase/runtime/SortedClusterTestResultSet.java:
##########
@@ -0,0 +1,1218 @@
+/*
+ * 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.iotdb.itbase.runtime;
+
+import org.apache.iotdb.itbase.exception.InconsistentDataException;
+
+import java.io.InputStream;
+import java.io.Reader;
+import java.math.BigDecimal;
+import java.net.URL;
+import java.sql.Array;
+import java.sql.Blob;
+import java.sql.Clob;
+import java.sql.Date;
+import java.sql.NClob;
+import java.sql.Ref;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.RowId;
+import java.sql.SQLException;
+import java.sql.SQLWarning;
+import java.sql.SQLXML;
+import java.sql.Statement;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class SortedClusterTestResultSet implements ResultSet {

Review Comment:
   The name of this class is not consistent with the true meaning of its implementation. How about `DisorderedClusterTestResultSet` ?



-- 
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: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org