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/20 20:08:20 UTC
[GitHub] [calcite-avatica] asolimando opened a new pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
asolimando opened a new pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139
This PR complements https://github.com/apache/calcite-avatica/pull/105/ by adding unit-tests for the fix, and improving coverage for Double/Float/Real in classes with tests related to the mapping of these data types (notably, `ArrayImplTest.java` and `ArrayTypeTest.java`).
The PR has 5 commits:
1. improved test coverage for avatica/util/ArrayImplTest.java
2. reduced warnings in avatica/remote/ArrayTypeTest.java
3. improved test coverage for avatica/remote/ArrayTypeTest
4. [CALCITE-3163] Mapping of Float and Real in AbstractCursor#convertValue() does not adhere to JDBC specifications (unit-tests added)
5. [CALCITE-3163] Mapping of Float and Real in AbstractCursor#convertValue() does not adhere to JDBC specifications
What each commit does:
1. Adds coverage for Float/Double/Real array component (before only Integer was covered)
2. Reduces few warnings (lambda replacing anonymous classes, redundant boxing, double negation in JUnit assert)
3. Added coverage to `Float` and `Real` (the latter treated "specially" since hsqldb does not adhere to JDBC standard, it maps `Real` to `Double` instead of `Float, it took me while to understand why the test was working, as it is now it's explicit, it serves as documentation and might save time to people down the line)
4. The unit-tests for the fix strictly speaking
5. The original commit
4. and 5. must be kept, 1-3 are optional but I think they add value,
If 1-3 are dropped, I'd merge the two classes introduced in commit '2', since `UtilTestCommon` would have a method used only by `AbstractCursorTest`, it would make little sense.
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] asolimando commented on a change in pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
asolimando commented on a change in pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#discussion_r580496281
##########
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:
I have tried to split the methods (as discussed in another comment), now `static void assertRowsValuesMatchCursorContentViaArrayAccessor` mainly iterates over the cursor by creating an array accessor.
I have introduced two additional helper classes `ColumnMetadataTestUtils` (to simplify the handling of `ColumnMetadata`, pretty repetitive and error prone, and it opens the door to support more types via JUnit Parameterized class later on), and `CursorTestUtils` (to help creating Array/List/ArrayImpl-based cursors more easily).
Validation is delegated to the concrete implementations of the `Validator` interface, which can be as simple as a lambda if you don't need much like in the tests I have added.
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] asolimando edited a comment on pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
asolimando edited a comment on pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#issuecomment-786739740
> Hey @asolimando, I did a few changes on top your work and rebased in https://github.com/zabetak/calcite-avatica/tree/calcite-3163-reb. Can you have a final look? If everything is good I will push it as is.
Hi @zabetak , thanks a lot for the changes, it looks much better in this way.
I just realized there is a package for "common" test utilities: `package org.apache.calcite.avatica.test`.
I feel that `AvaticaMatchers.java` and the other `*TestUtils` classes would fit better there other than under `org.apache.calcite.avatica.util` where I originally put the test helper classes.
In any case LGTM, thanks again!
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] asolimando commented on a change in pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
asolimando commented on a change in pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#discussion_r580485973
##########
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:
It's now `AssertTestUtils`
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] zabetak commented on pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
zabetak commented on pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#issuecomment-786705212
Hey @asolimando, I did a few changes on top your work and rebased in https://github.com/zabetak/calcite-avatica/tree/calcite-3163-reb. Can you have a final look? If everything is good I will push it as is.
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] asolimando edited a comment on pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
asolimando edited a comment on pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#issuecomment-786739740
> Hey @asolimando, I did a few changes on top your work and rebased in https://github.com/zabetak/calcite-avatica/tree/calcite-3163-reb. Can you have a final look? If everything is good I will push it as is.
Hi @zabetak , thanks a lot for the changes, it looks much better in this way.
I just realized there is a package for "common" test utilities: `package org.apache.calcite.avatica.test`.
I feel that `AvaticaMatchers.java`, `IsArrayAccessorResultSetEqual.java` and the other `*TestUtils.java` files would fit better there other than under `org.apache.calcite.avatica.util` where I originally put the test helper classes.
In any case LGTM, thanks again!
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] zabetak closed pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] asolimando edited a comment on pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
asolimando edited a comment on pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#issuecomment-786739740
> Hey @asolimando, I did a few changes on top your work and rebased in https://github.com/zabetak/calcite-avatica/tree/calcite-3163-reb. Can you have a final look? If everything is good I will push it as is.
Hi @zabetak , thanks a lot for the changes, it looks much better in this way.
I just realized there is a package for "common" test utilities: `package org.apache.calcite.avatica.test`.
I feel that `AvaticaMatchers.java` would fit better there other than under `org.apache.calcite.avatica.util` where I originally put my test helper class.
In any case LGTM, thanks again!
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] asolimando commented on a change in pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
asolimando commented on a change in pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#discussion_r579867777
##########
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:
It's indeed more appropriate to rename the test `ArrayAccessor` test, because that's specifically the `AbstractCursor` component used here. It's the whole class, so no need to refine further I'd say.
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] zabetak commented on a change in pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [calcite-avatica] asolimando commented on a change in pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
asolimando commented on a change in pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#discussion_r580491936
##########
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:
I have tried to split it along different subcomponents, now the signature is as follow:
```
static void assertRowsValuesMatchCursorContentViaArrayAccessor(
List<List<Object>> rowsValues, ColumnMetaData.ScalarType arrayContentMetadata,
Cursor cursorOverArray, ColumnMetaData arrayMetaData, ArrayImpl.Factory factory,
Validator validator) throws Exception
```
In a nut-shell, the method check that a collection of values (`rowsValues`) matches the values retrieved by a cursor via the array accessor (the input values are a List<List>, which should match the row<array> the cursor exposes).
What "matches" means is defined via the input `Validator` (I moved away from the generic interface to adopt a per-test one, as suggested).
The method signature is still a bit long unfortunately, but the metadata and factory are needed to properly instantiate the accessor. Moving the accessor as parameter would remove 2 parameters and add 1, and to me it makes sense to delegate the accessor creation to the method.
----------------------------------------------------------------
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
[GitHub] [calcite-avatica] asolimando commented on pull request #139: [CALCITE-3163] Master calcite 3163 avatica convert value fix
Posted by GitBox <gi...@apache.org>.
asolimando commented on pull request #139:
URL: https://github.com/apache/calcite-avatica/pull/139#issuecomment-786739740
> Hey @asolimando, I did a few changes on top your work and rebased in https://github.com/zabetak/calcite-avatica/tree/calcite-3163-reb. Can you have a final look? If everything is good I will push it as is.
Hi Stamatis, thanks a lot for the changes, it looks much better in this way.
I just realized there is a package for "common" test utilities: `package org.apache.calcite.avatica.test`.
I feel that `AvaticaMatchers.java` would fit better there other than under `org.apache.calcite.avatica.util` where I originally put my test helper class.
In any case LGTM, thanks again!
----------------------------------------------------------------
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