[CALCITE-3163] Master calcite 3163 avatica convert value fix

[asolimando]

This PR complements #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

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

[zabetak]

Hey @asolimando , many thanks for taking over this and enhancing the test coverage for Avatica.

I left some small comments around UtilTestCommon#assertResultSetFromArray mostly to see if we can do something to improve the readability of the tests relying on this.

[zabetak]

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.

[asolimando]

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, which should match the row 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.

[zabetak]

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.

[asolimando]

It's now AssertTestUtils

[zabetak]

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.

[asolimando]

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.

[zabetak]

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.

[asolimando]

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!