You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/02/21 18:12:55 UTC
[GitHub] [calcite-avatica] zabetak commented on a change in pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
zabetak commented on a change in pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#discussion_r579840222
##########
File path: core/src/test/java/org/apache/calcite/avatica/util/UtilTestCommon.java
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.calcite.avatica.util;
+
+import org.apache.calcite.avatica.ColumnMetaData;
+import org.apache.calcite.avatica.MetaImpl;
+
+import java.sql.Array;
+import java.sql.ResultSet;
+import java.util.Collections;
+import java.util.List;
+import java.util.function.BiFunction;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Common auxiliary methods for tests in util package.
+ */
+public class UtilTestCommon {
Review comment:
I am wondering if we could rename the class to better reflect its current purpose/content. If we want to use this class for assertions we could rename it to `AvaticaAssert`, `ArraysAssert`, `UtilAssert`, or something along these lines.
##########
File path: core/src/test/java/org/apache/calcite/avatica/util/AbstractCursorTest.java
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.calcite.avatica.util;
+
+import org.apache.calcite.avatica.ColumnMetaData;
+
+import org.junit.Test;
+
+import java.sql.Types;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.function.BiFunction;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test class for verifying functionality in abstract cursors.
+ */
+public class AbstractCursorTest {
+
+ private static final double DELTA = 1e-15;
+
+ @Test
+ public void resultSetFromIntegerArray() throws Exception {
Review comment:
The `AbsractCursor` class has many pieces and the name of the test does not indicate which part of it we are testing. Can we make it more specific?
Are we testing a specific accessor (e.g.,`ArrayAccessor`)? Are we testing some particular method of a specific accessor?
##########
File path: core/src/test/java/org/apache/calcite/avatica/util/UtilTestCommon.java
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.calcite.avatica.util;
+
+import org.apache.calcite.avatica.ColumnMetaData;
+import org.apache.calcite.avatica.MetaImpl;
+
+import java.sql.Array;
+import java.sql.ResultSet;
+import java.util.Collections;
+import java.util.List;
+import java.util.function.BiFunction;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Common auxiliary methods for tests in util package.
+ */
+public class UtilTestCommon {
+
+ private UtilTestCommon() {
+ // private constructor
+ }
+
+ static void assertResultSetFromArray(ColumnMetaData.ScalarType arrayComponentType,
Review comment:
As I wrote previously, I think we are doing many things in the same method thus its a bit hard to follow.
We could possibly break it down to simpler primitives (e.g., creation of the cursor, iteration through the results, validation etc.) distributing them possibly in other helper classes and calling those from the tests directly. This could possibly make the tests a bit more verbose but I think will be easier to understand.
##########
File path: core/src/test/java/org/apache/calcite/avatica/util/AbstractCursorTest.java
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.calcite.avatica.util;
+
+import org.apache.calcite.avatica.ColumnMetaData;
+
+import org.junit.Test;
+
+import java.sql.Types;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.function.BiFunction;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test class for verifying functionality in abstract cursors.
+ */
+public class AbstractCursorTest {
+
+ private static final double DELTA = 1e-15;
+
+ @Test
+ public void resultSetFromIntegerArray() throws Exception {
Review comment:
Moreover it is a bit confusing that we are testing the `AbstractCursor` but we see nowhere inside the test the creation of the cursor.
##########
File path: core/src/test/java/org/apache/calcite/avatica/util/AbstractCursorTest.java
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.calcite.avatica.util;
+
+import org.apache.calcite.avatica.ColumnMetaData;
+
+import org.junit.Test;
+
+import java.sql.Types;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.function.BiFunction;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test class for verifying functionality in abstract cursors.
+ */
+public class AbstractCursorTest {
+
+ private static final double DELTA = 1e-15;
+
+ @Test
+ public void resultSetFromIntegerArray() throws Exception {
+ BiFunction<Object, Object, Void> validator = (Object o1, Object o2) -> {
+ assertEquals((int) o1, (int) o2);
+ return null;
+ };
+
+ ColumnMetaData.ScalarType intType =
+ ColumnMetaData.scalar(Types.INTEGER, "INTEGER", ColumnMetaData.Rep.INTEGER);
+
+ List<List<Object>> rowsValues = Arrays.asList(Arrays.asList(1, 2),
+ Collections.singletonList(3), Arrays.asList(4, 5, 6));
+
+ UtilTestCommon.assertResultSetFromArray(intType, rowsValues, validator, false);
Review comment:
From the name and input params it is not clear what exactly we are asserting and I think the problem comes from the fact that many things happen inside the `assertResultSetFromArray` method.
Just wondering if we could rewrite the test using `assertThat(result, matcher)` style not necessarily using this particular API but clarifying what is the result and how we are matching/validating it. Adding a custom matcher maybe or use-case specific API may be better than using a general purpose API such as `BiFunction`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org