Propagate array item types from SQL to Avro schema

[shnapz]

Before the change it generated this:

  }, {
    "name" : "arr1",
    "type" : [ "null", {
      "type" : "array"
    } ],
    "doc" : "From sqlType 2003 ARRAY (java.sql.Array)",
    "default" : null,
    "typeName" : "ARRAY",
    "columnClassName" : "java.sql.Array",
    "sqlCode" : "2003",
    "columnName" : "arr1"
  } ],

after the change it generetes:

  }, {
    "name" : "arr1",
    "type" : [ "null", {
      "type" : "array",
      "items" : "string"
    } ],
    "doc" : "From sqlType 2003 ARRAY (java.sql.Array)",
    "default" : null,
    "typeName" : "ARRAY",
    "columnClassName" : "java.sql.Array",
    "sqlCode" : "2003",
    "columnName" : "arr1"
  } ],

• Executed e2e (Postgres) and manually inspected output
• Modified existing unit tests to include typed arrays and assert the correct behavior on in-memory H2 DB

Checklist for PR author(s)

• Changes are covered by unit tests (no major decrease in code coverage %) and/or integration tests.
• Ensure code formating (use mvn com.coveo:fmt-maven-plugin:format org.codehaus.mojo:license-maven-plugin:update-file-header)
• Document any relevant additions/changes in the appropriate spot in javadocs/docs/README.

[shnapz]

it is needed to call resultSet.getArray(i). Also it corresponds to the method name

[shnapz]

if you run e2e tests this change will result in:

  }, {
    "name" : "arr1",
    "type" : [ "null", {
      "type" : "array",
      "items" : "string"
    } ],
    "doc" : "From sqlType 2003 ARRAY (java.sql.Array)",
    "default" : null,
    "typeName" : "ARRAY",
    "columnClassName" : "java.sql.Array",
    "sqlCode" : "2003",
    "columnName" : "arr1"
  } ],

[codecov]

Codecov Report

Attention: Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.65%. Comparing base (9c72eac) to head (e913ef4).
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #931      +/-   ##
============================================
+ Coverage     91.47%   91.65%   +0.17%     
- Complexity      243      252       +9     
============================================
  Files            26       26              
  Lines           927      947      +20     
  Branches         67       71       +4     
============================================
+ Hits            848      868      +20     
  Misses           52       52              
  Partials         27       27              

[shnapz]

needed to get test output from container to the local folder. This is how I verified the change

[labianchin]

Nice!

1. Note this might be a schema breaking change for exports from ARRAY. I guess that is not too frequent, so it might be fine.
2. Consider adding/improving tests to JdbcAvroSchemaTest, JdbcAvroRecordTest. And perhaps adding array type to dbeam-core/src/test/java/com/spotify/dbeam/Coffee.java fixture.

[RustedBones]

Looking at the beam jdbc schema conversion here, it relies on the getColumnTypeName to infer the element type.

See in the tests:

• getColumnType -> ARRAY
• getColumnTypeName -> INTEGER

[shnapz]

Looking at the beam jdbc schema conversion here, it relies on the getColumnTypeName to infer the element type.

See in the tests:

• getColumnType -> ARRAY
• getColumnTypeName -> INTEGER

I am not in favor of Beam's approach, because getColumnTypeName does:
Retrieves the designated column's database-specific type name.
that in fact corresponds to Postgres's native types https://www.postgresql.org/docs/current/datatype.html like text, point, inet
and when Beam calls:
JDBCType.valueOf(md.getColumnTypeName(index));
it coincidentally works for all types that are common between JDBCType and database specific like integer, bigint, but won't work for anything else.
So there should be a logic like:

if (db == "postgres") {
  // match Postgres types type
} else if (db == "mysql") {
  //
}

but the solution I made doesn't need this at all, because resultSet.getArray(i).getBaseType() already returns the kind of type that our code already works with

[RustedBones]

Can you also update the doc here ?